-
-
Notifications
You must be signed in to change notification settings - Fork 32.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
[core] Type custom onChange
implementations with a generic react event
#21552
Conversation
@@ -8,7 +8,7 @@ export type TabTypeMap<P = {}, D extends React.ElementType = 'div'> = ExtendButt | |||
fullWidth?: boolean; | |||
icon?: string | React.ReactElement; | |||
label?: React.ReactNode; | |||
onChange?: (event: React.ChangeEvent<{ checked: boolean }>, value: any) => void; |
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.
I don't know why the target should implement checked
. Probably some wrong c&p while refactoring.
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.
Awesome, I was leaning in the same direction. I think that it could be interesting to add a test case for #20191 too (don't we solve this issue too?).
Yes! Thanks for linking it. It's the same underlying issue. Will add the same test. I'm also checking out again why our docs were working fine |
5e423d2
to
31f261c
Compare
onChange
as a generic react eventonChange
implementations with a generic react event
3211703
to
4977b6f
Compare
Closes #17454
Closes #20191
for:
Accordion
(formerExpansionPanel
)BottomNavigation
Slider
Tabs
These components do not necessarily dispatch a react change event. They only passes whatever event triggered the change in value. I don't want to type it as exact as possible (
FocusEvent | ClickEvent
) because that restricts our implementation. We simply say that it's any react event at this point. You would have to narrow it later anyway which we can always do later if there are legitimate use cases.The takeaway from this typing is that
event.target
is not interchangeable with aref
on the component.event.target
could point to any element that the component rendered. It's a bit unfortunate that we overload what is usually considered a change event. We have to make the trade-off at some point: either when passing props (Slider behaves just like any other input: I pass avalue
andonChange
) or when typing theonChange
(event.target === event.currentTarget).Removes
onChange
types fromBottomNavigationAction
andTab
. These are overridden anyway and considered private.About OverrideableComponent changes
Let's say we have a component that implements the
component
likePreviously we assumed that any prop that is implemented by
Button
is also forwarded to the passedcomponent
.However, this is almost never the case e.g. in
Tab
we expect that the givencomponent
either doesn't implementonChange
or as the same type.The reality is that
Tab
does not forwardonChange
and thereforecomponent
never "sees"onChange
.There are some exceptions e.g.
ButtonBase
interceptshref
but also passes it along. We currently expect thehref
to be a string but this isn't required if acomponent
is passed.Instead of expecting the passed
component
to extend the props ofButton
we should expectcomponent
to implement a separate interface e.g. inthe passed
component
needs to implement thetarget
prop. It doesn't need to implementexternal
nor should it ever expect to receive anexternal
prop if it is used in<Button />
.This change is a bit scary but safe in
next
. I'll follow-up in a later PR with more test and experimentation if it makes sense to requirecomponent
to implement a certain interface.Future work
React.ChangeEvent<{}>
. It works structurally right now but has some implications that can't be expressed via types yet so we need to be careful with using it.