-
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
adding more pages to a11y testing #4435
Conversation
Preview documentation changes for this PR: https://eui.elastic.co/pr_4435/ |
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.
Couple small items
@@ -153,7 +152,7 @@ exports[`EuiTreeView is rendered 1`] = ` | |||
</span> | |||
</button> | |||
<div | |||
id="euiNestedTreeView-euiTreeView_generated-id" | |||
id="euiNestedTreeView-euiTreeView_generated-id--euiTreeView_generated-id" |
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'm not sure this outcome is what you were expecting? That's a really long id
that seems to duplicate the same string.
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.
Yup, this is the expected outcome in the test.
It's so long because the component setup this pattern of a semi-understandable id
throughout its use. So by knowing just the parent id
, you can construct a child id
(to some degree).
I could make the id
shorter and the code simpler if we got rid of this and just used purely random id
s but I'm not sure if that'll break something for someone at this point so I kept with the same system as this component had before (even though I had to change it slightly because there were duplicate id
issues).
Preview documentation changes for this PR: https://eui.elastic.co/pr_4435/ |
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 good from my side, but I'll want @chandlerprall to look at the lifecycle 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.
Changes LGTM! Verified intended changes are reflected by snapshots
Preview documentation changes for this PR: https://eui.elastic.co/pr_4435/ |
Summary
Enable 5 more docs pages for automatic a11y testing: Button, TreeView, SideNav, CSS Utility Classes, Focus Traps
Update axe-core (one minor, 2 patches), axe-puppeteer (one minor), puppeteer (one minor, one patch)
Checklist
[ ] Check against all themes for compatibility in both light and dark modes[ ] Checked in mobile[ ] Checked in Chrome, Safari, Edge, and Firefox[ ] Props have proper autodocs[ ] Added documentation[ ] Checked Code Sandbox works for the any docs examples