Skip to content
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

[Slider] Ensure onValueCommitted is called with the same value as latest onValueChange #1296

Merged
merged 4 commits into from
Jan 7, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view

This file was deleted.

56 changes: 0 additions & 56 deletions docs/src/app/(private)/experiments/slider-format.tsx

This file was deleted.

99 changes: 0 additions & 99 deletions docs/src/app/(private)/experiments/slider.module.css

This file was deleted.

1 change: 1 addition & 0 deletions packages/react/src/slider/control/SliderControl.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ const testRootContext: SliderRootContext = {
}),
setValue: NOOP,
largeStep: 10,
lastChangedValueRef: { current: null },
thumbMap: new Map(),
max: 100,
min: 0,
Expand Down
8 changes: 5 additions & 3 deletions packages/react/src/slider/control/SliderControl.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -24,14 +24,15 @@ const SliderControl = React.forwardRef(function SliderControl(
disabled,
dragging,
getFingerState,
setValue,
lastChangedValueRef,
minStepsBetweenValues,
onValueCommitted,
state,
percentageValues,
registerSliderControl,
setActive,
setDragging,
setValue,
state,
step,
thumbRefs,
} = useSliderRootContext();
Expand All @@ -40,14 +41,15 @@ const SliderControl = React.forwardRef(function SliderControl(
disabled,
dragging,
getFingerState,
setValue,
lastChangedValueRef,
minStepsBetweenValues,
onValueCommitted,
percentageValues,
registerSliderControl,
rootRef: forwardedRef,
setActive,
setDragging,
setValue,
step,
thumbRefs,
});
Expand Down
12 changes: 7 additions & 5 deletions packages/react/src/slider/control/useSliderControl.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,14 +49,15 @@ export function useSliderControl(
disabled,
dragging,
getFingerState,
setValue,
onValueCommitted,
lastChangedValueRef,
minStepsBetweenValues,
onValueCommitted,
percentageValues,
registerSliderControl,
rootRef: externalRef,
setActive,
setDragging,
setValue,
step,
thumbRefs,
} = parameters;
Expand Down Expand Up @@ -126,9 +127,9 @@ export function useSliderControl(

setActive(-1);

commitValidation(finger.value);
commitValidation(lastChangedValueRef.current ?? finger.value);

onValueCommitted(finger.value, nativeEvent);
onValueCommitted(lastChangedValueRef.current ?? finger.value, nativeEvent);

touchIdRef.current = null;

Expand Down Expand Up @@ -277,13 +278,14 @@ export namespace useSliderControl {
| 'disabled'
| 'dragging'
| 'getFingerState'
| 'setValue'
| 'lastChangedValueRef'
| 'minStepsBetweenValues'
| 'onValueCommitted'
| 'percentageValues'
| 'registerSliderControl'
| 'setActive'
| 'setDragging'
| 'setValue'
| 'step'
| 'thumbRefs'
> {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ const testRootContext: SliderRootContext = {
}),
setValue: NOOP,
largeStep: 10,
lastChangedValueRef: { current: null },
thumbMap: new Map(),
max: 100,
min: 0,
Expand Down
38 changes: 38 additions & 0 deletions packages/react/src/slider/root/SliderRoot.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -1293,6 +1293,44 @@ describeSkipIf(typeof Touch === 'undefined')('<Slider.Root />', () => {
value: 4,
});
});

it('onValueCommitted is called with the same value as the latest onValueChange when pointerUp occurs at a different location than onValueChange', async () => {
const handleValueChange = spy();
const handleValueCommitted = spy();

await render(
<TestSlider
onValueChange={handleValueChange}
onValueCommitted={handleValueCommitted}
defaultValue={0}
/>,
);

const sliderControl = screen.getByTestId('control');

stub(sliderControl, 'getBoundingClientRect').callsFake(
() => GETBOUNDINGCLIENTRECT_HORIZONTAL_SLIDER_RETURN_VAL,
);

fireEvent.pointerDown(sliderControl, {
buttons: 1,
clientX: 10,
});
fireEvent.pointerMove(sliderControl, {
buttons: 1,
clientX: 15,
});
fireEvent.pointerUp(sliderControl, {
buttons: 1,
clientX: 20,
});

expect(handleValueChange.callCount).to.equal(2);
expect(handleValueChange.args[0][0]).to.equal(10);
expect(handleValueChange.args[1][0]).to.equal(15);
expect(handleValueCommitted.callCount).to.equal(1);
expect(handleValueCommitted.args[0][0]).to.equal(15);
});
});

describe('keyboard interactions', () => {
Expand Down
2 changes: 1 addition & 1 deletion packages/react/src/slider/root/SliderRoot.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -288,7 +288,7 @@ SliderRoot.propTypes /* remove-proptypes */ = {
/**
* Callback function that is fired when the slider's value changed.
*
* @param {number | number[]} value The new value.
* @param {number | readonly number[]} value The new value.
* @param {Event} event The corresponding event that initiated the change.
* You can pull out the new value by accessing `event.target.value` (any).
* @param {number} activeThumbIndex Index of the currently moved thumb.
Expand Down
Loading
Loading