NumberControl: Add TypeScript prop types#43791
Merged
Conversation
|
Size Change: +2.84 kB (0%) Total Size: 1.25 MB
ℹ️ View Unchanged
|
mirka
commented
Sep 1, 2022
| }; | ||
|
|
||
| const autoComplete = typeProp === 'number' ? 'off' : null; | ||
| const autoComplete = typeProp === 'number' ? 'off' : undefined; |
Member
Author
There was a problem hiding this comment.
null is an invalid value for the autoComplete attribute.
| return ( | ||
| <Root className="components-unit-control-wrapper" style={ style }> | ||
| <ValueInput | ||
| aria-label={ label } |
Member
Author
There was a problem hiding this comment.
label variable is a ReactNode, but aria-label only accepts strings. I went ahead and removed this redundant aria-label, because the same value is added on line 270 via label prop.
Contributor
There was a problem hiding this comment.
Noted. Basically the input will be labelled via an actual label element (with the for attribute), instead of the aria-label attribute 👌
Contributor
|
Going to leave a review at later / early next week, but in the meantime I'm going to leave a link to my previous attempt #38753 as it may be useful |
ciampo
reviewed
Sep 5, 2022
Contributor
ciampo
left a comment
There was a problem hiding this comment.
Good job! I left a few comments, a good chunk of which are inspired by the previous work in #38753.
Regarding the migration to TypeScript, the main part that still need to be addressed is which type we should assign to the value prop. I wrote a good recap of the situation in #40535 (comment). Although I would understand if we preferred to defer that decision to a separate PR, for the sake of getting this PR merged quickly and unblock further work that depends on NumberControl being typed
| return ( | ||
| <Root className="components-unit-control-wrapper" style={ style }> | ||
| <ValueInput | ||
| aria-label={ label } |
Contributor
There was a problem hiding this comment.
Noted. Basically the input will be labelled via an actual label element (with the for attribute), instead of the aria-label attribute 👌
ciampo
reviewed
Sep 5, 2022
Co-authored-by: Marco Ciampini <marco.ciampo@gmail.com>
Member
Author
Yes exactly, the main motivation of this PR is to unblock our way to clearing out the |
ciampo
reviewed
Sep 6, 2022
ciampo
reviewed
Sep 6, 2022
|
|
||
| const handleOnChange = ( next: string ) => { | ||
| const handleOnChange = ( next?: string ) => { | ||
| // @ts-expect-error TODO: Investigate if this is problematic |
Contributor
There was a problem hiding this comment.
Should this use ensureNumber ?
Member
Author
There was a problem hiding this comment.
This one can't be fixed by ensureNumber because the problem is that next can potentially be undefined.
Technically, we can remove the ts-ignore without changing effective runtime behavior by doing something like this:
let nextValue = typeof next === 'undefined' ? NaN : parseFloat( next );But then again, I really think this one needs further investigation because it's unclear whether it's intended for setValue() to sometimes receive a NaN. It could actually be problematic, so I wouldn't want to imply that this behavior is legit.
| const applyEmptyValue = | ||
| required === false && currentValue === ''; | ||
|
|
||
| // @ts-expect-error TODO: Investigate if this is wrong |
Contributor
There was a problem hiding this comment.
Instead of "Investigate if this is wrong", should the comment be more about the fact that this error will only be solved once we make a decision on the value type?
(same for the rest of the @ts-expect-error comments about the type of the value prop)
Member
Author
There was a problem hiding this comment.
Good call, I improved the TODO comments all around. 90948f0
One of the interesting causes was the isValueEmpty() util function. I was able to fix the typing, but it's a bit out of scope for this PR so I'll submit it separately.
This was referenced Sep 6, 2022
ciampo
approved these changes
Sep 7, 2022
Contributor
ciampo
left a comment
There was a problem hiding this comment.
🚀
Good to be merged once we add a CHANGELOG entry
This was referenced Sep 8, 2022
This was referenced Sep 9, 2022
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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?
Adds "provisional" prop types to NumberControl.
Why?
In the past, we've had major complications with properly typing NumberControl due to the string/number ambiguity of the values both upstream and downstream. This is why NumberControl is untyped and
@ts-nochecked at the moment.However, this has started to bottleneck TypeScript migrations for other components that inherit NumberControl props. I think it would be useful to improve the situation a bit by actually defining prop types, even if there are some contradictions that need to be
@ts-ignored.I hope this reads as a constructive baby step forward, rather than a messy pile of bandaids. The benefit is that we can clearly demarcate where the type contradictions are, instead of keeping a huge untyped blackhole in our component chain.
How?
See inline code comments for minor runtime changes.
Testing Instructions
npm run storybook:devand see the Docs view for NumberControl and UnitControl.