ComboboxControl: Convert to TypeScript #47581
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
It doesn't do anything
This definitely works with the initial value undefined, as seen in the readme code snippet and in unit tests.
|
Size Change: +34 B (0%) Total Size: 1.31 MB
ℹ️ View Unchanged
|
mirka
commented
Jan 30, 2023
| @@ -235,15 +257,14 @@ function ComboboxControl( { | |||
| className, | |||
| 'components-combobox-control' | |||
| ) } | |||
| tabIndex="-1" | |||
There was a problem hiding this comment.
Removed this tabIndex because it isn't a valid prop on BaseControl and doesn't do anything.
| - Type: `mixed` | ||
| - Required: Yes | ||
| - Type: `string | null` | ||
| - Required: No |
There was a problem hiding this comment.
This definitely works with the initial value undefined, as seen in the readme code snippet and in unit tests.
There was a problem hiding this comment.
Interesting to see how null is being used here, in the context of #47473
There was a problem hiding this comment.
Exactly, it might even be the first one I've seen with an explicit null 🤔
Comment on lines
+53
to
+59
| const getIndexOfMatchingSuggestion = ( | ||
| selectedSuggestion: ComboboxControlOption | null, | ||
| matchingSuggestions: ComboboxControlOption[] | ||
| ) => | ||
| selectedSuggestion === null | ||
| ? -1 | ||
| : matchingSuggestions.indexOf( selectedSuggestion ); |
There was a problem hiding this comment.
Typesafe helper function to explicitly handle the case when selectedSuggestion is null. Should not change runtime behavior, as Array.indexOf() will return -1 when there is no match.
Comment on lines
+17
to
+23
| const countries = [ | ||
| { name: 'Afghanistan', code: 'AF' }, | ||
| { name: 'Åland Islands', code: 'AX' }, | ||
| { name: 'Albania', code: 'AL' }, | ||
| { name: 'Algeria', code: 'DZ' }, | ||
| { name: 'American Samoa', code: 'AS' }, | ||
| ]; |
There was a problem hiding this comment.
I shortened this list for simplicity.
ciampo
reviewed
Feb 2, 2023
| - Required: No | ||
|
|
||
| #### onChange | ||
|
|
||
| Function called with the selected value changes. | ||
|
|
||
| - Type: `Function` | ||
| - Type: `( value: string | null ) => void` |
There was a problem hiding this comment.
(Not something strictly for this PR, more of a general reflection to be discussed and prioritised separately)
We should take a look at the type signature of onChange functions across the package. In particular:
- we should make sure that the value passed as argument of
onChangehas the same type as thevalueprop of the component - we should standardise the behaviour when the component's value is changed to an empty state (e.g cleared). Should we fire
onChangewithnullas a the argument ? Orundefined? I believe that for some components, theonChangecallback may not be fired at all in similar scenarios - we should try to also standardize the shape of the callback's first argument. For example,
TokenInputProps['onChange']passes en object with thevalueprop, instead of the value directly
What are your thoughts? We could probably open a separate issue to track this.
There was a problem hiding this comment.
I have a feeling we won't be able to actually standardize the APIs for our existing components (because back compat). The only thing we can do is agree on a standard moving forward for new components.
• we should make sure that the value passed as argument of
onChangehas the same type as thevalueprop of the component
• we should try to also standardize the shape of the callback's first argument
Yes 💯
• we should standardise the behaviour when the component's value is changed to an empty state (e.g cleared)
This will likely not be a concern for new components, since we're moving away from built in reset buttons. If it does arise though, I still think calling onChange( undefined ) is the most straightforward and "standardizable" way. This is an overview of the current built-in reset behavior by the way:
undefined
- ColorPalette
- DuotonePicker
- FontSizePicker
- GradientPicker
- RangeControl
- BoxControl (
Record< string, undefined >)
null
- ComboboxControl
- DateTimePicker
empty string
- SearchControl
empty array
- FormTokenField
There was a problem hiding this comment.
I have a feeling we won't be able to actually standardize the APIs for our existing components (because back compat). The only thing we can do is agree on a standard moving forward for new components.
Yeah, I have the same suspicion. But as you say, we can at least enforce a standard for future work
This is an overview of the current built-in reset behavior by the way:
This recap is already so valuable, thank you!
| }, | ||
| { | ||
| value: 'jake', | ||
| label: 'Jake Weary', |
There was a problem hiding this comment.
Best web dev joke since 2006. Cracks me up every time 🤣
There was a problem hiding this comment.
Omg I didn't even notice! 😂
ciampo
reviewed
Feb 2, 2023
| /** | ||
| * Function called with the selected value changes. | ||
| */ | ||
| onChange?: ( value: ComboboxControlProps[ 'value' ] ) => void; |
There was a problem hiding this comment.
Is it expected that the argument can be undefined ? Otherwise we should wrap ComboboxControlProps[ 'value' ] with NonNullable
There was a problem hiding this comment.
Hmm, good question 🤔 It will never fire undefined with the current code, but there's also a part of me that wants to keep our options open vis-à-vis the controversial null problem. Do you feel strongly about it either way? I don't have a strong opinion since it's possible that we'll never be able to change the reset button behavior anyway.
(📌 TODO for self: Align with type annotation in readme)
There was a problem hiding this comment.
You make a good point — let's keep it as-is (ie. allowing null and undefined)
|
Flaky tests detected in 91bc89a. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4107266404
|
ciampo
approved these changes
Feb 6, 2023
DaisyOlsen
added
the
[Type] Code Quality
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Part of #35744
What?
Converts the ComboboxControl component to TypeScript, including stories and tests.
How?
See inline comments.
Testing Instructions
npm run storybook:devand see the Docs view for ComboboxControl.