components: InputControl to TypeScript #33696
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
sarayourfriend
requested review from
ajitbohra and
chrisvanpatten
as code owners
|
Size Change: +1.93 kB (0%) Total Size: 1.08 MB
ℹ️ View Unchanged
|
ciampo
reviewed
Jul 27, 2021
There was a problem hiding this comment.
Great work, @sarayourfriend !
It's a bit hard for me to review in detail all of the types that you extracted to the types.ts file, since this is quite a big PR and I'm not super familiar with how the component works internally.
I tried to focus particularly for any potential runtime change, and left a few comments
stokesman
reviewed
Jul 27, 2021
stokesman
reviewed
Jul 27, 2021
diegohaz
reviewed
Jul 28, 2021
diegohaz
reviewed
Jul 28, 2021
Comment on lines
35
to
39
| return { | ||
| ...initialInputControlState, | ||
| ...initialState, | ||
| initialValue: value, | ||
| }; | ||
| } as InputState; |
There was a problem hiding this comment.
Why does this need to be explicitly cast? TypeScript should be able to infer and combine the types from the other objects. If there's an error or something, there may be a bug there that the cast is hiding?
There was a problem hiding this comment.
It was because the initialValue couldn't be undefined and the Partial<InputState> was making it allowed to be undefined. But I think previously this was allowable anyway so I've made value and initialValue undefined.
Though won't that mess up the "controlled-ness" of the input? Should we only allow "empty" values but not undefined?
There was a problem hiding this comment.
I'm going to merge this PR and we can refine the types for this if we decide it's a problem. Currently I think we're not introducing anything that isn't already true for the intrinsic input element.
diegohaz
reviewed
Jul 28, 2021
Comment on lines
185
to
+188
| dispatch( { | ||
| type, | ||
| payload: { value: nextValue, event }, | ||
| } ); | ||
| } as actions.InputAction ); |
There was a problem hiding this comment.
Same here I guess. What's the error if we don't explicitly cast this object?
There was a problem hiding this comment.
It complains that the type parameter must be INVALIDATE for some reason. I'm not sure why it complains here and not in the other places 🤔
diegohaz
reviewed
Jul 28, 2021
sarayourfriend
added
the
[Type] Code Quality
7 tasks
ciampo
reviewed
Aug 2, 2021
* Convert Select to TypeScript * Stop spreading props into InputBase and add all HTML attributes for select * Remove need for `omit`
|
Whoops, I accidentally merged the SelectControl into this PR 😬 But it's been reviewed here #33784 |
5 tasks
ciampo
reviewed
Aug 4, 2021
6 tasks
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.
Description
I did this refactor while I was working on something else so I figured I might as well open a PR for it.
It just converts the InputControl to TypeScript and (I think) improves the way the reducer code is structured while we're at it.
How has this been tested?
Storybook should work the same for InputControl, NumberControl and UnitControl, the last two depend on InputControl.
TS build should pass as well.
Types of changes
Janitorial
Checklist:
*.native.jsfiles for terms that need renaming or removal).