Repository navigation
feat: add direction to I18nProvider as an override of the locale - #10537
dylanpulver wants to merge 4 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
|
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! noting that we're reversing the decision in Ability to manually set direction to RTL |
Replaces the Slider `direction` prop with the approach asked for in review: `I18nProvider` takes an optional `direction` that overrides the direction implied by the locale, and a control whose direction comes from its content rather than from the reading order is wrapped in one. That works for every direction-aware component instead of only the slider, and it nests, so a part of a subtree can be put back to the locale direction. The locale itself is untouched by the override, so formatters and translated strings inside an ltr-pinned Arabic subtree are still Arabic. `SliderFill` now anchors with a physical `left`/`right` taken from `useLocale()` rather than with `insetInlineStart`. The thumb has always been positioned with a physical `left` against that same direction, while `insetInlineStart` resolves against the document, so the two disagreed as soon as a provider overrode the direction and the fill and the thumb landed on opposite edges. With no override the resolved edge is unchanged, only the property name, which is why the three existing SliderFill assertions move from `insetInlineStart` to `left`.
|
Made the change. One thing the approach needs, and it is a decision for you. That hazard applies to any component that styles itself with logical properties inside an overridden subtree, so the story and the prop doc both say to set Tested in
Not tested: screen readers. Nor high contrast or zoom levels. From this machine: 235 passing across the 11 slider and i18n suites, 3,289 passing across the three core packages. |
Intent
Some controls take their direction from their content rather than from the reading order of the UI. A media playback bar stays left to right in an RTL UI. A manga page position bar runs right to left in an LTR UI. Today the only direction available is the locale's, so these are mirrored the wrong way with no way to opt out.
This PR started as a
directionprop on Slider. Per review (#10537, and this comment) it is now an override onI18nProvider, which covers every direction-aware component instead of one, and which nests, so part of a subtree can be put back to the locale direction.Closes the slider half of #10112. Reverses the decision in #6469.
What changed
I18nProvidertakes an optionaldirection. When set it replaces the direction implied by the locale. The locale itself is untouched, so formatters and translated strings inside anltr-pinned Arabic subtree are still Arabic.SliderFillanchors with a physicalleftorrightfromuseLocale()rather than withinsetInlineStart. The thumb has always been positioned with a physicalleftagainst that same direction, whileinsetInlineStartresolves against the document, so the two disagreed under an override and the fill and the thumb landed on opposite edges. With no override the resolved edge is unchanged and only the property name differs, which is why three existingSliderFillassertions move frominsetInlineStarttoleft.Because
I18nProviderrenders no DOM, an override does not change what CSS logical properties resolve against. The prop doc and the new story both setdiron an element inside the provider, the wayProviderin@react-spectrum/s2and theI18ndocs example do.Tested
In
en-USandar-AE, with and without an override, horizontal and vertical:Not tested: screen readers. Nor high contrast or zoom levels.
235 passing across the 11 slider and i18n suites, 3,289 passing across the three core packages.
oxlintandoxfmt --checkclean.tsc --noEmitreports only the three pre-existing@mdx-js/reactduplicate-JSX errors fromnode_modules. Reverting the override inI18nProviderfails 13 of these tests.Written with AI assistance (Claude Opus 5). The numbers above are from runs on this machine.