Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

A11y improvment (part 1) #552

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

basile-parent
Copy link
Contributor

@basile-parent basile-parent commented Jul 24, 2024

First part of the improvments proposed in #544 :

  • removing the "radio" attribute on buttons when it's not relevant
  • properly grouping the relevant radio button

image

The only thing I could not do is to translate the new locale entry to all languages

@basile-parent basile-parent changed the title A11y improvment A11y improvment (part 1) Jul 24, 2024
@basile-parent basile-parent mentioned this pull request Jul 24, 2024
2 tasks
@petyosi
Copy link
Contributor

petyosi commented Jul 24, 2024

Thank you for the PR.

The project has prettier configured, but for some reason, your PR uses a different formatting which results in a massive change - this makes it very hard for me to review it; the build won't pass as well.

Can you fix the formatting? I don't think that the change you mean to introduce should change 300+lines.

@basile-parent
Copy link
Contributor Author

Yeah, I don't know why it don't apply... I'll fix that and push a new commit

@basile-parent
Copy link
Contributor Author

@petyosi I think we are good now

@petyosi
Copy link
Contributor

petyosi commented Jul 25, 2024

The current state of the PR has typescript failing and hooks with missing dependencies. Also, it changes the spacing of the toolbar, making it wider:

image

@basile-parent
Copy link
Contributor Author

The missing imports were so weird... It should have worked at all 🤔 Anyway, it's fixed.

I also fixed the spacing but had to make some compromises to have the exact same render : I had to externalize the <div className={styles.toolbarGroupOfGroups}>. If not, there are additional margins on the sup/sub formatting (only difference).
If you think this difference is acceptable, I can rollback this change.

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants