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

fix: radio accessiblity #25

Merged
merged 6 commits into from
Jun 24, 2019
Merged

fix: radio accessiblity #25

merged 6 commits into from
Jun 24, 2019

Conversation

jsomsanith-tlnd
Copy link
Contributor

What is the problem this PR is trying to solve?
Radio component is not accessible

  1. non focusable and ignored by SR because of visibility: hidden
  2. label is not required
  3. errors are set as labels
  4. checkbox with errors are not set as invalid
  5. missing focus visual aspect for keyboard navigation
  6. subLabels are set as labels

What is the chosen solution to this problem?

  1. make input visible again, but outside of viewport
  2. label is required, introducing hideLabel props. Be careful to give a clear context when you hide the label in the designs
  3. errors are in a div next to the label, associated with input via aria-describedby
  4. use aria-invalid on input when there is an error
  5. use the same style as checkbox for focus state
  6. subLabels are not linked as descriptions. Should we rename it ?

@jsomsanith-tlnd jsomsanith-tlnd changed the title fix: radio aaccessiblity fix: radio accessiblity Jun 23, 2019
@kylesuss
Copy link
Contributor

kylesuss commented Jun 24, 2019

🎉

Thanks for another contribution @jsomsanith ! This is great.

I see a slight difference in the alignment of the description (the old subLabel):

https://www.chromaticqa.com/snapshot?appId=5ccbc373887ca40020446347&id=5d0fee6c77b75e0020f10492

Looks left aligned now vs offset from the left. Is that something you can take a look at before we merge this?

padding: 0px !important;
position: absolute !important;
white-space: nowrap !important;
width: 1px !important;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Were these !important tags necessary to make the styles apply correctly? Or are they more for being explicit about the behavior? Does look to me like they should work without !important, but maybe I am missing something.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is extra safety to make sure no inherited styles collide. 👍

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kylesuss Just for extra info, this part has !important which is usually veeeeery bad, but this is a standard trick to make it rendered out of the viewport, but still visible by screen readers.
Every css framework has a class with this type of css, for example bootstrap has sr-only.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. Thanks for the explanation @jsomsanith . Makes sense!

Copy link
Contributor

@kylesuss kylesuss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯

@kylesuss kylesuss merged commit a5a3312 into master Jun 24, 2019
@kylesuss kylesuss deleted the jsomsanith/fix/radio_a11y branch June 24, 2019 16:26
@kylesuss
Copy link
Contributor

🚀 PR was released in v0.0.26 🚀

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

Successfully merging this pull request may close these issues.

4 participants