Spacer block: use same min value resizable box and controls, remove max limit
#39577
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] Enhancement
ciampo
requested review from
mirka,
jasmussen,
andrewserong,
aaronrobertshaw and
stacimc
ciampo
commented
Mar 18, 2022
|
Size Change: -11 B (0%) Total Size: 1.21 MB
ℹ️ View Unchanged
|
In the light of the above, I decided to have a separate set of min/max values for the spacer in horizontal and vertical configurations. As of now, I've chosen the same values that were already used previously:
I also realised that the solution that I'm proposing is not currently a good one because, while we can assume that these quantities will always refer to My proposed solution is:
What do folks think? |
|
Thanks for following up!
A minimum of
That's a good question. I'm not sure if there was a particular decision behind the
|
Agreed!
I don't know the answer to this question. @jasmussen (and @stacimc who worked on this block), do you know of any reasons why we wouldn't use the same min/max for vertical and horizontal spacers?
I see the utility of a max value when dragging, although I see 2 aspects that need to be clarified:
|
A few pieces to this one:
Not strong opinions, the above, and ultimately what works well works well, so feel free to go with something! Even if it means unifying on a single set of values across hoz and vert. |
This is a fair point, although I think it could be still ok because the block could be selected also via the block list (and tweaked via the controls in the sidebar). Although we should probably explore a way to make "zero-sized" blocks easier to selected in the canvas.
The problem with having any other values than
What you say sounds sensible, although it means that we won't be able to "sync" Overall, it looks like it's almost impossible to make changes to how the Otherwise I'm also happy to close this PR and leave the block as-is for now. |
|
Seems fine to move ahead in that direction, zero and no max, if that's what's needed.
Now that I think of it, I think we do have a pseudo element there to enable zero height blocks being selected: |
ciampo
force-pushed
the
fix/spacer-block-align-min-values
branch
from
15a81af to
6ae3da6
Compare
|
Alright, I went ahead and pushed some changes:
This is the resulting experience: spacer-block.mp4Let me know how it feels, happy to tweak further! |
|
Seems okay to try. |
|
Alright, let's wait to see what other folks think too and potentially merge — thank you @jasmussen ! |
ciampo
changed the title
min value resizable box and controls, remove max limit
andrewserong
approved these changes
Mar 22, 2022
There was a problem hiding this comment.
Thanks for the follow-up @ciampo and the feedback @jasmussen! This is testing nicely for me, and fits my personal preferences for using the spacer in both horizontal and vertical arrangements. Particularly now that #39531 has been merged, it feels easier to use with smaller values (like 0.5em) now 👍
Looks good to try, to me! 🎉
|
I've opened up a separate PR (#39623) to look at ensuring that the tooltip text for the ResizableBox stays on a single line (I noticed it overflowed at narrower widths when the Spacer is running in a horizontal configuration). |
jostnes
pushed a commit
to jostnes/gutenberg
that referenced
this pull request
Mar 23, 2022
… `max` limit (WordPress#39577) * Spacer block: update min size from 1 to 10 * Spacer: apply min/max sizes to ResizableSpacer * Spacer block: apply min/max sizes to controls * Separate width/height constraints for horizontal or vertical configurations * Set `min` to `0` * Remove `max` value * Unify vertical/horizontal min sizes back into one variable
This was referenced Apr 5, 2022
12 tasks
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?
This PR tweaks the interactive resizable box and the sidebar controls used to tweaks the Spacer block. With the changes in this PR, both the resizable box (used to resize the spacer by dragging its handle) and the sidebar controls have:
minvalue of0maxvalueThis is true for both the horizontal and the vertical orientations.
Why?
As flagged by @aaronrobertshaw in #39513 (review), currently the controls in the sidebar and the interactive resizable box operate with different minimum and maximum allowed values.
How?
The main reason for adopting a minimum value of
0and not having a maximum value is that it would be almost impossible to have a numeric min/max value that would translate fairly to all possible units available throughUnitControlTesting Instructions
Screenshots or screencast
spacer-block.mp4