UnitControl: tidy up utils and types
#38987
Conversation
|
Size Change: +59 B (0%) Total Size: 1.15 MB
ℹ️ View Unchanged
|
ciampo
added
[Feature] Component System
ciampo
force-pushed
the
feat/unit-control-tidy-types
branch
from
57a4b7f
to
b5d6a28
Compare
ciampo
changed the title
UnitControl: tidy up utilities and typesUnitControl: tidy up utils and types
packages/block-editor/src/components/border-radius-control/test/utils.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
I love these improvements, it's like a masterclass in using empty arrays and undefined instead of false/null to simplify conditionals 💯 And as someone who's reading this code for the first time, the verbosity helps a lot. Beautiful work!
I mainly approached this as a code reader, so although it looks like we have decent test coverage around the runtime changes, I'll defer to others who've worked with UnitControl more intimately to take a better look at the behavioral aspects.
packages/block-editor/src/components/line-height-control/index.native.js
Outdated
Show resolved
Hide resolved
| @@ -20,7 +21,7 @@ export default function LineHeightControl( { value: lineHeight, onChange } ) { | |||
| step={ STEP } | |||
| value={ value } | |||
| onChange={ onChange } | |||
| units={ false } | |||
| units={ [] } | |||
There was a problem hiding this comment.
Based on the separate index, I'd assume this is the RN version of UnitControl, so we probably shouldn't make this change? Like, I see some runtime changes that would happen.
There was a problem hiding this comment.
You definitely make a good point, although this may lead to more issues, since the native UnitControl component shares the same utility functions of the web component.
Also, I think it would be overall be better to have the web and mobile counterpart share the same public APIs, since they also share the same README etc.
For now, I've gone ahead and removed all changes related to native components. That, in combination with the "more robust" checks that I've added back to the shared utils, should be good enough to avoid introducing regressions to the native components.
Having said that, I'm still not convinced that this is best solution. Ideally, we should update the native UnitControl's behaviour not to rely on units being false at all — although I'm not in a position to say what's the best solution here, since it looks like the native UnitControl behaves differently when false is passed instead of []. Tagging in @enejb and @lukewalczak who recently worked on this component for guidance.
|
Things are working well for me. I tested changing units, changing individual box control values. All looks as it should in the frontend and editors. I've tested the following: Components using UnitControl in the @wordpress/block-editor package work as expected:✅ BorderRadiusControl Components using UnitControl in the @wordpress/edit-site package work as expected:✅ Controls in the border panel and dimensions panel
|
There was a problem hiding this comment.
What a great PR, thanks for all your work here tidying up the component, I think this greatly improves the readability of the component and its utility functions! ✨
I haven't been able to find any actual issues with using the component in practice, but I've added some comments about where there are some runtime changes surrounding the expected value for units and the removal of some Array.isArray() checks. Most of the feedback is totally optional, of course, and sort of point more to the question of how careful should the component be about dealing with potential values that it's passed? Because it's such a heavily used component, I think I lean a bit toward a cautious approach of leaving some of those checks in, but I do appreciate it might be cleaner to remove some of them, too!
Also, just to echo Lena's feedback, I love the shift to undefined in most of the areas, and using it as a valid value (without accidentally rendering it as a string) — I think that fix probably explains why null might have been used in lots of places in the past?
The careful function renaming and added inline comments are really great, too, gives the component a much-needed polish. Kudos!
ciampo
force-pushed
the
feat/unit-control-tidy-types
branch
from
08625a7
to
51951b8
Compare
There was a problem hiding this comment.
Thank you all! I'm humbled by all of your thoughtful and detailed reviews 🙇
I've pushed changes and replied to all comments, trying to be as granular as possible with my commits to facilitate reviewing.
Most of the feedback is totally optional, of course, and sort of point more to the question of how careful should the component be about dealing with potential values that it's passed? Because it's such a heavily used component, I think I lean a bit toward a cautious approach of leaving some of those checks in, but I do appreciate it might be cleaner to remove some of them, too!
You definitely make a good point, and I agree with you on this one. Using TypeScript internally to UnitControl doesn't necessarily prevent the component from being used incorrectly in non-typed environments. And since there may already be a few instances out there, it's probably best to stay on the side of caution and re-introduce a few more checks.
packages/block-editor/src/components/border-radius-control/test/utils.js
Outdated
Show resolved
Hide resolved
| @@ -20,7 +21,7 @@ export default function LineHeightControl( { value: lineHeight, onChange } ) { | |||
| step={ STEP } | |||
| value={ value } | |||
| onChange={ onChange } | |||
| units={ false } | |||
| units={ [] } | |||
There was a problem hiding this comment.
You definitely make a good point, although this may lead to more issues, since the native UnitControl component shares the same utility functions of the web component.
Also, I think it would be overall be better to have the web and mobile counterpart share the same public APIs, since they also share the same README etc.
For now, I've gone ahead and removed all changes related to native components. That, in combination with the "more robust" checks that I've added back to the shared utils, should be good enough to avoid introducing regressions to the native components.
Having said that, I'm still not convinced that this is best solution. Ideally, we should update the native UnitControl's behaviour not to rely on units being false at all — although I'm not in a position to say what's the best solution here, since it looks like the native UnitControl behaves differently when false is passed instead of []. Tagging in @enejb and @lukewalczak who recently worked on this component for guidance.
ciampo
force-pushed
the
feat/unit-control-tidy-types
branch
from
51951b8
to
44b9c47
Compare
|
Nice work making all those updates @ciampo, I've replied and resolved all the feedback I gave (looks like you've addressed it all), and this PR is looking in great shape to me. I haven't had a chance to re-test it in detail yet, but happy to do that tomorrow. Also, looks like it needs a rebase now due to other changes in Thanks again for all the detailed work here! |
ciampo
force-pushed
the
feat/unit-control-tidy-types
branch
from
24077bd
to
49b645b
Compare
|
Update: I rebased this PR (and renamed a variable) to include changes from #39148. Regarding the React Native runtime changes, I've decided to remove those changes from this PR as they were causing a regression (see details below). I will tackle those changes separately, so that they don't delay this PR further (cc @geriux @hypest @enejb — I'll ping you once the new PR is ready). Since the rest of these changes have already been approved, I'll wait for CI to go green (both on this PR and on wordpress-mobile/gutenberg-mobile#4618) and then merge. |
Thanks @ciampo! |
Works for me @ciampo , thank you for that strategy! By the way, curious to ask, was that a regression you spotted by manual testing or some other means? Thanks! |
|
Forgot to add it to my previous comment! Basically, my changes were unintentionally hiding the part of the UI showing the unit (including the unit picker). Here's an example for the
In that new PR, we will need to investigate better the logic around checking the |
Description
While working on #38753 (specifically on the type of the
valueprop of theNumberControlcomponent), it became clear that we should first untangle a few components usingNumberControl— andUnitControlis one of them.This PR tries to refactor and tidy up some of the types (and consequently the logic) of the
UnitControl, in particular its many utility functions. With that in mind, this PR's main goal is to add a clear internal split between the numeric quantity and the unit parts that together compose the value held byUnitControl, all while introducing the least amount of runtime change possible and ideally no changes to the public API ofUnitControl(despite it being flagged as an experimental component).Changes overview
UnitControl, which internally goes through the component's parsing logic, is now referred to as "Raw Value"number(when defined)string(when defined)UnitControl(e.g14,'2.5rem','600%'...) is parsed by the internal logic and split into a "quantity" (of typenumber) and a "unit" (of typestring)defaultproperty of theWPUnitControlUnithas changed to be anumber, following the type of a parsed "quantity" (it was also already documented as such in the README)Valuehas been removed, and replaced by more explicitstringand/ornumbertypes where necessaryWPUnitControlUnitListhas been removed, and has been replaced byWPUnitControlUnit[](which means that those values can only be array, and can't befalseanymore)valueprop ofUnitControlwas previously typed as required, this PR changes it to be non-required (it was already being omitted or passed asnullin the repo, and advertised as such in the README anyway)UnitControlbehaviour has slightly changed around how a value of[]for theunitsprop is handled (see code changes)List of introduced runtime changes
WPUnitControlUnitListtype, when passing a list of units, allfalsevalues were replaced by[]UnitControl, changed values fromnulltoundefinednumber, the parsing logic internal toUnitControlnow returnsundefinedif a quantity can't be parsed (it previously returned''). As a consequence, the fallback to''has been moved to each consumer the parsing utility (if necessary).defaultvalue of all "base" units has changed from''toundefinedvaluecan now beundefined(and that it is already passed asnullorundefinedanyway across the repo), a few more checks and fallbacks have been added to theUnitControlcomponent and some of its utility functionsTesting Instructions
UnitControlwork as expected, both in Storybook and in the editor:FontSizePickerBoxControlFocalPointPickerToolsPanelUnitControlin the@wordpress/block-editorpackage work as expected:BorderRadiusControlLetterSpacingControlLineHeightControlBorderWidthEditGapEditUnitControlin the@wordpress/edit-sitepackage work as expected:Types of changes
Refactor
Checklist:
*.native.jsfiles for terms that need renaming or removal).