-
Notifications
You must be signed in to change notification settings - Fork 273
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
feat(switcher): automate max height, closes on bg click, update disabled tab index #870
feat(switcher): automate max height, closes on bg click, update disabled tab index #870
Conversation
…carbon into fix-global-spacing
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/carbon-design-system/gatsby-theme-carbon/lbhxqp4uk |
packages/gatsby-theme-carbon/src/components/Switcher/Switcher.js
Outdated
Show resolved
Hide resolved
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.
the build is failing :(
packages/gatsby-theme-carbon/src/components/Switcher/Switcher.js
Outdated
Show resolved
Hide resolved
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.
Lookin good just some minor stuff
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.
lgtm! :) @jnm2377
packages/gatsby-theme-carbon/src/components/Switcher/Switcher.js
Outdated
Show resolved
Hide resolved
@jnm2377 the disabled thing is actually intended. You want disabled form elements to be tabbable. Otherwise users navigating by keyboard won't know they exist at all, which is a different thing than the "exists but isn't selectable" info that sited users get from a disabled element. |
@vpicone we don't do that in any of our carbon components. For example if a button is disabled, we it's not tabbable. How does that make sense here? |
@jnm2377 I think you're right just because they're disabled doesn't mean they're invisible to screen readers. Not sure if there's any screen reader hints that the link is disabled or if it'd show up as normal text. But either way I think taking them out of the tab order is the right move here. |
@dakahn just want to confirm this is ok to do. We currently have a disabled item in the switcher and it's still tabbable. In this PR, I updated switcher items to not be tabbable if they're disabled. Is that ok? It seems like that's the pattern we do for everything else that's disabled. |
@jnm2377 yup, I'm on board with your change. It's weird because there is no 'disabled' attribute for native links on the web (like there is for buttons or other for inputs), so its sort of a weird gray area but I think sticking to the Carbon |
@vpicone looks like the changes are working correctly 🎉 |
packages/gatsby-theme-carbon/src/components/Switcher/Switcher.module.scss
Outdated
Show resolved
Hide resolved
🚀 |
Closes #840
Closes #856
As well as fixes a bug I found:
Changed