-
Notifications
You must be signed in to change notification settings - Fork 115
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
Action list a11y #1828
Action list a11y #1828
Conversation
🦋 Changeset detectedLatest commit: 7372a32 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
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.
This is awesome 🤩 Just a few suggestions!
previews/primer/alpha/nav_list_preview/trailing_action.html.erb
Outdated
Show resolved
Hide resolved
previews/primer/alpha/nav_list_preview/trailing_action.html.erb
Outdated
Show resolved
Hide resolved
previews/primer/alpha/nav_list_preview/trailing_action.html.erb
Outdated
Show resolved
Hide resolved
previews/primer/alpha/nav_list_preview/trailing_action.html.erb
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.
Looks great @camertron !
Description
This PR addresses accessibility feedback for the
NavList
andActionList
components as identified in https://github.com/github/primer/issues/1120. Namely it:<ul>
s have been removed and separators are now<div>
s instead of<li>
saria-labelledby
to sub-list<ul>
s. If a heading and anaria-label
are both provided, an error is now raised.removes theTheshow_on_hover
attribute fromActionList
andNavList
items, as only showing buttons on hover is not an accessible patternshow_on_hover
option has been left in. We will address it (if necessary) in a future PR.NavList
's trailing action preview so that the buttons actually do somethingNavList
s are wrapped with the<nav>
tagIntegration
Yes. Since the
sections
slot has been renamed togroups
, any place in dotcom where we're callingwith_section
will need to change towith_group
. In addition, we will need to removearia-label
s from any section/group that also contains a header.Merge checklist