Cover: add margin block support #33835
Conversation
|
Size Change: -77 B (0%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
|
Depending on the layout (and, presumably, the theme) margins may have no effect, or appear difference in the editor. Here the cover block (with the red background) is center aligned with top and left padding.
On the frontend however, right and left margin values are overwritten with
For me, the user experience takes experimentation before getting used to. I've been trying to think of some ways to make things appear more consistent, and also wondering if having margin supports on cover blocks addresses any design needs. When placed in columns, adjusting top/left margins on the cover block allows such tremendously stunning layouts like this 😆
Turning on margin supports and let the user work it outIt's not a difficult feat to work out what's possible given the limitations of theme and/or layout. After a short time of experimentation, I think the user should be able to work out what's going on. It's where margin changes are expected to behave similar to padding changes, e.g., that the block increases its distance from surrounding HTML at a centre origin, where I think folks might be scratching their heads. Specifying vertical margin only on the cover blockVertical margins do behave consistently as far as I can tell in most themes and layouts. One option is to enable vertical axis margins. So, in the block.json: "margin": [ "top", "bottom" ]for the following effect. cover-margin-top-bottom-only.mp4Relying on parent blocks for layoutIt's also possible to adjust padding spacing when the cover block is a child of a group block. group-with-spacing-support-cover.mp4#33909 introduces margin block support for the group block, which could achieve the same affect. Or there might be a better solution waiting in gap support: #32571 Being able to work directly on the cover block, rather than the parent is more ideal however. It may not occur to everybody to nest blocks in order to achieve certain spacing configurations. What do folks think about all this? I'm documenting what I'm finding, and probably over-analysing it! 🙇 |
ramonjd
force-pushed
the
add/cover-block-margin-support
branch
from
f7857ce
to
c0b422f
Compare
ramonjd
added
[Block] Cover
ramonjd
force-pushed
the
add/cover-block-margin-support
branch
from
c0b422f
to
8e3c0bb
Compare
ramonjd
requested review from
stacimc,
aaronrobertshaw,
andrewserong and
glendaviesnz
ramonjd
requested review from
ajitbohra,
spacedmonkey and
TimothyBJacobs
as code owners
ramonjd
force-pushed
the
add/cover-block-margin-support
branch
from
8e3c0bb
to
dedc0f5
Compare
ramonjd
added
the
[Feature] Design Tools
ramonjd
force-pushed
the
add/cover-block-margin-support
branch
2 times, most recently
from
1940c86
to
347136d
Compare
|
This tests as you have outlined for me. The basics are there, but as you note there are some complications with margins that may confuse users, just to help with your over thinking, even just top and bottom margins still has margin collapsing issues, ie a bottom margin of 20px on one block and top margin of 20px on another becomes a gap of 20px:
I can see why there is some reluctance to make margin settings widely available, but I also wonder if isn't so complicated that users won't be able to tinker with it, and maybe get the effect they were looking for, and if not will just toggle it off again? |
ramonjd
force-pushed
the
add/cover-block-margin-support
branch
from
347136d
to
59eb408
Compare
|
Thanks for testing this @glendaviesnz !
Yeah, that sounds reasonable. I think margin control would have some uses in certain designs, and we probably shouldn't make it default. We could also start with axial padding if folks feel strongly about it. While testing I think I found a bug however (unrelated to this PR) #34766 |
ramonjd
force-pushed
the
add/cover-block-margin-support
branch
from
c7eab9d
to
334816b
Compare
ramonjd
force-pushed
the
add/cover-block-margin-support
branch
from
334816b
to
e170ad3
Compare
+1! This seems like a great example of a non-default control to me. It's maybe a little more niche of a use case, and it may require some user experimentation or trial and error -- but that feels acceptable for an 'advanced' control, and could enable some really interesting and creative layouts. Personally I think it's really exciting to have a way to add controls like this in without cluttering the default panel/giving unwarranted visual priority. The issue you brought up with left and right margins overridden by the layout was the only thing that stood out to me while playing around with this. I'm inclined to think of that as a separate issue, since I believe it similarly affects the margin block support in many other blocks 🤔 |
…vel. - Activating margin block support in experimental-default-theme.json so that it's turned on in development by default. - Removing BoxControlVisualizer for padding to avoid incongruity, that is, that it's on for padding and not for margin - To add a BoxControlVisualizer for margin we would have to introduce a wrapper around the entire block in edit.js for margin to display properly.
ramonjd
force-pushed
the
add/cover-block-margin-support
branch
from
e170ad3
to
c16b888
Compare
|
Thanks for testing @stacimc!
Yeah, sounds like we should constrain the effect to the vertical as mentioned and confirmed below. 👍 |
There was a problem hiding this comment.
Thanks for all the work on this @ramonjd 👍
I rebased this branch on top of master and I think it's been addressed now that #34725 has been merged.
Nothing in #34725 touched the layout support. It's in that block support, as you've linked in earlier comments, that the margin-left: auto !important and margin-right: auto !important styles are coming from.
The issue with left/right margins being overridden for top-level blocks persists for any theme that sets a "content size" or "wide size".
With that in mind, I've taken this for a test drive.
✅ Code changes look ok (further comment later)
✅ With exception of left/right margins for top-level blocks, this works in block editor and frontend
✅ Setting Cover block margins via theme.json worked as expected
✅ Margin controls display in site editor and for TT1 work with exception of left/right margin
I also had mixed results testing with other themes. TwentyTwentyTwo for example uses block gap support as well. So even though it wasn't overriding the left/right margins, when the cover block was within a Group block, its vertical margins were being overridden instead.
My vote would be that initially, the most we can opt into would be top and bottom margin support.
|
Thanks for testing @aaronrobertshaw
Thanks for confirming. Sounds like a reasonable way forward. I'll update.
🤦 I meant to reply to myself in #33835 (comment) by saying that I can close the issue I created here: #34766 (comment) I'll update the comment to avoid |
|
Updated margin support to
🥂 |
@youknowriad or @jasmussen Do you see any issues with removing the 🙇 |
|
One problem with having both margin and padding on the same block, as I also outlined on this other PR, is that their controls look virtually identical, as do their effect on the block cases like a block with a transparent background. The blue visualization is one tool to help handle this, the toolspanel with good defaults is another. For blocks like Cover, padding feels more important than margin, whereas for something like Heading, margin feels more important than padding. Is there an urgent need for margin on the Cover block? Not a strong opinion, but if we can, we might want to hold off on adding it for now, at least surfaced visually, and perhaps get a little farther on differentiating the controls and enhancing visualizations. #33221 alludes to a margin control potentially existing in the canvas. What do you think? |
|
Thanks for the quick feedback!!!
Not that I'm aware of. Now that we're constraining the margin to top and bottom sides in this PR, there is also the option of keeping
I truly don't anticipate riots in the comments if we put this one on ice until the concepts around the canvas UI evolves. 😄 I'm drawn to the ideas in #33221, specifically, visually linking the canvas to the controls. |
|
Closing this for now pending decisions on whether gap and other spacing supports are more appropriate. |
❗ Hey! Since this change displays the dimensions panel, we should remove the duplicate dimensions panel first. That means adding min-height support. We are doing this in #33970, on which this PR, therefore, is dependent.
Description
This PR:
block.jsonfor web.theme.jsonso that it's turned on in development by default.<BoxControlVisualizer />for padding to avoid incongruity, that is, that it's on for padding and not for margin.cover-block-margin.mp4
On the topic of BoxControlVisualizer
To add a
<BoxControlVisualizer />for margin we would have to introduce a wrapper around the entire block inedit.jsfor it to display.It appears the visualizer has been removed in other blocks for this reason.
Here's what it looks like when we do add it:
spacing-visualizer.mp4
How has this been tested?
Manually, in Chrome, Safari, Firefox and Edge browsers.
In a new post, create a cover block. Check that the dimensions panel is there and margin controls are disabled by default.
Select margin under the ToolsPanel and adjust the value.
Check that your changes are reflected on the published post.
Behold my ground-breaking results!
Rejoice!
Types of changes
Switching on margin block support, which is available since #30608
Checklist:
*.native.jsfiles for terms that need renaming or removal).