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

[Checkbox][Switch] Remove checked argument from onChange #25871

Merged
merged 5 commits into from
Apr 24, 2021

Conversation

m4theushw
Copy link
Member

@m4theushw m4theushw commented Apr 21, 2021

Breaking change

function MyCheckbox() {
- const handleChange = (event: React.ChangeEvent<HTMLInputElement>, checked: boolean) => {
+ const handleChange = (event: React.ChangeEvent<HTMLInputElement>) => {
+   const checked = event.target.checked;
  };
  return <Checkbox onChange={handleChange} />;
}
function MySwitch() {
- const handleChange = (event: React.ChangeEvent<HTMLInputElement>, checked: boolean) => {
+ const handleChange = (event: React.ChangeEvent<HTMLInputElement>) => {
+   const checked = event.target.checked;
  };
  return <Switch onChange={handleChange} />;
}

Part of #20012.

This PR changes the signature of the onChange prop in SwitchBase to remove the second argument. Users can obtain the same outcome using event.target.checked.

@mui-pr-bot
Copy link

mui-pr-bot commented Apr 21, 2021

Details of bundle changes

Generated by 🚫 dangerJS against 84fa324

@@ -207,9 +207,10 @@ describe('<SwitchBase />', () => {
);

getByRole('checkbox').click();
setProps({ checked: false });
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're relaying on a reference to the target now. If the checked prop is not updated, event.target.checked will be true again in next tick.

Copy link
Member

@eps1lon eps1lon Apr 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call pointing this out. The click() was missing an act() i.e. flushing of effects. The setProps was implicitly doing that.

The other problem is that the test was no longer testing the click() but rather setProps({ checked: false }). I changed the test so that we actually test `onChange={(event) => setChecked(event.target.checked)}: bacce0c

@oliviertassinari oliviertassinari changed the title [SwitchBase] Remove checked argument from onChange [Checkbox][Switch] Remove checked argument from onChange Apr 21, 2021
@oliviertassinari oliviertassinari added component: checkbox This is the name of the generic UI component, not the React module! component: switch This is the name of the generic UI component, not the React module! labels Apr 21, 2021
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It also impacts the Checkbox. How about we update the migration guide as well?
Regarding introducing a deprecation in v4, I can't think of any obvious way to do such. Is it possible? Maybe not.

docs/src/pages/guides/migration-v4/migration-v4.md Outdated Show resolved Hide resolved
m4theushw and others added 2 commits April 21, 2021 19:41
Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
@m4theushw
Copy link
Member Author

Regarding introducing a deprecation in v4, I can't think of any obvious way to do such. Is it possible? Maybe not.

I think that only with ESLint is possible to give such warning. However, the plugin we have is for internal use.

A bit hacky, but we could warn if event.target.checked was not used inside the onChange callback. The trade-offs are:

  • Users that don't use it would still be impacted.
  • We have to check if onChange returns a Promise.

@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 21, 2021

@m4theushw Ok, I vote for not doing any deprecation, I think that it is only worth it if it's simple to put in place.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 23, 2021
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change component: checkbox This is the name of the generic UI component, not the React module! component: switch This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants