-
Notifications
You must be signed in to change notification settings - Fork 841
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
[EuiCollapsibleNavBeta] Consider adding aria-hidden="true" to the icon button for screen reader UX #7353 #8236
base: main
Are you sure you want to change the base?
Conversation
💚 CLA has been signed |
👋 Since this is a community submitted pull request, a Buildkite build has not been started automatically. Would an Elastic organization member please verify the contents of this pull request and kick off a build manually? |
@Kukki8 thank you so much for your contribution! I'll review your PR soon 💚 |
packages/eui/src/components/collapsible_nav/__snapshots__/collapsible_nav.test.tsx.snap
Outdated
Show resolved
Hide resolved
@Kukki8 hey, there! 👋🏻 Just wanted to check up on you, do you need any help making the suggested changes? |
@weronikaolejniczak Hello! I'm sorry for the delay, I've implemented the changes suggested 😄 |
@Kukki8 no worries and thank you, all looks good now! 💚 Although the changes are exactly what the ticket says to do, I tested the component accessibility and I have some doubts, I'll get back to you in a day 😄 |
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.
We have tested the changes (you can see the details here), everything looks great! Thank you so much for the amazing contribution 🥳 Feel free to take any other tasks if you want, we greatly appreciate any help!
Summary
This PR provides:
aria-hidden={buttonIsInteractive ? 'true' : 'false'}
tag to theaccordion_trigger.tsx
file, as advised in the issue description.This PR doesn't contain:
closes #7353
Implementation
QA
Remove or strikethrough items that do not apply to your PR.
General checklist
Checked in both light and dark modesChecked in mobileEdge, and FirefoxAdded documentationProps have proper autodocs (using@default
if default values are missing) and playground togglesChecked Code Sandbox works for any docs examplesUpdated visual regression testsIf applicable, added the breaking change issue label (and filled out the breaking change checklist)If applicable, file an issue to update EUI's Figma library with any corresponding UI changes. (This is an internal repo, if you are external to Elastic, ask a maintainer to submit this request)