TextControl: Convert component to TypeScript #40633
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
walbo
added
[Type] Enhancement
|
Size Change: +7 B (0%) Total Size: 1.23 MB
ℹ️ View Unchanged
|
ciampo
reviewed
Apr 28, 2022
There was a problem hiding this comment.
Thank you for helping out on the @wordpress/components TypeScript refactor, @walbo !
I just left a couple of inline comments, but this PR is in a great shape already.
Finally, would you mind adding an entry to the pachake's CHANGELOG ? You may want to add an ### Internal section under ## Unreleased.
| /** | ||
| * The current value of the input. | ||
| */ | ||
| value: string | number; |
There was a problem hiding this comment.
If I were to write this component from scratch, I would type value as a string.
Since this component is not experimental (and since the purpose of these TypeScript migrations is to introduce the least amount of changes possible), we may want to keep this type as it currently is, but consider updating it in a follow-up (see also this recent related conversation)
There was a problem hiding this comment.
I agree it should be string in this case. The old jsdocs said string but the README file says string | number.
There was a problem hiding this comment.
Let's keep it as string|number for this PR. It would be great if we could then open a follow-up PR to change it to be only string — we could then assess in that PR if effectively it's a change that can be applied without introducing any breaking change.
Would you be up for that?
There was a problem hiding this comment.
Yes. Lets do that. I will creeate a follow-up PR once this is merged.
mirka
reviewed
Apr 28, 2022
| import type { WordPressComponentProps } from '../ui/context'; | ||
| import type { TextControlProps } from './types'; | ||
|
|
||
| function UnforwardedTextControl( |
There was a problem hiding this comment.
No worries :) Great that you updated the guide. Did follow the guide and seems to cover most of the stuff you need.
There was a problem hiding this comment.
Thank you for the feedback! And of course do let us know if you feel like there's something missing (or something wrong) in the guide 😄
| argTypes: { | ||
| onChange: { | ||
| action: 'onChange', | ||
| control: { type: null }, |
There was a problem hiding this comment.
Nit: It looks like we could remove the control: { type: null } in this case. Storybook hides the controls for on* props by default.
There was a problem hiding this comment.
Thanks. I didn't know that 👍 Removed in e30a8dd
Comment on lines
37
to
48
| const DefaultTemplate: ComponentStory< typeof TextControl > = ( { | ||
| ...args | ||
| } ) => { | ||
| const [ value, setValue ] = useState( '' ); | ||
|
|
||
| return ( | ||
| <TextControl | ||
| { ...args } | ||
| value={ value } | ||
| onChange={ ( v ) => { | ||
| setValue( v ); | ||
| } } |
There was a problem hiding this comment.
Apparently, the Action thing (enabled on this line) only works by passing through the onChange prop.
const DefaultTemplate: ComponentStory< typeof TextControl > = ( {
+ onChange,
...args
} ) => {
const [ value, setValue ] = useState( '' );
return (
<TextControl
{ ...args }
value={ value }
onChange={ ( v ) => {
setValue( v );
+ onChange( v );CleanShot.2022-04-29.at.01.46.10-converted.mp4
- Remove control on onChange. Storybook hides on* by default. - Add onChange to trigger action
bph
changed the title
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.
What?
Converts the
TextControlcomponent to TypeScript.Why?
Part of the @wordpress/components's TypeScript migration (#35744).
How?
Refactors the current
TextControlto TypeScript.Testing Instructions
TextControlcontinues to function as expectedcc: @ciampo