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.

Sign up for GitHub

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] onChange handler should be called only when value has changed #36706

Merged
merged 10 commits into from
Apr 20, 2023
Merged

[Slider] onChange handler should be called only when value has changed #36706

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
572fd4d
fix: enable handleChange to be called only when value has changed
gitstart Mar 30, 2023
1806658
Merge commit '6566cce150596a35ea176ac55534de814816b2fb' of https://gi…
gitstart Apr 3, 2023
208690e
fix failing tests
gitstart Apr 3, 2023
55fce13
Merge commit '7239a54451ab85f85a9207d11cc1ce63cb39ad63' of https://gi…
gitstart Apr 12, 2023
9d966aa
implement feedback
gitstart Apr 12, 2023
f5121e2
fix: resolve prettier ci error
gitstart Apr 12, 2023
2bc1980
Merge commit '710dc38963b965f1aad4d58dfd325dd08b7d1592' of https://gi…
gitstart Apr 19, 2023
e926bd7
implement feedback
gitstart Apr 19, 2023
c52b6b6
Merge commit 'cd7bd09e503b7e64e5358ffdef7db88472fa29de' of https://gi…
gitstart Apr 20, 2023
f6ad0af
update test case
gitstart Apr 20, 2023
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
23 changes: 18 additions & 5 deletions packages/mui-base/src/useSlider/useSlider.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import {
UseSliderRootSlotProps,
UseSliderThumbSlotProps,
} from './useSlider.types';
import { EventHandlers } from '../utils';
import { areArraysEqual, EventHandlers } from '../utils';

const INTENTIONAL_DRAG_COUNT_THRESHOLD = 2;

Expand Down Expand Up @@ -140,6 +140,19 @@ function focusThumb({
}
}

function areValuesEqual(
newValue: number | Array<number>,
oldValue: number | Array<number>,
): boolean {
if (typeof newValue === 'number' && typeof oldValue === 'number') {
return newValue === oldValue;
}
if (typeof newValue === 'object' && typeof oldValue === 'object') {
return areArraysEqual(newValue, oldValue);
}
return false;
}

const axisProps = {
horizontal: {
offset: (percent: number) => ({ left: `${percent}%` }),
Expand Down Expand Up @@ -356,7 +369,7 @@ export default function useSlider(parameters: UseSliderParameters): UseSliderRet
setValueState(newValue);
setFocusedThumbIndex(index);

if (handleChange) {
if (handleChange && !areValuesEqual(newValue, valueDerived)) {
handleChange(event, newValue, index);
}

Expand Down Expand Up @@ -466,7 +479,7 @@ export default function useSlider(parameters: UseSliderParameters): UseSliderRet
setDragging(true);
}

if (handleChange && newValue !== valueDerived) {
if (handleChange && !areValuesEqual(newValue, valueDerived)) {
handleChange(nativeEvent, newValue, activeIndex);
}
});
Expand Down Expand Up @@ -517,7 +530,7 @@ export default function useSlider(parameters: UseSliderParameters): UseSliderRet

setValueState(newValue);

if (handleChange) {
if (handleChange && !areValuesEqual(newValue, valueDerived)) {
handleChange(nativeEvent, newValue, activeIndex);
}
}
Expand Down Expand Up @@ -584,7 +597,7 @@ export default function useSlider(parameters: UseSliderParameters): UseSliderRet

setValueState(newValue);

if (handleChange) {
if (handleChange && !areValuesEqual(newValue, valueDerived)) {
handleChange(event, newValue, activeIndex);
}
}
Expand Down
63 changes: 40 additions & 23 deletions packages/mui-material/src/Slider/Slider.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,23 +119,23 @@ describe('<Slider />', () => {
}));

fireEvent.touchStart(container.firstChild, createTouches([{ identifier: 1, clientX: 0 }]));
expect(handleChange.callCount).to.equal(1);
expect(handleChange.callCount).to.equal(0);
expect(handleChangeCommitted.callCount).to.equal(0);

fireEvent.touchStart(document.body, createTouches([{ identifier: 2, clientX: 40 }]));
expect(handleChange.callCount).to.equal(1);
expect(handleChange.callCount).to.equal(0);
expect(handleChangeCommitted.callCount).to.equal(0);

fireEvent.touchMove(document.body, createTouches([{ identifier: 1, clientX: 1 }]));
expect(handleChange.callCount).to.equal(2);
expect(handleChange.callCount).to.equal(1);
expect(handleChangeCommitted.callCount).to.equal(0);

fireEvent.touchMove(document.body, createTouches([{ identifier: 2, clientX: 41 }]));
expect(handleChange.callCount).to.equal(2);
expect(handleChange.callCount).to.equal(1);
expect(handleChangeCommitted.callCount).to.equal(0);

fireEvent.touchEnd(document.body, createTouches([{ identifier: 1, clientX: 2 }]));
expect(handleChange.callCount).to.equal(2);
expect(handleChange.callCount).to.equal(1);
expect(handleChangeCommitted.callCount).to.equal(1);
});

Expand Down Expand Up @@ -330,25 +330,27 @@ describe('<Slider />', () => {
left: 0,
}));

fireEvent.touchStart(
container.firstChild,
createTouches([{ identifier: 1, clientX: 21, clientY: 0 }]),
);
fireEvent.touchStart(container.firstChild, createTouches([{ identifier: 1, clientX: 20 }]));

fireEvent.touchMove(
document.body,
createTouches([{ identifier: 1, clientX: 22, clientY: 0 }]),
);
fireEvent.touchMove(
document.body,
createTouches([{ identifier: 1, clientX: 22.1, clientY: 0 }]),
);
fireEvent.touchMove(document.body, createTouches([{ identifier: 1, clientX: 21 }]));

fireEvent.touchEnd(document.body, createTouches([{ identifier: 1, clientX: 21 }]));

fireEvent.touchStart(container.firstChild, createTouches([{ identifier: 1, clientX: 21 }]));

expect(handleChange.callCount).to.equal(3);
fireEvent.touchMove(document.body, createTouches([{ identifier: 1, clientX: 22 }]));

fireEvent.touchEnd(document.body, createTouches([{ identifier: 1, clientX: 22 }]));

fireEvent.touchStart(container.firstChild, createTouches([{ identifier: 1, clientX: 22 }]));

fireEvent.touchMove(document.body, createTouches([{ identifier: 1, clientX: 22.1 }]));

fireEvent.touchEnd(document.body, createTouches([{ identifier: 1, clientX: 22.1 }]));

expect(handleChange.callCount).to.equal(2);
expect(handleChange.args[0][1]).to.deep.equal([21, 30]);
expect(handleChange.args[1][1]).to.deep.equal([22, 30]);
// TODO, consider not firing this change event since the values are the same to improve the DX.
expect(handleChange.args[2][1]).to.deep.equal([22, 30]);
});

it('should not react to right clicks', () => {
Expand Down Expand Up @@ -1116,12 +1118,20 @@ describe('<Slider />', () => {
</div>
);
}

render(<Test />);
const slider = screen.getByTestId('slider');

fireEvent.touchStart(slider, createTouches([{ identifier: 1 }]));
stub(slider, 'getBoundingClientRect').callsFake(() => ({
width: 100,
height: 10,
bottom: 10,
left: 0,
}));

expect(handleChange.callCount).to.equal(1);
fireEvent.touchStart(slider, createTouches([{ identifier: 1, clientX: 0 }]));

expect(handleChange.callCount).to.equal(0);
expect(handleNativeEvent.callCount).to.equal(1);
expect(handleNativeEvent.firstCall.args[0]).to.have.property('target', slider);
expect(handleEvent.callCount).to.equal(1);
Expand Down Expand Up @@ -1149,9 +1159,16 @@ describe('<Slider />', () => {
render(<Test />);
const slider = screen.getByTestId('slider');

stub(slider, 'getBoundingClientRect').callsFake(() => ({
width: 100,
height: 10,
bottom: 10,
left: 0,
}));

fireEvent.mouseDown(slider);

expect(handleChange.callCount).to.equal(1);
expect(handleChange.callCount).to.equal(0);
expect(handleNativeEvent.callCount).to.equal(1);
expect(handleNativeEvent.firstCall.args[0]).to.have.property('target', slider);
expect(handleEvent.callCount).to.equal(1);
Expand Down