-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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(uishell): provide tabindex as a named prop #10900
Conversation
✔️ Deploy Preview for carbon-react-next ready! 🔨 Explore the source changes: 223da3f 🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-react-next/deploys/622bdbb2dc34250009872b54 😎 Browse the preview: https://deploy-preview-10900--carbon-react-next.netlify.app |
✔️ Deploy Preview for carbon-components-react ready! 🔨 Explore the source changes: 223da3f 🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-components-react/deploys/622bdbb2f006a700091700a5 😎 Browse the preview: https://deploy-preview-10900--carbon-components-react.netlify.app |
✔️ Deploy Preview for carbon-elements ready! 🔨 Explore the source changes: 223da3f 🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-elements/deploys/622bdbb262f8e500086ee4b1 😎 Browse the preview: https://deploy-preview-10900--carbon-elements.netlify.app |
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.
For situations like this, would it be helpful if our convention was to make this an explicit/named prop?
This would allow us to safely override things that we need to control and allow a way to add in support for things over time. There are definitely things that should always be configurable (like tabindex
) that we should straighten out but figured this could be a good heuristic going forward. Curious what you think
Is it possible to make a similar change here? We're running into the same issue trying to override the tabIndex for this component: https://github.com/carbon-design-system/carbon/blob/main/packages/react/src/components/UIShell/HeaderMenuItem.js#L37 |
@joshblack Great point, I made them named props. @ColbyJohnIBM I also made the update in |
Surfaced in slack:
This just moves the tabindex to be set beforeThis makes...rest
so it can be overriddentabIndex
a named and configurable prop.Changelog
Changed
tabIndex
as a prop forSwitcherItem
andHeaderMenuItem
Testing / Reviewing