-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
CustomSelectControl: support generic props type #63985
CustomSelectControl: support generic props type #63985
Conversation
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @meteorlxy! In case you missed it, we'd love to have you join us in our Slack community. If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information. |
c3d5dc4
to
4f5c724
Compare
packages/components/CHANGELOG.md
Outdated
@@ -6,6 +6,10 @@ | |||
|
|||
- `CustomSelectControl`: Restore `describedBy` functionality ([#63957](https://github.com/WordPress/gutenberg/pull/63957)). | |||
|
|||
### Enhancements |
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.
Since this is a type change, I believe it should be under Internal
instead.
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.
Cuz the type will be used by user, and will improve the developer experience, I think it's more of an improvement / enhancement?
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.
IMHO, types and type changes are an internal kind of enhancement, and we have this "Internal" group to distinguish them. General Enhancements would usually be targeted towards end users.
@@ -51,7 +51,9 @@ function getDescribedBy( currentValue: string, describedBy?: string ) { | |||
return sprintf( __( 'Currently selected: %s' ), currentValue ); | |||
} | |||
|
|||
function CustomSelectControl( props: CustomSelectProps ) { | |||
function CustomSelectControl< T extends CustomSelectOption >( |
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.
Should we add a default to the generic so any existing consumers will continue working without changing anything?
The same question applies to all instances where we're adding a generic and there wasn't one before.
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.
The generic param is not going to be set manually by user. It's used for type inferring. To be specific, it will use the item type of the options
prop automatically. So consumers would not need to change anything.
You could checkout this branch and test with the snippets that pasted in the "Testing Instructions" section.
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.
TypeScript should always be able to infer the T
type. CustomSelectProps
have a mandatory options: Array<T>
field, so T
is always the type of whatever you pass as options
, it's easy to infer. TypeScript will merely check if that type is compatible with CustomSelectOption
.
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.
This looks good and tests well in my editor, thanks for the contribution @meteorlxy 🙌
The only thing I'd change would be to move the CHANGELOG entry to "Internal".
@tyxla updated |
@@ -24,15 +24,15 @@ type Option = { | |||
/** | |||
* The object returned from the onChange event. | |||
*/ | |||
type ChangeObject = { | |||
export type CustomSelectChangeObject< T extends CustomSelectOption > = { |
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.
Do we need to export this one? Seems like we don't.
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.
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.
Right 👍
Exporting it here doesn't export it from the package, though. And if we're not importing it inside the package, then it's effectively unnecessary to export it, IMHO. We can always start exporting it if we need to in the future.
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.
Yes, at the moment we are avoiding public type exports as much as possible, thanks 🙏
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.
ok, removed
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, thank you! 🚀
What?
Fixes #63984
Why?
It would be helpful to infer the option type
How?
CustomSelectProps
a generic typeCustomSelectOption
andCustomSelectChangeObject
typeTesting Instructions
Using the following snippet to see if the
selectedItem
type is inferred correctly:Testing Instructions for Keyboard
N/A
Screenshots or screencast
Before
After