Block Spacing: Don't show UI when only one direction is supported #47523
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
|
Size Change: +32 B (0%) Total Size: 1.31 MB
ℹ️ View Unchanged
|
t-hamano
force-pushed
the
fix/block-gap-one-direction
branch
from
9f4a839 to
b51636c
Compare
t-hamano
added
[Type] Bug
t-hamano
requested review from
glendaviesnz,
andrewserong and
ramonjd
and removed request for
ellatrix
|
Thanks for opening this PR! I've just added some comments over on the linked issue (#47518 (comment)) as the axial controls for block spacing were built assuming that opting in to a single direction would not be supported. In short, my main question is, what's the use case for defining a single direction over using While the change in this PR fixes the UI issues, the output uses a fallback gap for the opposing direction (e.g.
So, in order to support passing in one direction, I think it would also involve updating the layout support output to support only outputting a particular Because it would involve changing things in a few places, I think it'd be good to better understand the use case for a single direction in Thanks again for all the digging in here @t-hamano! |
t-hamano
changed the title
t-hamano
force-pushed
the
fix/block-gap-one-direction
branch
from
b51636c to
6b5c17a
Compare
|
This PR was originally intended to correct the UI to work properly when supporting one direction. However, one direction support is not expected. Based on this discussion, I have made it so that when only one direction is supported, a console error is displayed instead of showing the UI. |
andrewserong
approved these changes
Feb 2, 2023
There was a problem hiding this comment.
Nice one, thanks for continuing on with this @t-hamano, and for the thorough consideration surrounding all of the potential values for blockGap in block.json!
This is testing well for me:
✅ Common UI is displayed when expected (true value, empty array, or sides prop set to empty array or false)
✅ Axial pair are available when using array containing both vertical and horizontal or sides prop containing the same
✅ Existing critical error for sides: true looks fine to me
✅ Warnings displayed correctly when only a single direction is set
LGTM! ✨
andrewserong
added
the
[Type] Code Quality
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.
Fixes #47518
What?
This PR
fixes a problem in which values are not reflected correctly in the controldoesnt't display the UI control and output a console warning message when the block supports only one direction insupports.spacing.blockGapproperty.Why?
The attribute stored by
blockGapholds only two properties, top and left. However, components such asBoxControlthat pass this value must specify all side values. Therefore, property mapping is performed in the following part:(Deleted)
If only one direction is supported (i.e.,
splitOnAxisistrue),undefinedvalues may enter some of the properties in this part of the property mapping.This will cause the component receiving the value to behave in an unintended way.
@update: Based on this discussion, the UI itself should not be displayed as it is not intended to support only one direction.
How?
Remove properties with undefined values.This process could be accomplished with the cleanEmptyObject function alone, but this function depends on lodash. I used this approach because I know that the move away from lodash is being promoted by #17025.If blockGap does not support both axes, the UI is not displayed and a warning message is displayed in the browser console.
Testing Instructions
blockGap.block.json, change this support as follows and confirm that the expected results are obtained.A common UI for both axis sides should be displayed
"blockGap": [],"blockGap": true,"blockGap": { "sides": [] },"blockGap": { "sides": false },UI should not be displayed
Behavior is the same as before.
"blockGap": false,A single UI control for each axial pair should be displayed
Behavior is the same as before.
"blockGap": [ "vertical", "horizontal" ],"blockGap": [ "vertical", "horizontal", "other" ],"blockGap": { "sides": [ "vertical", "horizontal" ] },"blockGap": { "sides": [ "vertical", "horizontal", "other" ] },Critical Error
Behavior is the same as before.
"blockGap": { "sides": true },UI should not be displayed with console warning
This PR prevents unintended UI rendering in the following variations.
"blockGap": [ "vertical" ],"blockGap": [ "horizontal" ],"blockGap": [ "vertical", "other" ],"blockGap": { "sides": [ "vertical" ] },"blockGap": { "sides": [ "horizontal" ] },"blockGap": { "sides": [ "vertical", "other" ] },