-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
UnitControl: fix exhaustive-deps warnings #44161
Conversation
Size Change: +4 B (0%) Total Size: 1.26 MB
ℹ️ View Unchanged
|
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.
Looks good to me 🚀
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.
Just noting that this now makes the effect also fire each time the number is changed, where previously it was only when the unit is changed. Would that cause any problems?
Might be worth trying to stabilize the state setter in useControlledState
itself?
@@ -100,7 +100,7 @@ function UnforwardedUnitControl( | |||
if ( parsedUnit !== undefined ) { | |||
setUnit( parsedUnit ); | |||
} | |||
}, [ parsedUnit ] ); | |||
}, [ parsedUnit, setUnit ] ); |
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.
Ooh, TIL about this useControlledState
thing. Looks useful. I just saw the same pattern in ToggleGroupControl, but without using this utility hook.
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.
Yup!
And there's also useControlledValue
🤦 It would be good to see if we can use only one implementation across the whole package
I can look into it and see what happens! |
d88f29d
to
a4324af
Compare
Done in a4324af |
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.
Works as expected 👍
What?
Part of #41166
Fix all exhaustive-deps warnings on the
UnitControl
component.Why?
See #41166 for more context
How?
Add missing deps on the web component, add eslint-disable comments on the native component
Testing Instructions
npx eslint --rule 'react-hooks/exhaustive-deps: warn' packages/components/src/unit-control/
and make sure that no warnings are printed