-
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
components: Allow for non-polymorphic components #32796
Conversation
1a1ef99
to
19869e8
Compare
Size Change: 0 B Total Size: 1.04 MB ℹ️ View Unchanged
|
e7e4243
to
c8a0ecf
Compare
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.
Love the new, simpler types! LGTM 🚀
We should probably rename all this stuff to just like "WordPressComponent" or something like that.
+1
export type PolymorphicComponent< | ||
T extends React.ElementType, | ||
O, | ||
IsPolymorphic extends boolean |
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.
Why doesn't IsPolymorphic
here default to true
, like it does in PolymorphicComponentProps
?
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 because PolymorphicComponent is never used directly, so there's no reason to give it a default. It helps with compiler warnings when casting PolymorphicComponentProps to a PolymorphicComponent in case there's ever a mistake there.
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.
Got it, thank you for the explanation. Feel free to ignore this comment :)
c8a0ecf
to
e7b6ca4
Compare
Description
Simplifies some type extraction and also adds non-polymorphism.
We should probably rename all this stuff to just like "WordPressComponent" or something like that.
How has this been tested?
Type checks pass! Also you should be able to use the
Divider
and not assign anas
prop to it, TypeScript should error telling you that you cannot assign anything tonever
.Types of changes
New feature
Checklist:
*.native.js
files for terms that need renaming or removal).