-
Notifications
You must be signed in to change notification settings - Fork 8.1k
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 vis type selection accessibility #29498
Conversation
Pinging @elastic/kibana-app |
💚 Build Succeeded |
@@ -104,6 +104,19 @@ describe('NewVisModal', () => { | |||
}); | |||
}); | |||
|
|||
describe('filter for visualization types', () => { | |||
it('should render as expected', () => { |
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.
Perhaps could we update the description slightly so that if one does not know the expectation, they will have a better understanding of what the expectation is? Perhaps something like "should render filtered results with meaningful accessibility descriptions"?
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 think if we rename this, we would want to make that test more specific. Currently we use should render as expected
for most tests that just compare a snapshot. Also since usually if you update the snapshot you might not actually look exactly at the accessible description in there. So I would prefer leaving the name like that or writing a more specific test, that actually finds the description in there and check for it.
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.
Got it, thanks for explaining the description convention! Good to know :)
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.
LGTM tested it locally with voiceover. Thanks @timroes
Summary
Improves accessibility of vis type in two points:
aria-live
region to properly announce to assistive technologies how many results have been found while filtering for visualizations.Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.[ ] This was checked for cross-browser compatibility, including a check against IE11[ ] Documentation was added for features that require explanation or tutorialsFor maintainers
[ ] This was checked for breaking API changes and was labeled appropriately[ ] This includes a feature addition or change that requires a release note and was labeled appropriately