feat(Chip): accessibility improvements #1471
Conversation
|
🚀 Deployed on https://pr-1471--dhis2-ui.netlify.app |
Passing run #3424 ↗︎
Details:
Review all test suite changes for PR #1471 ↗︎ |
|||||||||||||||
components/chip/src/chip/chip.js
Outdated
| } | ||
|
|
||
| return ( | ||
| <button |
There was a problem hiding this comment.
Chips are mainly used for links. Button is definitely better than the previous span, but I think we should look into how we would support links, instead of having to do the navigation imperatively in onClick handler. It's not valid HTML to wrap a button in a link, so this component would have to support it.
It might be worth supporting a component-prop, and href as props. See how material UI does it: https://mui.com/material-ui/react-chip/#clickable-link . A lot of times you would want to use a Chip with eg. react-routers NavLink.
There was a problem hiding this comment.
it sounds correct .. but not in the scope of this PR, right? we can add a separate ticket for it
There was a problem hiding this comment.
You're right, but we may want to release that together. I brought it up because this PR changes the underlying element from span to a button. And wrapping a button in anchor tag is not valid, so it's potentially breaking .
There was a problem hiding this comment.
It is a breaking change for sure.
Two things:
Links should not have the role = option.
The accessibility features require that each chip be a direct child of the chipGroup.
I can change the button back to span and handle the enter key event for now or add a component, to & href props which in this case will pass the role=option to links.
if you have another way of doing it suggest it please. @Birkbjo @kabaros
There was a problem hiding this comment.
very good PR, not an easy one to implement - two things missing for me:
-
I am able to navigate through the chips, but I don't get a visual feedback that a chip is selected. They should have a focused style similar to buttons. (see screenshot below)
-
Please also add a StoryBook to show the new component ChipGroup, and update the documentation page (for Chip, or separate one) to mention this new component and its use.
| if (!onRemove) { | ||
| return | ||
| } | ||
| if (event.key === 'Backspace' || event.key === 'Delete') { |
There was a problem hiding this comment.
I'd say also event.stopPropagation here
components/chip/src/chip/chip.js
Outdated
| } | ||
|
|
||
| return ( | ||
| <button |
There was a problem hiding this comment.
it sounds correct .. but not in the scope of this PR, right? we can add a separate ticket for it
Implements LIBS-552
Description
Implement:
LIBS-553
LIBS-554
LIBS-555
Checklist