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(StructuredList): add role attribute to StructuredListRow #8898

Merged
merged 4 commits into from
Jun 16, 2021
Merged

fix(StructuredList): add role attribute to StructuredListRow #8898

merged 4 commits into from
Jun 16, 2021

Conversation

atikenny
Copy link
Contributor

Relates to #4828

Fix the accessibility problem in the StrcuturedListRow.

Changelog

Changed

  • added the row role to the StructuredListRow

Testing / Reviewing

Check the accessibility of the component in Storybook using the IBM Able Chrome plugin.

Note

Tried to fix the related issue about the missing role for the label but that invalidates another rule that labels cannot have roles specified.

@atikenny atikenny requested a review from a team as a code owner June 14, 2021 11:33
@github-actions
Copy link
Contributor

github-actions bot commented Jun 14, 2021

DCO Assistant Lite bot All contributors have signed the DCO.

@atikenny
Copy link
Contributor Author

I have read the DCO document and I hereby sign the DCO.

@netlify
Copy link

netlify bot commented Jun 14, 2021

✔️ Deploy Preview for carbon-elements ready!

🔨 Explore the source changes: 56c75a7

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-elements/deploys/60ca5bf4bbf0a200074225f1

😎 Browse the preview: https://deploy-preview-8898--carbon-elements.netlify.app

@netlify
Copy link

netlify bot commented Jun 14, 2021

✔️ Deploy Preview for carbon-components-react ready!
Built without sensitive environment variables

🔨 Explore the source changes: 56c75a7

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-components-react/deploys/60ca5bf442a80c000807eab7

😎 Browse the preview: https://deploy-preview-8898--carbon-components-react.netlify.app/iframe

@tay1orjones
Copy link
Member

Fix the accessibility problem in the StrcuturedListRow.

@atikenny could you clarify what the specific problem is that this change solves?

StructuredList was updated recently with some accessibility improvements #8172, though I believe they're all behind the v11 flag for now. @dakahn was this addressed as part of it - moving away from the table role to the grid role?

@tay1orjones tay1orjones requested a review from dakahn June 14, 2021 19:26
@atikenny
Copy link
Contributor Author

Fix the accessibility problem in the StrcuturedListRow.

@atikenny could you clarify what the specific problem is that this change solves?

StructuredList was updated recently with some accessibility improvements #8172, though I believe they're all behind the v11 flag for now. @dakahn was this addressed as part of it - moving away from the table role to the grid role?

This PR fixes the original problem with the table role missing a row role in it.

@andreancardona andreancardona requested review from andreancardona and removed request for andreancardona June 15, 2021 19:31
Copy link
Contributor

@dakahn dakahn left a comment

Choose a reason for hiding this comment

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

@tay1orjones we have a total accessibility overhaul behind our feature flags right now yeah. But as for fixing existing problems with v10 StructuredList I'm all for that. I still see 16 errors for role="row" in my Accessibility Checker though 👀

@atikenny
Copy link
Contributor Author

@tay1orjones we have a total accessibility overhaul behind our feature flags right now yeah. But as for fixing existing problems with v10 StructuredList I'm all for that. I still see 16 errors for role="row" in my Accessibility Checker though 👀

I tried fixing those as well, but unfortunately, I wasn't able to (it used to be 20 problems now it's 16).
The problem is that instead of a table we use a collection of divs and labels. But we would also like to use a label as a row in that table. This is impossible with the table design, but we cannot ditch the label as then we would lose the selection use-case of this component.

Copy link
Contributor

@dakahn dakahn left a comment

Choose a reason for hiding this comment

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

i mean i'm cool with this. It fixes four errors 🏄🏽

Copy link
Member

@tay1orjones tay1orjones left a comment

Choose a reason for hiding this comment

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

Thanks for the input @dakahn!

@kodiakhq kodiakhq bot merged commit 2d53daa into carbon-design-system:main Jun 16, 2021
@jnm2377 jnm2377 mentioned this pull request Jun 22, 2021
22 tasks
@atikenny atikenny deleted the structured-list-a11y-fixes branch July 23, 2021 12:01
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.

4 participants