Cover block: stop using UnitControl's deprecated unit prop
#39522
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
ciampo
added
[Type] Code Quality
|
Size Change: +8 B (0%) Total Size: 1.16 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Thanks for these PRs @ciampo!
Is there also an opportunity in this PR to remove the temporaryInput internal state of the CoverHeightInput component? I'm struggling a bit to understand what it's doing currently, but apologies if I'm missing something obvious! Since it's slightly outside the scope of the change to deprecate the unit prop, we can always look at it separately, though.
For the moment, this change is testing well for me, I think we might possibly need to add unit to a dependency array, though.
I had a quick go at seeing if we can remove temporaryInput in the below diff, which wound up requiring adding unit to the dependency array.
Attempt to remove `temporaryInput` internal state
diff --git a/packages/block-library/src/cover/edit.js b/packages/block-library/src/cover/edit.js
index 285e232616..d49e660a8b 100644
--- a/packages/block-library/src/cover/edit.js
+++ b/packages/block-library/src/cover/edit.js
@@ -103,8 +103,6 @@ function CoverHeightInput( {
unit = 'px',
value = '',
} ) {
- const [ temporaryInput, setTemporaryInput ] = useState( null );
-
const instanceId = useInstanceId( UnitControl );
const inputId = `block-cover-height-input-${ instanceId }`;
const isPx = unit === 'px';
@@ -127,27 +125,15 @@ function CoverHeightInput( {
: undefined;
if ( isNaN( inputValue ) && inputValue !== undefined ) {
- setTemporaryInput( unprocessedValue );
return;
}
- setTemporaryInput( null );
onChange( inputValue );
};
- const handleOnBlur = () => {
- if ( temporaryInput !== null ) {
- setTemporaryInput( null );
- }
- };
-
const computedValue = useMemo( () => {
- const inputValue = temporaryInput !== null ? temporaryInput : value;
- const [ parsedQuantity ] = parseQuantityAndUnitFromRawValue(
- inputValue
- );
-
+ const [ parsedQuantity ] = parseQuantityAndUnitFromRawValue( value );
return [ parsedQuantity, unit ].join( '' );
- }, [ temporaryInput, value ] );
+ }, [ value, unit ] );
const min = isPx ? COVER_MIN_HEIGHT : 0;
@@ -157,7 +143,6 @@ function CoverHeightInput( {
id={ inputId }
isResetValueOnUnitChange
min={ min }
- onBlur={ handleOnBlur }
onChange={ handleOnChange }
onUnitChange={ onUnitChange }
style={ { maxWidth: 80 } }What do you think? I know these components are a bit of a rabbit hole, so I totally understand if you'd just like to get the simpler change landed first!
| ); | ||
|
|
||
| return [ parsedQuantity, unit ].join( '' ); | ||
| }, [ temporaryInput, value ] ); |
There was a problem hiding this comment.
Do we need to add unit to the dependency array here? In this change, I couldn't find any issues that came up from not including it, but if we were to refactor to remove the temporaryInput internal state, then I was wondering if it could be an issue.
There was a problem hiding this comment.
Do we need to add
unitto the dependency array here?
Absolutely, I definitely missed adding it on my end.
(Also, that's why I hope we can soon add the exhaustive-deps eslint rule to the repo).
ciampo
force-pushed
the
refactor/cover-block-unit-prop
branch
from
cc6df7a to
61ff1e0
Compare
|
Thank you for your review, @andrewserong !
I am not familiar with the code in blocks in general (as I tend to mostly work in the components package), but I took a look at the I went ahead and pushed changes to remove the |
andrewserong
added
the
[Block] Cover
andrewserong
approved these changes
Mar 18, 2022
There was a problem hiding this comment.
Thanks for including those changes @ciampo, much appreciated! It's so nice when we can simplify things a bit with these refactors 😀
✅ Manually inputing values works the same as trunk
✅ Switching units works correctly, and resets the value to the default for that unit
✅ Empty values are treated as empty
✅ Dragging the resizer switches to the px unit
LGTM! 🎉
There was a problem hiding this comment.
This is testing well on the web for me 👍
✅ Inputting values worked as expected
✅ Clearing values works
✅ Switching units correctly applies default for that unit
✅ Resizing via the resizable box's drag handles works and enforces px units.
My only question is should this PR remove the unit prop from the native controls as well?
|
Thank you both, @andrewserong and @aaronrobertshaw ! Regarding the native controls, the unit A couple of weeks ago I had a chat with @geriux and agreed that he would help aligning the native component with the web component — he should continue the work in this draft PR. Until that's done, we won't be able to update the native consumers of |
jostnes
pushed a commit
to jostnes/gutenberg
that referenced
this pull request
Mar 23, 2022
…dPress#39522) * Cover block: stop using `UnitControl` s deprecated `unit` prop * Add `unit` to the list of `computedValue` memo dependencies * Remove `temporaryValue`
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
Coverblockmin heightcontrols 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
min height(change unit, change value by typing, pressing arrows up/down, mouse dragging...) and make sure it behaves like in productionmin heightin a unit different thanpxpx