Add visualizers for padding and margin for all blocks #40505
Merged
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
youknowriad
added
[Type] Enhancement
youknowriad
requested review from
mcsf,
mtias,
jasmussen,
aaronrobertshaw and
a team
|
@aaronrobertshaw I noticed that you've added a new visualizer for border, maybe we can use the same technique as this PR? How can I experience it? |
|
Size Change: -281 B (0%) Total Size: 1.23 MB
ℹ️ View Unchanged
|
|
Does this supersede #40253, or will the new |
Thanks for the ping. @kevin940726 is currently working on #40253 which reimagines the visualizer. I believe the plan was to support borders via that. I'm not sure we can call the new |
|
I actually didn't know about #40253 :) In principle, it's the same thing but I think "Popover" is already capable of handing what the new component is trying to do without the added code and complexity. (I do think there are opportunities to improve the internals of Popover) though. |
|
This is just delightful. The visualizers of paddings and margins both work great, but notably for margins it might finally unblock the addition of margin tools to more blocks since it finally properly visualizes how they work, and surface any margin-collapsing that might otherwise make margins not take. All shown in this GIF: This is all working pretty well as-is, though I encountered one issue. In this GIF I group two paragraphs, apply a bunch of padding, and while focus is still inside the padding input field, I press ⌘Z to undo, and it doesn't undo the padding change and it makes the padding flash:
Something similar but not quite the same happens when I press the explicit undo button in the toolbar. |
|
@jasmussen I haven't been able to reproduce neither of these issues 🤔 |
|
I can still reproduce after a fresh npm install. Try these steps exactly:
What do you see? |
|
@jasmussen I reproduced once, but I'm failing to reproduce now 😬 |
|
@jasmussen oh, I see now, It is actually a bug in trunk as well, it's independent of this PR. Basically when I try to undo a change of padding, it undo but then restores the old value right away. I think it's probably a bug in |
|
@jasmussen the fix for the padding change is here #40518 but I might need help from components folks to make sure the tests and everything is correct. I think we shouldn't block the work in the current PR though since the bug is already in trunk. |
|
Definitely agree, this one feels amazing. I'd be happy to give a green check. |
|
I love this is being accomplished with more code removed than added :) |
mcsf
approved these changes
Apr 21, 2022
There was a problem hiding this comment.
I like the approach, and this seems to be working well.
As far as implementation details go, I like the locality: the relationship between the hooks, BlockPopover and Popover is fairly intuitive, and it's clear what each piece is responsible for.
Bonus points for cutting down on unused components and converging. :)
Comment on lines
-747
to
-751
| <BoxControlVisualizer | ||
| values={ styleAttribute?.spacing?.padding } | ||
| showValues={ styleAttribute?.visualizers?.padding } | ||
| className="block-library-cover__padding-visualizer" | ||
| /> |
Co-authored-by: Miguel Fonseca <[email protected]>
|
Going to merge this as it's gets the feature in without new APIs or overhead but it doesn't mean we can't extract reusable components from this later if needed. |
This was referenced Jun 27, 2022
7 tasks
This was referenced Jul 27, 2022
10 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.
closes #36418
closes #40057
What?
This PR adds "visualizers" when editing padding/margin controls in blocks that support them.
Why?
The visualizers gives the user clarity of the impact its changes are having on the block.
How?
Previously we had
BoxControl.Visualizercomponents, it was used in the Cover block padding for instance, but it suffered two major issues:Note
I'm planning to use the BlockPopover (with cover argument) in #40376 to render the side remove buttons... So this PR is also a requirement for that.
Testing Instructions
1- Insert a block that supports padding or margin (group, cover...)
2- Tweak the value for these controls
3- Notice the visual effect (blue squares) when changing the value.
Screenshots or screencast
Screen.Recording.2022-04-21.at.10.15.46.AM.mov