-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Improve the accessibility of the Discover screen #12681
Conversation
aria-expanded is a true/false/undefined state so we need to make sure even if showFilter is undefined, we will print false (that's why the double negation) aria-haspopup doesn't really fit, since this is not a popup in the sense the ARIA standard describes ("A popup is generally presented visually as a group of items that appears to be on top of the main page content.") Add aria-controls to reference the controlled element. Fix elastic#12638 and toggle label when filter toggles
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.
+1 from me. Great work!
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.
Awesome!!! 👏 I only have one really nit-picky little request.
> | ||
<span aria-hidden="true" class="kuiIcon fa-gear"></span> | ||
</button> | ||
</h3> | ||
</div> | ||
|
||
<div class="sidebar-item discover-field-details" ng-show="showFilter"> | ||
<div class="sidebar-item discover-field-details" ng-show="showFilter" id="discover-field-filter"> |
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.
Really minor nit, but per our HTML style guide, can we write IDs in camel case, for both this ID and the other ones we're adding?
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.
Absolutely not nit-picking, but me not reading the style-guide carefully enough. Thanks for pointing this out. Fixed it. The file itself (and I assume several others) contain quite some more ids not using camel-case. But I think, that needs to be addressed in a different 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.
LGTM
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!!
This PR fixes several issues of the discovery app. Fixes #12635, #12642, #12636, #12638
Besides I added some addition a11y (
aria-expanded
) to the collapsible side bar, add anaria-describedby
from the search input to the "lucene query syntax link", add somearia-controls
where needed.