feat: add direction to Slider for tracks that don't follow the locale - #10537
dylanpulver wants to merge 2 commits into
Conversation
|
Thanks for the PR! You'll need to sign the CLA and then close/re-open the PR to trigger a re-run |
|
Thanks for the PR. I had a chat with Adobe's i18n and linguistics, the direction of the slider isn't restricted to only being LTR in an RTL UI. There are cases where it will need to be RTL in an LTR ui.
Instead of a boolean, I'm leaning more towards something like |
A horizontal slider mirrors with the UI locale, which is right when the track represents reading order. It is wrong when the direction comes from the content instead: a media playback bar stays left to right in an RTL UI, and a track over right-to-left content such as a manga page stays right to left in an LTR UI. `direction?: 'ltr' | 'rtl'` pins the track. Omitted, the slider mirrors the locale exactly as before. Direction is read in four places and they mirror by two mechanisms. `useSlider` (track click, track drag) and `useSliderThumb` (arrow keys, drag, thumb `left`) invert in JS and now resolve `state.direction ?? localeDirection`, so both pinned directions fall out of the existing `direction === 'rtl'` checks. `SliderFill` mirrors in CSS via `insetInlineStart`, which resolves against the document and is therefore the wrong edge for a pinned track; it now anchors physically, `left` for a pinned ltr track and `right` for a pinned rtl one, and keeps `insetInlineStart` untouched when nothing is pinned. Vertical is unaffected. The prop lives on `SliderState` rather than being threaded through both hooks, following `orientation`, which means S2 picks it up through `SliderBaseProps`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011M5uTyCU4WcNTsPvGrErDo
pointerDown installs mouseup/touchend/pointerup listeners on window. The two stacked-thumb tests never released the pointer, so on React 16 and 17 the listeners outlived the unmounted component and fired during a later test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011M5uTyCU4WcNTsPvGrErDo
3db72f5 to
c856427
Compare
dylanpulver
changed the title
|
Thanks for the updates, we had another round of discussions today. Since this may be necessary for other components, we want to go in the direction this comment suggested #10112 (comment) with the expectation that users would wrap their timeline sliders with an I18nProvider that overrides the direction.
If there are things inside the slider which should not have the direction changed (beyond just text since that already exists nesting within a If you don't have time to make the change, let us know. Thanks again! |
3 participants
Closes #10112
direction?: 'ltr' | 'rtl'as you suggested. A boolean could only pin to the UI's opposite, so it can't express an RTL track in an LTR UI — the manga case.Omitted, behaviour is exactly as before. Set, the hooks resolve
state.direction ?? localeDirectionandSliderFillanchors physically; vertical is unaffected.✅ Pull Request Checklist:
📝 Test Instructions:
Storybook: Slider Pinned Direction.
26 tests, each proven live by a mutant; the boolean-shaped fill mutant fails exactly the
direction="rtl"one. Upstream's 49 slider tests, unchanged, give identical results.yarn jest slider Slider233/233;yarn lintclean.🧢 Your Project:
Personal open-source contribution.