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] onChange being called when value is not changing #36614

Closed
2 tasks done
DanSchoppe opened this issue Mar 22, 2023 · 3 comments · Fixed by #36706
Closed
2 tasks done

[Slider] onChange being called when value is not changing #36614

DanSchoppe opened this issue Mar 22, 2023 · 3 comments · Fixed by #36706
Assignees
Labels
bug 🐛 Something doesn't work component: slider This is the name of the generic UI component, not the React module!

Comments

@DanSchoppe
Copy link

Duplicates

  • I have searched the existing issues

Latest version

  • I have tested the latest version

Steps to reproduce 🕹

Link to live example:
https://codesandbox.io/s/mui-slider-excessive-onchange-dudwxj?file=/demo.tsx
(^ forked from Discrete Sliders demo)

Steps:

  1. Open console
  2. Click and release slider thumb
  3. Observe onChange handler has fired and console.log'd the existing value without a change to value

Current behavior 😯

onChange fires when the slider thumb is clicked, but not moved.

Expected behavior 🤔

Per #34448: [Slider] Only call onChange if the value changed.

Perhaps these tests are not effective:

it('should only fire onChange when the value changes', () => {
const handleChange = spy();
const { container } = render(<Slider defaultValue={20} onChange={handleChange} />);
stub(container.firstChild, 'getBoundingClientRect').callsFake(() => ({
width: 100,
left: 0,
}));
fireEvent.mouseDown(container.firstChild, {
buttons: 1,
clientX: 21,
});
fireEvent.mouseMove(document.body, {
buttons: 1,
clientX: 22,
});
// Sometimes another event with the same position is fired by the browser.
fireEvent.mouseMove(document.body, {
buttons: 1,
clientX: 22,
});
expect(handleChange.callCount).to.equal(2);
expect(handleChange.args[0][1]).to.deep.equal(21);
expect(handleChange.args[1][1]).to.deep.equal(22);
});

Context 🔦

Excessive onChange invocations can degrade the performance of an application if downstream effects are expensive to compute.

Your environment 🌎

No response

@DanSchoppe DanSchoppe added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Mar 22, 2023
@zannager zannager added the component: slider This is the name of the generic UI component, not the React module! label Mar 23, 2023
@mj12albert mj12albert assigned DanSchoppe and unassigned michaldudak Mar 23, 2023
@mj12albert
Copy link
Member

@DanSchoppe Thanks for reporting this, I agree its a bug, clicking a Thumb in place doesn't change the value and shouldn't call onChange

@mj12albert mj12albert added bug 🐛 Something doesn't work and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Mar 23, 2023
@gitstart
Copy link
Contributor

@DanSchoppe @mj12albert @michaldudak I would like to pick this up

@ZeeshanTamboli
Copy link
Member

@gitstart Please go ahead and create a PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: slider This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants