Luna 1326: Changes BpkSlider base library for mobile accessibility
Base library updated to radux-ui/react-slider which allows for screen reader swipe interaction on mWeb.
All props of previous react-slider library have been changed to ensure 99% backwards compatibility however ariaLabel is now a required prop
The Problem we had - swiping does not move thumb
https://github.com/Skyscanner/backpack/assets/28352715/bc461f79-5f5a-4cf8-8de6-2dfa8e10f9af
With new base library - note this is using the dist build from this PR in banana
https://github.com/Skyscanner/backpack/assets/28352715/5e46ba44-16bd-427f-bb1f-8d0277fffa80
Breaking change
-
ariaLabelis now a required prop -
classNameprop removed
Remember to include the following changes:
- [x] Ensure the PR title includes the name of the component you are changing so it's clear in the release notes for consumers of the changes in the version e.g
[KOA-123][BpkButton] Updating the colour - [ ]
README.md(If you have created a new component) - [ ] Component
README.md - [x] Tests
- [x] Storybook examples created/updated
- [ ] Type declaration (
.d.ts) files updated - [ ] For breaking changes or deprecating components/properties, migration guides added to the description of the PR. If the guide has large changes, consider creating a new Markdown page inside the component's docs folder and link it here
Browser support
If this is a visual change, make sure you've tested it in multiple browsers.
Generated by :no_entry_sign: dangerJS against 7ee61056effa1333d5cb9f7c593ce39864d0244b
Visit https://backpack.github.io/storybook-prs/3368 to see this build running in a browser.
Visit https://backpack.github.io/storybook-prs/3368 to see this build running in a browser.
Visit https://backpack.github.io/storybook-prs/3368 to see this build running in a browser.
Visit https://backpack.github.io/storybook-prs/3368 to see this build running in a browser.
Thanks @LouiseReid for the PR this is interesting this issue came up as we recently completed an audit for this component based on the old library and there were no screenreader issues coming. So this PR comes as a shock as its not working when it was passed in audits
Thanks @LouiseReid for the PR this is interesting this issue came up as we recently completed an audit for this component based on the old library and there were no screenreader issues coming. So this PR comes as a shock as its not working when it was passed in audits
Its strange as the keyboard arrow keys for navigation work on desktop which is what I believe should also power the mobile screen reader swipe but they don't
Thanks @LouiseReid for the PR this is interesting this issue came up as we recently completed an audit for this component based on the old library and there were no screenreader issues coming. So this PR comes as a shock as its not working when it was passed in audits
Its strange as the keyboard arrow keys for navigation work on desktop which is what I believe should also power the mobile screen reader swipe but they don't
Gotcha I see, now trying again on some different devices!
Could you also check the storybook for this? https://backpack.github.io/storybook-prs/3368/?path=/story/bpk-component-slider--visual-test
I just tried on Chrome on Android and its not working as expected, its the same behaviour of not slidiing but this time no highlighting of the slider controls
Visit https://backpack.github.io/storybook-prs/3368 to see this build running in a browser.
Will pause this just now. I've raised an issue on the source repo - https://github.com/radix-ui/primitives/issues/2850
Will give that a couple of weeks then we can decided next steps - nothing, finding another lib or merging as is to at least fix for iOS
@olliecurtis I've hit a bit of a wall with trying to get this working with Android talkback. I propose we merge as is at the next appropriate time given it enhances iOS and does nothing detrimental to android
Visit https://backpack.github.io/storybook-prs/3368 to see this build running in a browser.
Visit https://backpack.github.io/storybook-prs/3368 to see this build running in a browser.
Adding DON'T MERGE label so we don't merge until the point of next major and batch together
Also to check do we need to have a migration guide
No, its been make backward compatible matching the api of the old library with the only breaking change being the need to now send an array of aria labels (previously was also array but not required).
We could also make that an optional prop as to not be a breaking change but really aria labels and aria value text values should really be given for a11y
Visit https://backpack.github.io/storybook-prs/3368 to see this build running in a browser.
Visit https://backpack.github.io/storybook-prs/3368 to see this build running in a browser.