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

Add aria-label to dropdown component #1110

Conversation

meron-yemane
Copy link
Contributor

This dropdown component was causing 'Accessibility check failed!' errors in my cypress tests. Below is a screenshot of the produced error. Simple fix was just to add an aria-label to the component.
Screen Shot 2021-08-06 at 9 41 32 AM

@@ -70,6 +70,7 @@ export const DesktopDropdown = <T extends OptionType>({
aria-haspopup="menu"
role="button"
ref={initialFocus}
aria-label="dropdown"
Copy link
Contributor Author

@meron-yemane meron-yemane Aug 6, 2021

Choose a reason for hiding this comment

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

Would we want aria-label to refer to the dropdown as a whole or the specific option label that's currently in focus? aria-label={currentOption ? currentOption.label : 'dropdown'}

@michaeljaltamirano
Copy link
Contributor

This dropdown component was causing 'Accessibility check failed!' errors in my cypress tests. Below is a screenshot of the produced error. Simple fix was just to add an aria-label to the component.
Screen Shot 2021-08-06 at 9 41 32 AM

Is this Cypress failure somewhere on CI (or on the Cypress dashboard) where I could take a look, or is it local dev?

@meron-yemane
Copy link
Contributor Author

This dropdown component was causing 'Accessibility check failed!' errors in my cypress tests. Below is a screenshot of the produced error. Simple fix was just to add an aria-label to the component.
Screen Shot 2021-08-06 at 9 41 32 AM

Is this Cypress failure somewhere on CI (or on the Cypress dashboard) where I could take a look, or is it local dev?

The error is occurring locally. I'm pushing up the tests now so you can take a look but it seems to pass fine in CI. Interesting because the accessibility tab in Radiance for the dropdown component also points out the exact same issues I was seeing locally. Heres the PR

Copy link
Contributor

@michaeljaltamirano michaeljaltamirano left a comment

Choose a reason for hiding this comment

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

I think there's room for improvement in our component definition, especially for ways to encourage happy path a11y behavior (and enforce it in our Storybook environment to prevent the issues it is reporting), but a fix for your issue can be found in userland: https://github.com/curology/PocketDerm/pull/14014/files#r684494367

@michaeljaltamirano
Copy link
Contributor

Closing in favor of the fix in userland mentioned above ^^^^

I've merged in #1117 to fix some misleading accessibility violations in our Storybook app, which includes mention of 3 new issues to fix the remaining issues, at least a couple of which are legitimate a11y issues in the library.

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

Successfully merging this pull request may close these issues.

3 participants