Rewrite DatePicker, TimePicker and DateTimePicker in TypeScript #40775
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
noisysocks
force-pushed
the
add/more-DatePicker-tests
branch
from
7728df6 to
fce95e2
Compare
noisysocks
force-pushed
the
try/rewrite-date-time-picker-in-typescript
branch
from
2d24470 to
383ea01
Compare
noisysocks
force-pushed
the
try/rewrite-date-time-picker-in-typescript
branch
from
383ea01 to
9bcc66e
Compare
|
Size Change: +63 B (0%) Total Size: 1.24 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Nice work @noisysocks 👍
Everything is looking pretty good so far.
✅ Unit tests are still passing
✅ No typing errors reported
✅ DateTimePickers etc function as expected
I left a few small comments though. Mainly just passing on advice, requests or preferences I've been given while working on other components. Hopefully, these will help make our typing here a little more consistent with other components.
Not that it should happen in this PR but are we planning to also connect this component to the component context system?
P.S. This will also need a changelog entry
ciampo
reviewed
May 5, 2022
There was a problem hiding this comment.
Great work, @noisysocks!
This PR is already in a great shape — I only left a few minor inline comments.
In addition to those, it would be great if you could also add a CHANGELOG entry.
Thank you!
Comment on lines
+130
to
+124
| function update( name: 'date' | 'month' | 'year' | 'hours' | 'minutes' ) { | ||
| return ( value: number ) => { |
There was a problem hiding this comment.
We usually try not to introduce runtime changes — is there a particular reason why we're doing so here?
There was a problem hiding this comment.
So if you go with function update( name: string, value: number ) then you'll get an error because it can't be guaranteed that name will be a valid method in the moment object.
const newDate = date.clone()[ name ]( adjustedValue );Element implicitly has an 'any' type because expression of type 'string' can't be used to index type 'Moment'.
No index signature with a parameter of type 'string' was found on type 'Moment'.
If you go with function update( name: 'date' | 'month' | 'year' | 'hours' | 'minutes', value: number ) then you'll get an error because it can't be guaranteed that input will get back to you with a name that is one of date, month, year, hours, or minutes.
onUpdate={ update }Type '(name: "date" | "month" | "year" | "hours" | "minutes", value: number) => void' is not assignable to type '(name: string, value: number) => void'.
Types of parameters 'name' and 'name' are incompatible.
Type 'string' is not assignable to type '"date" | "month" | "year" | "hours" | "minutes"'.
One solution could be to paramatize UpdateOnBlurAsIntegerFieldProps.
type UpdateOnBlurAsIntegerFieldProps< T extends string > = {
name: T;
onUpdate: ( name: T, value: number ) => void;
};I think that is overly complex, though, so I went with changing update to be a partial function since that's a very common pattern and it's also how updateAmPm (immediately below this function) works.
There was a problem hiding this comment.
Thank you for the explanation — it makes sense to keep your changes
|
Feedback addressed. Let's see if it compiles. Thanks for the reviews @aaronrobertshaw and @ciampo! |
noisysocks
force-pushed
the
try/rewrite-date-time-picker-in-typescript
branch
from
618e1f9 to
0270b76
Compare
ciampo
approved these changes
May 9, 2022
There was a problem hiding this comment.
Thank you for working on this, @noisysocks ! All feedback so far has been addressed 🚀
Next up, we may want to look at rewriting Storybook examples and unit tests to TypeScript too (which can be done in 2 separate follow-ups) . Although I'm probably not going to be able to help with reviews until next Wednesday (May 18th), so no rush there!
I noticed that the WordPressComponentProps type is being only used for the UpdateOnBlurAsIntegerField component — we may want to look into using it for the remaining components too, but that can be also done in a follow-up PR (and with an even lower priority than the storybook and tests TS refactor).
noisysocks
added
[Type] Code Quality
This was referenced May 10, 2022
Unless I'm misunderstanding what |
7 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.
What?
Rewrites the component JavaScript in
packages/components/date-timein TypeScript.Part of #35744
Why?
We're gradually converting all of
@wordpress/components, and I'm about to do some work onDateTimePicker, so I thought I'd be a good citizen and help convert these components.How?
I followed steps 1-9 in this guide.
I'll convert the unit tests and storybooks in follow-up PRs as I ran into some issues there and this PR is big enough already.
The only real obstacle I ran into was getting
DayPickerSingleDateControllerfromreact-datesto work properly because the types are inreact-datesbut we importreact-dates/lib/components/DayPickerSingleDateControllerfor performance reasons.Runtime changes were mostly minimal: I added some null checks here and there and I changed the
updatefunction inTimePickerto be a partial function as that's easier to type.Testing Instructions
#40754 added tests. You can also play around with the Publish popover in the post sidebar to check that there is nothing off.
I'm a total noob at this so go easy on me and let me know if I missed anything! 😅 🙏