-
Notifications
You must be signed in to change notification settings - Fork 15
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(chip): add tabIndex prop #1435
base: master
Are you sure you want to change the base?
Conversation
🚀 Deployed on https://pr-1435--dhis2-ui.netlify.app |
Passing run #3145 ↗︎
Details:
Review all test suite changes for PR #1435 ↗︎ |
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.
Nice 👍
This allows keyboard focus for the main chip body, but can we propagate that down to the <Remove>
child component for removable
type chips? Potentially out of scope here, so approved without this.
@cooper-joe thanks for your quick input. After testing this in storybook, it seems like this is not enough to actually make The only problem of making this a button is if you wanted the chip to be a link - |
I would prefer this to be a button rather than hack together our own keypress handling. Alas, wrapping the chip in the link is exactly how it's being used, so looks like we may not have a choice unless we're prepared to release the breaking change. I'm open to the the breaking change, if it means a step towards better accessibility. |
Description
Adds
tabIndex
to chip component.I followed the pattern for the use of other
tabIndex
, but I wonder why this is astring
and notnumber
? Also in my opinion these tabindex should default to0
- but they don't? So I decided to keep it aligned with other components.Known issues
Checklist
All points above should be relevant for feature PRs. For bugfixes, some points might not be relevant. In that case, just check them anyway to signal the work is done.