-
-
Notifications
You must be signed in to change notification settings - Fork 32.4k
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
Conversation
@@ -207,9 +207,10 @@ describe('<SwitchBase />', () => { | |||
); | |||
|
|||
getByRole('checkbox').click(); | |||
setProps({ checked: false }); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this 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.
Co-authored-by: Olivier Tassinari <[email protected]>
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
|
@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. |
Breaking change
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 usingevent.target.checked
.