Skip to content

Commit

Permalink
[Slider] onChange handler should be called only when value has chan…
Browse files Browse the repository at this point in the history 
…ged (#36706)

Co-authored-by: seunexplicit <[email protected]>
  • Loading branch information
@gitstart @seunexplicit
gitstart and seunexplicit committed Apr 20, 2023
1 parent cd7bd09 commit 614cae5
Show file tree
Hide file tree
Showing 2 changed files with 58 additions and 28 deletions.
23 changes: 18 additions & 5 deletions packages/mui-base/src/useSlider/useSlider.ts
View file
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
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

0 comments on commit 614cae5

Please sign in to comment.