Skip to content
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

Use more specific rule skipping for a11y tests #77647

Merged
merged 6 commits into from
Sep 29, 2020

Conversation

myasonik
Copy link
Contributor

@myasonik myasonik commented Sep 16, 2020

Summary

Team details

@elastic/kibana-app I removed the role="menu" and role="menuitem" pieces because the roles weren't properly implemented (at least from a glance, missing keyboard controls and incorrect DOM structure. maybe other issues). It's debatable whether or not menu was ever right but it being broken would definitely cause more trouble than any value it could bring. The DOM without it is fine (a list of buttons).

@elastic/kibana-app-arch I'm guessing y'all own the query_string_input which I'm excluding from a specific set test because I'm assuming there's an eventual goal to replace that whole component with EUI. If not, I can create a ticket for fixing it.

@myasonik myasonik force-pushed the a11y-test-nav-search branch 2 times, most recently from 6f9ac3c to 22aaebd Compare September 17, 2020 16:45
@myasonik myasonik changed the title Add a11y test for nav search + add per-rule config Use more specific rule skipping for a11y tests Sep 17, 2020
@myasonik myasonik force-pushed the a11y-test-nav-search branch from 58f539b to e3b6d3f Compare September 22, 2020 20:27
@myasonik
Copy link
Contributor Author

@elasticmachine merge upstream

@myasonik myasonik force-pushed the a11y-test-nav-search branch from ee9bf57 to 167f77d Compare September 23, 2020 20:26
@myasonik myasonik added Project:Accessibility release_note:skip Skip the PR/issue when compiling release notes v7.10.0 v8.0.0 labels Sep 24, 2020
@myasonik myasonik requested a review from bhavyarm September 24, 2020 15:27
@myasonik myasonik marked this pull request as ready for review September 24, 2020 16:07
@myasonik myasonik requested a review from a team September 24, 2020 16:07
@myasonik myasonik requested a review from a team as a code owner September 24, 2020 16:07
@myasonik
Copy link
Contributor Author

@elastic/kibana-app I removed the role="menu" and role="menuitem" pieces because the roles weren't properly implemented (at least from a glance, missing keyboard controls and incorrect DOM structure. maybe other issues). It's debatable whether or not menu was ever right but it being broken would definitely cause more trouble than any value it could bring. The DOM without it is fine (a list of buttons).

@elastic/kibana-app-arch I'm guessing y'all own the query_string_input which I'm excluding from a specific set test because I'm assuming there's an eventual goal to replace that whole component with EUI. If not, I can create a ticket for fixing it.

Copy link
Contributor

@bhavyarm bhavyarm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for these awesome changes @myasonik MOAR TESTS

@myasonik
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

page load bundle size

id value diff baseline
data 1.3MB +41.0B 1.3MB
globalSearchBar 28.0KB +39.0B 27.9KB
visualizations 272.5KB -28.0B 272.5KB
total +52.0B

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

Copy link
Contributor

@stratoula stratoula left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kibana app team code review only, LGTM!

@myasonik myasonik merged commit 7b07fa5 into elastic:master Sep 29, 2020
@myasonik myasonik deleted the a11y-test-nav-search branch September 29, 2020 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Project:Accessibility release_note:skip Skip the PR/issue when compiling release notes v7.10.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants