-
Notifications
You must be signed in to change notification settings - Fork 33
Feat: Handle accessibility for crossing over thumbs #97
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
base: master
Are you sure you want to change the base?
Feat: Handle accessibility for crossing over thumbs #97
Conversation
src/components/Thumb.tsx
Outdated
| // Accessibility actions | ||
| const accessibilityActions = useEvent((event: RN.AccessibilityActionEvent) => { | ||
| const tenth = (maximumValue - minimumValue) / 10 | ||
| const tenth = Math.round((maximumValue - minimumValue) / 10) |
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 will break sliders relying on small values, like between 0 and 1.
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.
@Sharcoux In some cases, I need to round that value, and I believe others may have the same requirement. Therefore, my approach is to add a new prop function (e.g., formatAccessibilityValue) to format it accordingly. What are your thoughts on this?
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.
If you want to round the value, simply use "step". It was made for that.
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.
In my case, if my step is 1, and my range is large, from 0 to 999. I don't think increasing/decreasing the value by 1 is ideal.
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.
In that case, you can use the method return by useRounding. That method takes into account the min and max and step to decide the best rounding to apply.
src/components/Thumb.tsx
Outdated
| switch (event.nativeEvent.actionName) { | ||
| case 'increment': | ||
| updateValue(value + (step || tenth)) | ||
| updateValue(+(step || tenth)) |
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.
It would be better to make updateValue be a SetStateAction<number>, accepting both a number or a function.
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.
@Sharcoux I don't really understand SetStateAction<number>, and if I change that, I believe it will affect many old functions.
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.
SetStateAction is the type of the function returned by React.useState. For instance:
const [value, setValue] = React.useState(0)
Here, setValue is a SetStateAction that can accept setValue(1) as well as setValue(oldValue => oldValue + 1)
| onPress={onPress} onMove={onMove} onRelease={onRelease} | ||
| onPress={(value) => { | ||
| // We need to blur the min/max thumb if it is focused when the user interacts with the slider | ||
| blurThumbs() |
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 find this suspicious. What would have the focuse, then?
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.
@Sharcoux In the example, if I focus on the min thumb, and then drag this thumb over the max thumb, you will see that the min thumb remains focused while I'm interacting with the max thumb.
Screen.Recording.2024-05-23.at.5.08.06.PM.mov
| > | ||
| <Track color={outboundColor} style={minStyle} length={minTrackPct * 100} vertical={vertical} thickness={trackHeight} /> | ||
| <Thumb key='min' {...thumbProps} updateValue={updateMinValue} value={min} thumb='min' /> | ||
| <Thumb key='min' {...thumbProps} thumbRef={minThumbRef} updateValue={(value) => crossingAllowed ? updateAccessibilityMinValue(value) : updateMinValue(min + value)} value={min} thumb='min' /> |
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 that a better approach would be to uniquely identify each thumb with their "key", and order them based on their current value. This would require accepting the range to be reverted internally: [3, 1] would be a valid range internally/
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.
@Sharcoux If I order them based on their current value with their 'key', how can I focus on the max/min thumbs if I increase/decrease the value beyond them?
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.
If you order them based on their current value, the thumb would not change, so you would not need to handle the focus switch.
Ref: #95
Changes in this PR
Screenshots
Screen.Recording.2024-05-04.at.10.52.22.AM.mov