-
Notifications
You must be signed in to change notification settings - Fork 2.9k
feat(react-range-slider-preview): scaffold package and add spec #35440
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: react-range-slider
Are you sure you want to change the base?
feat(react-range-slider-preview): scaffold package and add spec #35440
Conversation
📊 Bundle size report✅ No changes found |
|
Pull request demo site: URL |
packages/react-components/react-range-slider-preview/library/docs/Spec.md
Outdated
Show resolved
Hide resolved
packages/react-components/react-range-slider-preview/library/docs/Spec.md
Outdated
Show resolved
Hide resolved
packages/react-components/react-range-slider-preview/library/docs/Spec.md
Outdated
Show resolved
Hide resolved
packages/react-components/react-range-slider-preview/library/docs/Spec.md
Outdated
Show resolved
Hide resolved
packages/react-components/react-range-slider-preview/library/docs/Spec.md
Outdated
Show resolved
Hide resolved
|
We also discussed this within the team and agreed that it would be better to add this to existing Slider component. So you better will be to create a new feature branch and change this PR to target that branch instead of master, locally reset this commit changes except the spec file, then run UPD: I forgot that |
9301b55 to
4065fac
Compare
4065fac to
9301b55
Compare
9301b55 to
30ad97e
Compare
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 thing I'd suggest to revisit is the accessibility shape of the DOM. In the proposed markup, the input type="range" elements are aria-hidden and tabindex="-1", and the thumbs are exposed as div role="slider". Let's check this approach
| ### Slider Open UI Research | ||
|
|
||
| - <https://open-ui.org/components/slider.research.parts> | ||
| - <https://open-ui.org/components/slider.research> |
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's common for both slider components, let's use separator and add it on the bottom of the document after both slider sections
| ### Slider Open UI Research | |
| - <https://open-ui.org/components/slider.research.parts> | |
| - <https://open-ui.org/components/slider.research> |
| - `pointermove` (or touch move) continues dragging the active thumb. | ||
| - Anti-crossing ensures thumbs never swap order. Collision is handled by clamping the active thumb to the other thumb's position. | ||
|
|
||
| #### Screen readers |
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.
maybe we should also mention here that when vertical is true we expose also aria-orientation:
| | `startInput` | none | Hidden (`aria-hidden="true"`, `tabindex="-1"`). Mirrors the start value for forms. | | ||
| | `endInput` | none | Hidden (`aria-hidden="true"`, `tabindex="-1"`). Mirrors the end value. | |
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.
| | `startInput` | none | Hidden (`aria-hidden="true"`, `tabindex="-1"`). Mirrors the start value for forms. | | |
| | `endInput` | none | Hidden (`aria-hidden="true"`, `tabindex="-1"`). Mirrors the end value. | | |
| | `startInput` | none | Hidden (`aria-hidden="true"`, `tabindex="-1"`). Mirrors the start value for forms. | | |
| | `endInput` | none | Hidden (`aria-hidden="true"`, `tabindex="-1"`). Mirrors the end value for forms. | |
| <input type="range" aria-hidden="true" tabindex="-1" value="20" /> | ||
| <input type="range" aria-hidden="true" tabindex="-1" value="80" /> |
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.
is no longer a control, hidden from AT, not focusable and visible
| <input type="range" aria-hidden="true" tabindex="-1" value="20" /> | |
| <input type="range" aria-hidden="true" tabindex="-1" value="80" /> | |
| <input type="hidden" tabindex="-1" value="20" /> | |
| <input type="hidden" tabindex="-1" value="80" /> |
| class="fui-RangeSlider__startThumb" | ||
| role="slider" | ||
| tabindex="0" | ||
| aria-labelledby="label-123" |
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 guess this DOM output should include visible label, otherwise instead of aria-labelledby here should be aria-label
| | `startInput` | `input` | Hidden `<input type="range">` mirroring the start value for form submissions and pointer/touch selection. | | ||
| | `endInput` | `input` | Hidden `<input type="range">` mirroring the end value. Receives most native props passed to `<RangeSlider>`. | |
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.
| | `startInput` | `input` | Hidden `<input type="range">` mirroring the start value for form submissions and pointer/touch selection. | | |
| | `endInput` | `input` | Hidden `<input type="range">` mirroring the end value. Receives most native props passed to `<RangeSlider>`. | | |
| | `startInput` | `input` | Hidden input mirroring the start value for form submissions and pointer/touch selection. | | |
| | `endInput` | `input` | Hidden input mirroring the end value. Receives most native props passed to `<RangeSlider>`. | |
|
|
||
| ## Slider Component | ||
|
|
||
| ### Slider Variants |
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 you do not need to explicitly have Slider / RangeSlider for each heading. Second level headings for Slider and RangeSlider sections should be enough
| aria-valuemin="20" | ||
| aria-valuemax="100" | ||
| aria-valuenow="80" | ||
| aria-valuetext="80" |
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.
Have you considered an alternative approach where the native elements remain the actual interactive controls, but are visually hidden using SR only styles, with the thumbs being purely visual?
Instead of using aria-hidden on real inputs and recreating slider behavior with div role="slider", the real inputs stay accessible and focusable, and only their visuals are replaced.
.fui-RangeSlider__srOnlyInput {
clip: rect(0 0 0 0);
clip-path: inset(50%);
width: 1px;
height: 1px;
overflow: hidden;
position: absolute;
border: 0;
padding: 0;
margin: 0;
}<div class="fui-RangeSlider">
<div className="fui-RangeSlider__rail" />
<div className="fui-RangeSlider__track" />
<div class="fui-RangeSlider__startThumb">
<!-- visual thumb handle goes here (e.g. a div or pseudo-element) -->
<input
class="fui-RangeSlider__srOnlyInput"
type="range"
min="0"
max="100"
step="1"
value="20"
aria-labelledby="label-1"
aria-valuetext="20"
/>
</div>
<div class="fui-RangeSlider__endThumb">
<input
class="fui-RangeSlider__srOnlyInput"
type="range"
min="0"
max="100"
step="1"
value="80"
aria-labelledby="label-2"
aria-valuetext="80"
/>
</div>
</div>the native inputs remain the source of truth for accessibility and keyboard interaction, while the custom thumbs and track are purely presentational
PR Summary