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

[Select] onChange fired with current value #20351

Closed
2 tasks done
ksrb opened this issue Mar 31, 2020 · 1 comment · Fixed by #20361
Closed
2 tasks done

[Select] onChange fired with current value #20351

ksrb opened this issue Mar 31, 2020 · 1 comment · Fixed by #20361
Labels
component: select This is the name of the generic UI component, not the React module! good first issue Great for first contributions. Enable to learn the contribution process.

Comments

@ksrb
Copy link
Contributor

ksrb commented Mar 31, 2020

  • The issue is present in the latest release.
  • I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 😯

<Select> onChange handler is being called when the value is not actually changed/the user has selected the same value.

Expected Behavior 🤔

<Select> onChange to only be called when the value is actually changed.

Steps to Reproduce 🕹

Fork of https://material-ui.com/components/selects/#simple-select

https://codesandbox.io/s/material-demo-b7fys

Steps:

  1. Change value
  2. Notice 'Called' appears in console, this is correct
  3. Select same value
  4. Notice 'Called' appears again the console, this is incorrect?
  5. Native select available for comparison

Context 🔦

Just noticed this happening and have dealt with it by doing a simple equality check, however, for consistency it would be nice if the non-native and native select onChange behavior would be the same.

Your Environment 🌎

Using code sandbox all latest dependencies at time of writing

@oliviertassinari oliviertassinari added component: select This is the name of the generic UI component, not the React module! good first issue Great for first contributions. Enable to learn the contribution process. labels Mar 31, 2020
@oliviertassinari
Copy link
Member

@ksrb Thanks for the report. I remember handling this problem with the Autocomplete in:

https://github.com/mui-org/material-ui/blob/5aad5f2f729066534bd9d408cc8518b51a9f0f14/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js#L482

I would propose the same fix for the Select component. What do you think of this patch? :)

diff --git a/packages/material-ui/src/Select/Select.test.js b/packages/material-ui/src/Select/Select.test.js
index afe173849..fc559858e 100644
--- a/packages/material-ui/src/Select/Select.test.js
+++ b/packages/material-ui/src/Select/Select.test.js
@@ -214,7 +214,7 @@ describe('<Select />', () => {
     it('should get selected element from arguments', () => {
       const onChangeHandler = spy();
       const { getAllByRole, getByRole } = render(
-        <Select onChange={onChangeHandler} value="1">
+        <Select onChange={onChangeHandler} value="0">
           <MenuItem value="0" />
           <MenuItem value="1" />
           <MenuItem value="2" />
diff --git a/packages/material-ui/src/Select/SelectInput.js b/packages/material-ui/src/Select/SelectInput.js
index 6ace9e2ba..4bc58297f 100644
--- a/packages/material-ui/src/Select/SelectInput.js
+++ b/packages/material-ui/src/Select/SelectInput.js
@@ -139,6 +139,10 @@ const SelectInput = React.forwardRef(function SelectInput(props, ref) {
       newValue = child.props.value;
     }

+    if (value === newValue) {
+      return;
+    }
+
     setValue(newValue);

     if (onChange) {

Do you want to send a pull request?

@oliviertassinari oliviertassinari changed the title Select onChange fired when there is no change in non-native selects [Select] onChange fired with current value Mar 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: select This is the name of the generic UI component, not the React module! good first issue Great for first contributions. Enable to learn the contribution process.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants