-
Notifications
You must be signed in to change notification settings - Fork 529
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
fix(accessibility): CR-4249 instantearch accessibility fixes #5884
Conversation
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 742b35a:
|
…CR-4249/accessibility-fixes
…lia/instantsearch into CR-4249/accessibility-fixes
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.
🪨 thanks for your contribution! I have a couple questions:
packages/instantsearch.js/src/components/Pagination/Pagination.tsx
Outdated
Show resolved
Hide resolved
packages/instantsearch.js/src/components/SearchBox/SearchBox.tsx
Outdated
Show resolved
Hide resolved
packages/instantsearch.js/src/components/SearchBox/SearchBox.tsx
Outdated
Show resolved
Hide resolved
packages/react-instantsearch/src/ui/__tests__/Pagination.test.tsx
Outdated
Show resolved
Hide resolved
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.
Current changes look good to me, I'll let you handle Haroen's remark and you can run yarn jest -u
to update the test snapshots, before we can move on with this contribution.
packages/instantsearch.js/src/components/Pagination/Pagination.tsx
Outdated
Show resolved
Hide resolved
@paynerichards it seems there are unresolved snapshot mismatchs, probably in /tests/common/widgets/pagination/option.ts. Is it solved when rerunning a snapshot update command with Jest? |
I believe the tests are still failing because Vue InstantSearch pagination might not be updated? |
Yes, the test and behaviour is common for all flavours, so the change should be done everywhere, thanks! If you can also do it in /specs, that would be ideal. Sorry for missing that in earlier reviews |
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.
👏
Summary
This PR addresses accessibility concerns brought up in CR-429 by adding ARIA labels to slider handles and sortBy menu, and expanding on pagination labels
Also: CR-1201 adding aria-label to SearchBox
Result
Targeted areas have accessible labels