UnitControl component: Refactor JSX components to TypeScript #35281
Conversation
andrewserong
added
[Feature] UI Components
|
Size Change: +367 B (0%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Thank you for the early ping, @andrewserong !
I've replied to your initial comment with hopefully a good solution.
Next up, I would:
- add the
unit-controlfolder totsconfig.json - rename
index.jsandindex.native.jstoindex.tsxandindex.native.tsx - Look at the imports of each TypeScript file in the
unit-controlfolder. If they are written in JS, add a// @ts-nocheckat the top of each imported file (this is to prevent TypeScript from type-checking the local (in the same package) dependencies ofunit-control, since we want to focus only on this folder. - Fix the rest of the TypeScript errors, until running
npm run devworks fine (and tests still pass)
As always, don't hesitate to ping me! I'm also happy to do some pairing on this, if you prefer.
andrewserong
changed the title
andrewserong
requested review from
ajitbohra and
chrisvanpatten
as code owners
|
@ciampo thanks again for the feedback, the discussion back and forth on the PR works nicely for me! Today, I've:
For the types, I needed to do a little bit of wrangling with the value type between places where it should be treated as a string (E.g. in the input component) and where it can happily be a number (e.g. passed to onChange for whatever is consuming the UnitControl). I think the expectation of a consumer would be that you could get a real number from the component, but that internally it'd be consistent with how the input control works, where the value is a string. I'm not sure if I've gotten the consistency quite right here, so very happy for feedback, of course! I've added in a few comments in Github where I made some decisions, to hopefully make it a bit clearer where behaviour has needed to change. I'm sure I've likely missed some details, so I'm keen to hear what you think. There's no rush on reviewing, though, as I've also got plenty else to go on with this week in case you're busy 🙂 |
There was a problem hiding this comment.
I've left replies to your comments — this is definitely moving in the right direction!
t doesn't look like we've done any of the other components' native components JS -> TSX yet, so I was wondering if it'd be better to leave that to another PR or time? (I haven't done much with React Native yet, either)
Me neither. Let's leave it as-is for now, and maybe involve someone who knows a bit more about React Native and how it works in Gutenberg before merging?
Looked at the imports of each TypeScript file in the unit-control folder — it looks like they're mostly being imported from TS files or files that already have the // @ts-nocheck, but I only looked quickly since I'm near the end of the day, so I might have missed something there
Looks like you're correct — that's good news!
For the types, I needed to do a little bit of wrangling with the value type between places where it should be treated as a string (E.g. in the input component) and where it can happily be a number (e.g. passed to onChange for whatever is consuming the UnitControl). I think the expectation of a consumer would be that you could get a real number from the component, but that internally it'd be consistent with how the input control works, where the value is a string. I'm not sure if I've gotten the consistency quite right here, so very happy for feedback, of course!
Yeah, that looks messy and it's definitely something that I'd like to improve in a follow-up PR. For now, let's focus on getting the best type definitions that we can while introducing the least amount ot runtime changes possible.
There's no rush on reviewing, though, as I've also got plenty else to go on with this week in case you're busy 🙂
Same for me. I may be busy with other work in the next days but I'll try to do my best to follow any progress!
|
Thanks for the review and the feedback! I've been sucked into a vortex of other tasks, but hope to get back to implementing the changes later on this week 🙂 |
|
Thanks again @ciampo (and for the great suggestions)! I've worked through the feedback and committed changes (and left a couple of comments where I haven't yet made further changes). I'll be switching back to some other tasks again for the rest of my week, so not sure if I'll get time to do more tweaking before the weekend. More than happy to pick this up again on Monday, though 🙂 |
|
This PR is already in a good place, I think we these latest changes we should be good to merge it! I left a couple more comments (mostly related to JSDocs) and replied to the opened conversations. Of course, no rush about working on this. I am also quite busy, so I may be a bit late with my reviews too. |
andrewserong
force-pushed
the
update/unit-control-component-refactor-to-typescript
branch
from
5febeeb
to
71f7bf6
Compare
|
Thanks again for the review @ciampo, I've implemented nearly all of the feedback, but left a couple of comments where I intentionally left things out or wasn't quite sure which way to go. I've given this a rebase, too, to make sure it's still current, and it still seems to be working well for me in testing 🤞 Let me know if I've missed anything, and no rush at all on following up 🙂 |
|
Thank you @andrewserong! I replied and left a couple more comments, but at this point the work that is left is mostly about documentation. I think I should be able to approve this PR during the next round of feedback 🤞 |
|
Thanks @ciampo! I've implemented this round of documentation feedback, let me know if you'd like any further tweaks, I'm happy to keep refining. Also, do feel free to commit any minor ones if there are little changes you'd like to get in quickly. Cheers! |
…ute of the Select element Co-authored-by: Marco Ciampini <marco.ciampo@gmail.com>
Co-authored-by: Marco Ciampini <marco.ciampo@gmail.com>
Co-authored-by: Marco Ciampini <marco.ciampo@gmail.com>
Co-authored-by: Marco Ciampini <marco.ciampo@gmail.com>
Co-authored-by: Marco Ciampini <marco.ciampo@gmail.com>
…Script style type description
ciampo
force-pushed
the
update/unit-control-component-refactor-to-typescript
branch
from
d7d3d16
to
6ec8dfa
Compare
There was a problem hiding this comment.
Thank you @andrewserong! This has been a great, great effort from your side, and it's very appreciated. I feel like the UnitControl component has become much more robust with the latest changes that you made.
I've pushed one small commit and rebased on top of latest trunk. I also left one last item of feedback, but it's small change which doesn't prevent me from approving this PR 🚀
Feel free to merge after taking care of it!
In case you had capacity to keep working on this component, the next steps I can think of would be:
- Refactoring the internal file names and folder structure (as advised in the guildelines)
- Connecting the component to the Context System (should be a small change which I'm also happy to take care of with you as a reviewer)
- Look a bit more into a couple of types that we had to add to the component. In particular:
- The
WPUnitControlUnitListtype, which is currently either an array orfalse(as discussed here) - All of the
string | undefinedtypes associated to units (it would be great if we could simplify that a bit)
- The
What do you think?
| /** | ||
| * Current value. To set a unit, provide a unit with a value through the `value` prop. | ||
| */ | ||
| value: string; |
There was a problem hiding this comment.
This should probably be Value, right?
In that case, we should also update the type in the README to be number | string.
Finally, I'm not sure I completely get the meaning of the comment "To set a unit, provide a unit with a value through the value prop." — maybe we should re-write it to clarify it?
There was a problem hiding this comment.
Good catch, yes, this should have been Value. I've updated the README and reworked the description to be clearer.
andrewserong
deleted the
update/unit-control-component-refactor-to-typescript
branch
|
Excellent, thank you @ciampo, I've implemented that last suggestion of switching to the
Those next steps sound good to me, too. I have a few other things to switch back to, but if there isn't too much urgency on these tasks, I'm happy to chip away at them over the coming weeks. (In case anyone else happens to read this and wants to pick any of these up sooner, feel free to, of course!)
Just to double-check for this one, among other changes, this would mean moving the |
Sounds great!
I would argue that sub-component folder structures should only be used for components that actually exported. Since, in the case of This is the folder structure that I'd have in mind: To get there, the main changes would be to:
For the split between hook and component, you can look at other components in this package as an example (and of course feel free to ping me early on in your PR if you need advice) |
Description
Follows on from #35138
This PR refactors the main JSX components of the UnitControl component to use TypeScript, and adds some additional types.
How has this been tested?
Manually in the editor (e.g. add a Cover block to a post, and try adjusting the padding and minimum height).
Running tests:
In Storybook:
Ensure that a theme's theme.json units settings overrides the available units
With TT1-Blocks applied, you should see that the list of available units does not include %, but you can also manually edit the theme.json file's
settings.spacing.unitsattribute to a reduced list of unit strings (e.g.[ 'px', 'em', 'rem' ]to ensure it works as expected.Screenshots
Types of changes
Checklist:
*.native.jsfiles for terms that need renaming or removal).