-
-
Notifications
You must be signed in to change notification settings - Fork 32.3k
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
[docs] Improve a11y of the chip array example #20294
Conversation
Details of bundle changes.Comparing: 981d471...a9a3cce Details of page changes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this. This seems good but I'd like to test this with various screen readers first.
What about this demo https://material-ui.com/components/chips/#chip-array? Should we update it too? This effort makes me think of https://vuetifyjs.com/en/components/chip-groups/#chip-groups. |
I don't think we need to update it because all Chips are read like buttons. We don't have a demo with only static Chips to add the
Vuetify's Chip group takes another approach by using the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this something we should recommend independent of the chip variant? Whether the items are are clickable or not should not change their grouping semantics. So I would like to see a similar approach for the chip array.
It's probably better to use <Paper component="ul">
and wrap each chip in an <li>
. This should be closer to the actual semantics which incidentally fixes the a11y issues.
@m4theushw What do you think of updating the pull request to match the direction @eps1lon has suggested? (ul > li) |
@oliviertassinari Demo updated.
Maybe, in the future a container component for Chips could be created to abstract the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! Combined with #20470, I think that we are good :)
@m4theushw Great work, thanks! |
Closes #20172