RangeControl: Convert component to TypeScript #40535
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
aaronrobertshaw
added
[Type] Enhancement
|
Size Change: +36 B (0%) Total Size: 1.25 MB
ℹ️ View Unchanged
|
ciampo
reviewed
Apr 22, 2022
ciampo
reviewed
Apr 22, 2022
There was a problem hiding this comment.
Great job so far, @aaronrobertshaw !
I have a couple of observations / questions:
-
Is there particular reason why only some components are typed through
WordPressComponentProps? (RangeMark,RangeRail,Marksare not) -
Regarding the
valueprop:- we should consider if we should type it as
number | string, and handle any parsing internally. - we should also try to align the
nullandundefinedinstances, and try to pick one out of the two
- we should consider if we should type it as
|
I appreciate the early feedback @ciampo, thank you 👍
I don't believe there was any reason except that I missed
I think from the I'm probably missing something here so happy to take on board any further suggestions.
Thanks for the heads up. I've had a quick look at those PRs and it's probably best if they do land first given their importance and functional changes. I'll try and help out with reviews or testing on those, then resolve the ensuing conflicts on this PR. |
Thank you! And you're right re.
The point that you make is totally valid, and is actually the main point of debate around converting to TypeScript the
And so, by picking the Here's a few considerations off the top of my head:
Probably the best way to move forward, considering that We should also assess how (curious to know that @mirka thinks about this) (Coincidentally, we have a related conversation on the PR migrating
Agreed, thank you for understanding and prioritizing accordingly! |
This sounds reasonable to me. To be honest I don't have much useful thoughts to add here since I haven't actually fought this particular chain of dragons yet 😇 |
aaronrobertshaw
changed the base branch from
trunk
to
fix/input-field-reset-behavior
aaronrobertshaw
force-pushed
the
refactor/range-control-to-typescript
branch
from
0013ca3 to
aaec90c
Compare
stokesman
force-pushed
the
fix/input-field-reset-behavior
branch
from
c8a8013 to
5150ade
Compare
stokesman
force-pushed
the
fix/input-field-reset-behavior
branch
from
5150ade to
3ac6020
Compare
aaronrobertshaw
force-pushed
the
refactor/range-control-to-typescript
branch
from
a7a5e07 to
78783d3
Compare
Thank you for the update! Let us know once this PR is ready for another round of review :) |
aaronrobertshaw
force-pushed
the
refactor/range-control-to-typescript
branch
from
165ecdd to
a94465f
Compare
aaronrobertshaw
removed
the
[Status] In Progress
See: #40535 (comment) for more information on why we are ignoring this error. TL;DR we are looking to address some conflicts between input control types for their value props.
aaronrobertshaw
force-pushed
the
refactor/range-control-to-typescript
branch
from
a435f81 to
f19616e
Compare
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.
Depends On:
Related:
What?
Converts the
RangeControlcomponent to TypeScript.(Unit tests and story TypeScript conversions are in separate PRs, #41444 & #41445)
Why?
While looking to create a new component combining both a
UnitControlandRangeControl(#40462) it presented an opportunity to improve the slider components we offered in a more composable way.The first step in this process was to type the
RangeControlcomponent.See #40507 for more details.
How?
Refactors the current
RangeControlto TypeScript.Testing Instructions
npm run dev:package-typesand ensure no typing errorsnpm run test-unit packages/components/src/range-control/test/and check they all pass