UnitControl component: Refactor utils to TypeScript #35138
Conversation
andrewserong
added
[Feature] UI Components
andrewserong
requested review from
ajitbohra and
chrisvanpatten
as code owners
|
CC @ciampo, I've started looking into refactoring the UnitControl component to TypeScript — I thought I'd try doing it in smaller PRs one file at a time to hopefully make it easier to review, but also since I don't have much TypeScript experience. Hopefully this will give me the chance to learn from any feedback before venturing too much deeper! 😄 |
|
Size Change: -2.22 kB (0%) Total Size: 1.06 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Thank you for your work on this, @andrewserong, it is much needed!
I've left a few comments inline, but overall it's looking good!
| legacyUnit, | ||
| units = ALL_CSS_UNITS | ||
| ) { | ||
| currentValue: number | string, |
There was a problem hiding this comment.
I would probably add a new type in types.ts:
type Value = number | string;And then replace it everywhere we currently intend to represent such a value (including in the WPUnitControlUnit and UseCustomUnitsProps types).
There was a problem hiding this comment.
I like that idea, it neatens things a fair bit and ensures consistency (previously I see I'd inconsistently used number | string and string | number). I've updated, but please let me know if I've missed anything.
| units: Array< WPUnitControlUnit > | false = ALL_CSS_UNITS | ||
| ): Array< WPUnitControlUnit > | false { |
There was a problem hiding this comment.
Using false as a valid value for units and also as a potential return value of the function feels a bit weird (I know the implementation was there before this PR, and in fact, it's thanks to using TypeScript that we can spot situations like this one).
I can see that the same pattern happens in the hasUnits function — this is probably something that we should tackle separately in a follow-up PR. WDYT?
There was a problem hiding this comment.
I thought the same, it does feel a bit weird in these functions. I think we'll still need to support theme.json providing false as a valid value to switch off units support, but there's likely a neater way of handling it than having these functions use it as an expected type.
this is probably something that we should tackle separately in a follow-up PR.
Agreed, it'll probably be easier to do as a follow-up as the implementation might need to change slightly.
|
|
||
| if ( isNaN( parsedValue ) || parsedValue === '' ) { | ||
| if ( ! Number.isFinite( parsedValue ) || parsedValue === '' ) { |
There was a problem hiding this comment.
We should consider this change carefully, since it introduces a runtime change — isNaN doesn't always return the same results as ! Number.isFinite
There was a problem hiding this comment.
Thanks for pointing this out, I meant to leave a specific comment for it. With the switch to TypeScript, using isNaN here generated an error:
This seems to be that in TS, isNaN expects to be called with a variable of type number, but our parsedValue is either a number or a string. At this point in getValidParsedUnit the parsedValue variable must be either a number or an empty string (the results of calling parseUnit). So, I think Number.isFinite is a more appropriate check than isNaN anyway, at least from my mental model of what this function should be doing:
- Check that the number is not
NaN - Check that the number is a valid value that we can use (e.g. not
Infinityor-Infinity) and not an arbitrary string
If it doesn't meet the above criteria then we fall back to the fallbackValue. One difference in using Number.isFinite is that the value won't be coerced to a number before checking to see if it's finite. I think this is okay, because the value has already been coerced in parseUnit so at this point it should be a real (finite) value in order for us to consider it valid.
Please let me know if you think that's not right, though! Happy to change the logic here, I think it's the main place where I had to introduce a runtime change, but 🤞 it isn't an unexpected one in usage.
It could be that we might want to tighten up the Value type a bit further, too, given that in some places it'll only be number or the string literal '' — I was a bit cautious about doing that, as we'd then have two different Value types floating around in the file, which could get confusing? But it also might make it a bit clearer 🤔
There was a problem hiding this comment.
Please let me know if you think that's not right, though! Happy to change the logic here, I think it's the main place where I had to introduce a runtime change, but 🤞 it isn't an unexpected one in usage.
Thank you for the explanation! With this context (and the extra reassurance given by the passing unit tests), I think we can leave your new implementation untouched.
I would maybe only add a comment with a short version of the explanation you just gave in your previous message.
It could be that we might want to tighten up the Value type a bit further, too, given that in some places it'll only be number or the string literal '' — I was a bit cautious about doing that, as we'd then have two different Value types floating around in the file, which could get confusing? But it also might make it a bit clearer 🤔
I had the same thought and came to the same conclusion
|
Thanks for the detailed feedback @ciampo, much appreciated! I think I've either answered or implemented all the feedback and questions, but do let me know if there are any other changes you'd like to see 🙂 |
| */ | ||
| export function hasUnits( units ) { | ||
| return ! isEmpty( units ) && units !== false; | ||
| export function hasUnits( units: Array< WPUnitControlUnit > | false ): boolean { |
There was a problem hiding this comment.
We could also extract Array< WPUnitControlUnit > | false type to types.ts, to make this pattern more clear. Maybe something like
type WPUnitControlUnitList = Array< WPUnitControlUnit > | false;This would also help us when potentially working on #35138 (comment)
|
|
||
| if ( isNaN( parsedValue ) || parsedValue === '' ) { | ||
| if ( ! Number.isFinite( parsedValue ) || parsedValue === '' ) { |
There was a problem hiding this comment.
Please let me know if you think that's not right, though! Happy to change the logic here, I think it's the main place where I had to introduce a runtime change, but 🤞 it isn't an unexpected one in usage.
Thank you for the explanation! With this context (and the extra reassurance given by the passing unit tests), I think we can leave your new implementation untouched.
I would maybe only add a comment with a short version of the explanation you just gave in your previous message.
It could be that we might want to tighten up the Value type a bit further, too, given that in some places it'll only be number or the string literal '' — I was a bit cautious about doing that, as we'd then have two different Value types floating around in the file, which could get confusing? But it also might make it a bit clearer 🤔
I had the same thought and came to the same conclusion
There was a problem hiding this comment.
Actually, to speed this up, I can already approve this PR, since the two actionable pieces of feedback that I left are quite trivial and wouldn't change the runtime code. Feel free to address them and merge! 🚀
I can see two follow-ups from here:
- Fully type UnitControl. If you're in search of good examples on how to do this, ToolsPanel component: refactor to typescript #34028 may be of help
- Investigate how to potentially improve the types around units as discussed in UnitControl component: Refactor utils to TypeScript #35138 (comment)
I'm happy to help with both items, in case you have the time to work on them!
|
Thanks @ciampo! I've added in the contextual comment for the I did add in another small runtime change to I've tested the change in a couple of different themes (with and without Happy to continue work in follow-ups (I'll look at fully typing the rest of the UnitControl component later on this week), but let me know if there are any issues and I can do other follow-ups too. |
andrewserong
deleted the
update/unit-control-component-to-typescript
branch
|
Thank you! In the context of refactoring to the We could make these changes in the follow-up issue/PR that investigates how to improve the |
|
Thanks again @ciampo!
Yes, let's look at it in that context in the follow-up. Hopefully it should be a little clearer / cleaner when we're just looking at that one thing in isolation. I thought I'd start on typing the JSX -> TSX first later on this week, and then come back to the |
Sounds good to me! Feel free to open a draft issue and ping me there if you want early feedback too :) |
Description
Following on from discussion in #34768 (review), this PR refactors the UnitControl component's
utilsfile and set of functions to TypeScript and adds a couple of types. To avoid creating a large PR that might make reviewing difficult, I thought we could look at refactoring this component one file at a time, with this one just focusing on the utils for the moment.It looks like I could safely remove the lodash dependency from the file, too. I've also updated the documentation for some of the attributes where there were some slight inaccuracies.
Something I noticed while working on the file is that often the list of
unitscould have a value set tofalse(there were a few places where this is expected). I believe this is to support eitheruseCustomUnitsreturning the false value to switch off custom units, or possibly atheme.jsonfile explicitly setting units tofalse. I think I've managed to get this fairly consistent, but I'm very open to feedback if folks think there's a better way to deal with it.How has this been tested?
Ensure tests pass
Ensure the UnitControl component works correctly
Via Storybook, run
npm run storybook:devand then visit http://localhost:50240/?path=/story/components-unitcontrol--defaultOr, in the editor, go to edit a block that uses padding and adjust the controls (the Cover block is a good one to try)
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'ssettings.spacing.unitsattribute to a reduced list of unit strings (e.g.[ 'px', 'em', 'rem' ]to ensure it works as expected.Screenshots
Types of changes
Code quality, non-breaking change.
Checklist:
*.native.jsfiles for terms that need renaming or removal).