Skip to content

Range slider#212

Open
72haja wants to merge 2 commits intomainfrom
range-slider
Open

Range slider#212
72haja wants to merge 2 commits intomainfrom
range-slider

Conversation

@72haja
Copy link
Contributor

@72haja 72haja commented Jan 21, 2026

No description provided.

@72haja 72haja self-assigned this Jan 21, 2026
@72haja 72haja requested a review from jesko-plitt as a code owner January 21, 2026 08:40
@72haja 72haja added the UI this PR has UI related changes label Jan 21, 2026
@changeset-bot
Copy link

changeset-bot bot commented Jan 21, 2026

⚠️ No Changeset found

Latest commit: 869238b

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@vercel
Copy link

vercel bot commented Jan 21, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Review Updated (UTC)
becklyn-components Ready Ready Preview, Comment Jan 21, 2026 8:41am

Comment on lines +86 to +89
<Slider.Thumb
onPointerUp={() => {
onValueCommit?.(currentValue);
}}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The onValueCommit callback receives a stale value instead of the current slider value because it's called in the same event cycle as the state update, before React re-renders.

View Details
📝 Patch Details
diff --git a/packages/components/components/atoms/RangeSlider/RangeSlider.stories.tsx b/packages/components/components/atoms/RangeSlider/RangeSlider.stories.tsx
index f6abb41..446d2f4 100644
--- a/packages/components/components/atoms/RangeSlider/RangeSlider.stories.tsx
+++ b/packages/components/components/atoms/RangeSlider/RangeSlider.stories.tsx
@@ -49,6 +49,30 @@ const InteractiveUncontrolledStory = (args: RangeSliderProps) => {
     );
 };
 
+const ControlledWithValueCommitStory = (args: RangeSliderProps) => {
+    const [value, setValue] = useState<number>(args.value || 0);
+    const [lastCommittedValue, setLastCommittedValue] = useState<number | null>(null);
+
+    return (
+        <div className="story-container mobileDesktop">
+            <RangeSlider
+                {...args}
+                value={value}
+                setValue={setValue}
+                defaultValue={undefined}
+                name={undefined}
+                onValueCommit={finalValue => {
+                    setLastCommittedValue(finalValue);
+                }}
+            />
+            <p>Current Value: {value}</p>
+            <p>
+                Last Committed Value: {lastCommittedValue !== null ? lastCommittedValue : "(none)"}
+            </p>
+        </div>
+    );
+};
+
 const meta: Meta<typeof RangeSlider> = {
     title: "Atoms/RangeSlider",
     component: RangeSlider,
@@ -84,3 +108,15 @@ export const Uncontrolled: Story = {
     },
     render: args => <InteractiveUncontrolledStory {...args} />,
 };
+
+export const WithValueCommit: Story = {
+    args: {
+        label: "Range Slider (with onValueCommit)",
+        min: 0,
+        max: 100,
+        step: 1,
+        value: 0,
+        setValue: () => {},
+    },
+    render: args => <ControlledWithValueCommitStory {...args} />,
+};
diff --git a/packages/components/components/atoms/RangeSlider/RangeSlider.tsx b/packages/components/components/atoms/RangeSlider/RangeSlider.tsx
index aa1c5a5..9fc868b 100644
--- a/packages/components/components/atoms/RangeSlider/RangeSlider.tsx
+++ b/packages/components/components/atoms/RangeSlider/RangeSlider.tsx
@@ -78,18 +78,18 @@ export const RangeSlider: FC<RangeSliderProps> = ({
                 step={step}
                 value={[currentValue]}
                 onValueChange={([newValue]) => handleValueChange(newValue)}
+                onValueCommit={values => {
+                    const finalValue = values[0];
+                    if (finalValue !== undefined) {
+                        onValueCommit?.(finalValue);
+                    }
+                }}
                 disabled={disabled}
                 name={name}>
                 <Slider.Track className={styles.sliderTrack}>
                     <Slider.Range className={styles.sliderRange} />
                 </Slider.Track>
-                <Slider.Thumb
-                    onPointerUp={() => {
-                        onValueCommit?.(currentValue);
-                    }}
-                    className={styles.sliderThumb}
-                    aria-label="Minimum"
-                />
+                <Slider.Thumb className={styles.sliderThumb} aria-label="Minimum" />
             </Slider.Root>
         </div>
     );

Analysis

RangeSlider onValueCommit callback never fires due to incorrect event handler placement

What fails: The onValueCommit callback passed to RangeSlider component is never invoked when user releases the slider. The component attempted to call it from Slider.Thumb's onPointerUp handler, but due to Radix UI Slider's pointer capture implementation, this handler never fires.

How to reproduce:

 
/>

Drag the slider and release. The console.log never fires - onValueCommit is not called.

Root cause:

Radix UI Slider's SliderImpl (the root element) calls setPointerCapture() on pointer down, causing all subsequent pointer events to be retargeted to the capturing element. Because pointer capture is active on the root, the onPointerUp handler attached to Slider.Thumb (a descendant) never fires - the event is retargeted to the root.

Fix:

Pass onValueCommit directly to Slider.Root component instead of attempting to call it from Slider.Thumb's onPointerUp handler. Radix UI Slider provides a built-in onValueCommit prop that fires correctly when slider interaction ends:

 
>
    ...
</Slider.Root>

Result: onValueCommit callback now fires correctly when user releases the slider, receiving the final committed value.

References:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-review UI this PR has UI related changes

Development

Successfully merging this pull request may close these issues.

1 participant