Spacer block: stop using UnitControl's deprecated unit prop
#39513
+10
−15
Conversation
This file contains hidden or 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
ciampo
requested review from
mirka and
stacimc
and removed request for
ajitbohra
ciampo
added
[Type] Code Quality
|
Size Change: +20 B (0%) Total Size: 1.21 MB
ℹ️ View Unchanged
|
Contributor
andrewserong
left a comment
andrewserong
left a comment
There was a problem hiding this comment.
Thanks @ciampo! This one appears to be testing the same as on trunk for me, but I did notice that the unit resets when emptying the value (e.g. set to a rem value and hit backspace, it'll revert to px). This is the same as trunk, but it did make me wonder that while removing / deprecating the unit prop from UnitControl, will we make it impossible to retain the currently selected unit when the quantity value is undefined in a controlled component?
There's an open issue in #38872 to look at the problem of deleting values resetting the units to px, and I wasn't too sure if this is something for us solve in the consumers of the UnitControl component, or if we should look at it within the component itself 🤔
Contributor
Author
|
Thank you for reviewing, @andrewserong ! I pushed an update to remove the
I believe that's something that is potentially caused by the |
ciampo
force-pushed
the
refactor/spacer-block-unit-prop
branch
from
f5c93e7 to
65374d7
Compare
Contributor
andrewserong
left a comment
andrewserong
left a comment
There was a problem hiding this comment.
Thanks for following up @ciampo! Removing the temporaryInput is working nicely for me in testing 👍
Just left a comment about preserving the isResetValueOnUnitChange setting to make sure that switching between px and em / rem units still feel smooth(ish). It'll probably feel a bit nicer once #39531 lands and clearing the input field doesn't automatically switch back to px.
What do you think?
Contributor
aaronrobertshaw
left a comment
aaronrobertshaw
left a comment
There was a problem hiding this comment.
This is testing pretty well for me 👍
I'd like to second the suggestion that we restore the isResetValueOnUnitChange as it's definitely the better user experience.
While testing I encountered another area we might be able to improve via a separate follow-up. At present, the UnitControl in this Spacer controls sets a minimum value of 0 whereas the ResizableBox components (both horizontal and vertical) get different minimum values (10px and 1px).
✅ Fixture tests pass
✅ Changing unit works
✅ Changing via typing works
✅ Changing via up/down arrow keys works
✅ Changing using modifier keys works
✅ Changing via mouse dragging works
✅ Changing spacer height via resizable box drag handles works
❓ The resizable box has different min/max values to the unit control
❓ Should the native controls also be updated to remove the deprecated unit prop?
ciampo
force-pushed
the
refactor/spacer-block-unit-prop
branch
from
65374d7 to
fd18928
Compare
Contributor
Author
|
Thank you both for your reviews! I've added the
Good catch, I opened #39577 with a potential fix
Unfortunately we can't update native controls until the native |
mirka
approved these changes
Mar 18, 2022
Member
mirka
left a comment
mirka
left a comment
There was a problem hiding this comment.
Looks good, very straightforward! 🚢
| }; | ||
|
|
||
| const inputValue = temporaryInput !== null ? temporaryInput : value; | ||
| const computedValue = useMemo( () => { |
Member
There was a problem hiding this comment.
Non-blocking: Just a hunch, but to me parseQuantityAndUnitFromRawValue() does not seem computationally taxing enough to offset the overhead of a useMemo. Not unless we have to loop through hundreds or maybe thousands of allowed units. (Not saying this warrants a perf benchmark, but just throwing it out there in case you were on the fence 😄)
Contributor
Author
There was a problem hiding this comment.
Good point, it does feel like a bit of an earlier optimisation — I went ahead and removed it in 1c22f97
I'll wait for CI to ✅ and merge
jostnes
pushed a commit
to jostnes/gutenberg
that referenced
this pull request
Mar 23, 2022
…rdPress#39513) * Memoize * Remove `temporaryInput` * Add `isResetValueOnUnitChange` prop back * Do not memoize value computation
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?
Related to #39503 , this PR refactors the
Spacerblockheightcontrols to avoid using the deprecatedunitprop from theUnitControlcomponent.Why?
The
unitprop is marked as deprecated, the component's docs recommend passing the unit directly through thevaluepropHow?
Refactor the code to pass the unit directly through the
valuepropTesting Instructions
px.px