BoxControl: stop using UnitControl's deprecated unit prop
#39511
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
added
[Type] Enhancement
ciampo
requested review from
aaronrobertshaw,
andrewserong and
mirka
and removed request for
ajitbohra
|
Size Change: +59 B (0%) Total Size: 1.16 MB
ℹ️ View Unchanged
|
andrewserong
approved these changes
Mar 17, 2022
Contributor
andrewserong
left a comment
andrewserong
left a comment
There was a problem hiding this comment.
This is testing nicely for me in the editor (e.g. padding controls on the group block, and for axial controls, padding controls on the button block) and in Storybook. It appears to behave exactly the same as on trunk as far as I can tell! 👍
The concatenated unit value appears to be present on trunk for 0 based values (e.g. 0px) so that doesn't appear to be different here, either.
LGTM! Since there's a fair bit going on in this component, it might be worth getting a second pair of eyes before landing the PR, just as a confidence check 🙂
Contributor
Author
Couldn't agree more. I actually spent more than I wanted on this refactor exactly because I didn't know the component very well and was afraid on introducing a regression. Let's wait for a second pair of eyes to review this |
ciampo
commented
Mar 17, 2022
| isLast={ last === side } | ||
| isOnly={ only === side } | ||
| value={ values[ side ] } | ||
| unit={ |
Contributor
Author
There was a problem hiding this comment.
For ease of review: the unit prop here was removed, and instead the parsing and potentially overriding of the unit is applied to the value prop
ciampo
commented
Mar 17, 2022
| isLast={ last === side } | ||
| isOnly={ only === side } | ||
| value={ side === 'vertical' ? values.top : values.left } | ||
| unit={ |
Contributor
Author
There was a problem hiding this comment.
For ease of review: the unit prop here was removed, and instead the parsing and potentially overriding of the unit is applied to the value prop
ciampo
commented
Mar 17, 2022
|
|
||
| // Set meaningful unit selection if no allValue and user has previously | ||
| // selected units without assigning values while controls were unlinked. | ||
| const allUnitFallback = ! allValue |
Contributor
Author
There was a problem hiding this comment.
For ease of review: the unit prop below was removed. As a consequence, the allUnitFallback variable was also removed. The logic used for computing allUnitFallback has been moved and "embedded" in the getAllValue utility function
mirka
approved these changes
Mar 17, 2022
Member
mirka
left a comment
mirka
left a comment
There was a problem hiding this comment.
Changes make sense to me, and the inline comments were helpful. Thanks!
Comment on lines
+78
to
+81
| const allParsedQuantities = parsedQuantitiesAndUnits.map( | ||
| ( value ) => value[ 0 ] ?? '' | ||
| ); | ||
| const allUnits = parsedQuantitiesAndUnits.map( ( value ) => value[ 1 ] ); | ||
| const allParsedUnits = parsedQuantitiesAndUnits.map( |
jostnes
pushed a commit
to jostnes/gutenberg
that referenced
this pull request
Mar 23, 2022
…Press#39511) * Stop using `unit` prop in `AllInputControl` * Stop using `unit` prop in `AxialInputControls` * Stop using `unit` prop in `InputControls` * CHANGELOG
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
BoxControlcomponent 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