-
Notifications
You must be signed in to change notification settings - Fork 185
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
Luna 1326: Changes BpkSlider base library for mobile accessibility #3368
Conversation
ariaValuetext={[getSliderTime(finalSliderStart), getSliderTime(finalSliderEnd)]} | ||
|
||
`} | ||
|
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.
Visit https://backpack.github.io/storybook-prs/3368 to see this build running in a browser. |
it('should render correctly with a "className" attribute', () => { | ||
const { asFragment } = render( | ||
<BpkSlider min={0} max={100} value={25} className="my-slider" />, | ||
); |
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.
className prop removed
@@ -1,3 +1,4 @@ | |||
/* eslint-disable @skyscanner/rules/forbid-component-props */ |
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.
This is needed here as a way to set styles on the external component
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.
Here we can add an exclusion to our linter so its centralised in one place, see here for examples: https://github.com/Skyscanner/backpack/blob/main/.eslintrc#L139
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. |
const processSliderValues = (sliderValues: number[], callback?: (val: number | number[]) => void) => { | ||
const val = sliderValues.length === 1 ? sliderValues[0] : sliderValues; | ||
if (callback) { | ||
callback(val); | ||
} | ||
}; | ||
|
||
const handleOnChange = (sliderValues: number[]) => { | ||
processSliderValues(sliderValues, onChange); | ||
}; | ||
|
||
const isRange = (rest.value || rest.defaultValue || []).length > 1; | ||
const handleOnAfterChange = (sliderValues: number[]) => { | ||
processSliderValues(sliderValues, onAfterChange); | ||
}; |
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.
This looks a bit wild but its to handle differences between the base libraries. This new library always returns the value in an array, so if there is just one thumb it is passed back like [value]
where as for a range its the expected [value, value]
The previous library returned a one thumb slider value back as just a number rather than a number in an array
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.
Out of curiosity how does the [value, value] get added to the array?
Is the value at index 0 always one of the sliders, and index 1 is at the other?
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.
Yea, that is from the internal workings of the library. Somewhere in here, its state is an array
https://github.com/radix-ui/primitives/blob/main/packages/react/slider/src/Slider.tsx#L99
Visit https://backpack.github.io/storybook-prs/3368 to see this build running in a browser. |
|
||
return ( | ||
<Slider | ||
<Slider.Root | ||
className={getClassName('bpk-slider')} |
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.
Why is it that we remove className throughout, when we still need it here for the root styling?
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.
We still need to give the slider skyscanner the styles but removing the className prop from BpkSlider prevents the likes of Banana adding their own styles on top of that
@@ -30,44 +30,52 @@ window.ResizeObserver = | |||
})); | |||
|
|||
describe('BpkSlider accessibility tests', () => { | |||
/* | |||
Currently this test fails because the third-party library |
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.
So nice to be able to delete this 🔥
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 |
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.
Just adding a merge prevention here whilst we are figuring out some A11y weirdness with Android
aria-label={ariaLabel[index]} | ||
aria-valuetext={ariaValuetext ? ariaValuetext[index] : val.toString()} | ||
className={getClassName('bpk-slider__thumb')} | ||
aria-valuenow={currentValue[index]} |
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 this might fix the android issue. Without it the valueText remained static when the slider moved but with it it changes with the slider value
Before
Screen.Recording.2024-04-18.at.11.30.37.mov
Now
Screen.Recording.2024-04-18.at.11.31.01.mov
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 - radix-ui/primitives#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. |
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.
One small comment see below. Also to check do we need to have a migration guide for folks to adopt the new version of the Slider to help manage this change?
@@ -1,3 +1,4 @@ | |||
/* eslint-disable @skyscanner/rules/forbid-component-props */ |
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.
Here we can add an exclusion to our linter so its centralised in one place, see here for examples: https://github.com/Skyscanner/backpack/blob/main/.eslintrc#L139
Adding DON'T MERGE label so we don't merge until the point of next major and batch together |
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. |
…essibility (#3368) [Luna 1326][BpkSlider]: Changes BpkSlider base library for mobile accessibility (#3368) * styling aligned * remove react-slider, install radix slider * initial props setup * aria value text update * remove class name from the example * update tests * update readme and stories` * ts ds * make step optional * handle range values being returned as an array * stick height in * remove debug * adds value now aria value * update linting for classname * Fixing package files for mistaken merge
…essibility (#3368) [Luna 1326][BpkSlider]: Changes BpkSlider base library for mobile accessibility (#3368) * styling aligned * remove react-slider, install radix slider * initial props setup * aria value text update * remove class name from the example * update tests * update readme and stories` * ts ds * make step optional * handle range values being returned as an array * stick height in * remove debug * adds value now aria value * update linting for classname * Fixing package files for mistaken merge
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 howeverariaLabel
is now a required propThe Problem we had - swiping does not move thumb
RPReplay_Final1713258395.MP4
With new base library - note this is using the dist build from this PR in banana
RPReplay_Final1713348804.mov
Breaking change
ariaLabel
is now a required propclassName
prop removedRemember to include the following changes:
[KOA-123][BpkButton] Updating the colour
README.md
(If you have created a new component)README.md
.d.ts
) files updated