-
Notifications
You must be signed in to change notification settings - Fork 646
fix(LabelGroup): add role and aria-label to hidden items overlay #7241
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
Conversation
🦋 Changeset detectedLatest commit: 4baf51d 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 |
|
👋 Hi, this pull request contains changes to the source code that github/github-ui depends on. If you are GitHub staff, test these changes with github/github-ui using the integration workflow. Or, apply the |
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.
Pull request overview
This pull request adds ARIA attributes to the LabelGroup component's overlay to improve screen reader accessibility when hidden labels are displayed. The change addresses an accessibility issue by adding role="dialog" and a dynamic aria-label to the overlay that appears when users click the "+X" button to view hidden labels.
Key Changes:
- Added
role="dialog"andaria-labelto the overlay via theoverlayPropsproperty in the OverlayToggle component
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/react/src/LabelGroup/LabelGroup.tsx | Added overlayProps with role="dialog" and dynamic aria-label to improve accessibility of hidden items overlay |
| .changeset/four-carrots-divide.md | Added changeset documenting this patch-level fix |
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
| </Button> | ||
| )} | ||
| focusZoneSettings={{disabled: true}} | ||
| overlayProps={{role: 'dialog', 'aria-label': `All ${hiddenItemIds.length} labels`}} |
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.
I noticed on the example (https://primer-eb2abbd43f-13348165.drafts.github.io/storybook/?path=/story/components-labelgroup-features--truncate-auto-with-interactive-tokens) it'll say "all 4 labels" but there are more than 4 labels in the dialog 🤔
Maybe we go with something more generic like "Additional labels"
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.
oops, thanks! I corrected it to the totalLength instead since it's actually showing all of them. I feel like "additional labels" is misleading as well
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
👋 Hi from github/github-ui! Your integration PR is ready: https://github.com/github/github-ui/pull/7850 |
|
🔴 ci completed with status |
|
Seeing same integration errors on main branch, merging |
Closes https://github.com/github/primer/issues/5207
This pull request makes a small accessibility improvement to the
LabelGroupcomponent. The change adds proper ARIA attributes to the overlay for better screen reader support.Changelog
New
role="dialog"and a dynamicaria-labelto the overlay via theoverlayPropsproperty, improving screen reader support for overlays displaying hidden labels.Rollout strategy
Testing & Reviewing
Merge checklist