-
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): use hr instead of li with role #14723
fix(UIShell): use hr instead of li with role #14723
Conversation
✅ Deploy Preview for v11-carbon-react ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for carbon-elements ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
I don't believe it's valid html to have a other than that it looks fine and doesn't have any a11y checker errors 🤔 Likely the correct html for this would require two separate lists with a hr in between (likely not a simple update) |
https://deploy-preview-14723--v11-carbon-react.netlify.app/iframe.html?args=&id=components-ui-shell-sidenav--fixed-side-nav-w-divider&viewMode=story I think it depends on where it's being used. |
@alisonjoseph can we just wrap it in an |
@tw15egan oh yea, good idea. I don't see why not 👍 |
@alisonjoseph updated 👍 |
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, no a11y errors
Hey there! v11.39.0 was just released that references this issue/PR. |
Closes #14479
Refactors UI Shell Side Nav Separator to use an
hr
instead of ali
with `role="separator" to fix an a11y issueChangelog
Changed
li
tohr
, added styles to match currentRemoved
Testing / Reviewing
Run a11y checker on a UI Shell story with a side nav separator and ensure there are no violations