Group Blocks: Add margin support (top/bottom).

[ndiego]

Description

This PR simply adds margin support to Group blocks, specifically the Editor UI. Margin is actually already supported, just not the UI. With this PR, theme developers can opt-in to margin support either by using the settings.spacing.appearanceTools.true flag, or at the block level with settings.blocks.core/group.spacing.margin.true in theme.json.

Note that this PR restricts margin to top/bottom, which is in keeping with the approach currently applied to Columns blocks.

There are countless reasons why allowing margin control on Group block is extremely useful when designing in the Editor. Here is one such example: #36797 (comment).

As mentioned in #37300, which concerns adding margin support to Paragraph blocks, theme developers (myself included) are resorting to "magic classes" to zero out, or modify, margins on blocks. Allowing themes to opt-in to Group margin support will provide greater flexibility and decrease the reliance on custom CSS/classes in the theme's stylesheet.

How has this been tested?

1. Activate the latest version of the Twenty Twenty-Two theme.
2. Make sure that settings.spacing.appearanceTools.true is set in theme.json
3. Add a new page and add an image block and paragraph block and wrap in a group block.
4. Check that the Dimensions panel is visible and you can add the margin to the Dimension panel.
5. Check that you can then modify top and bottom margin on the group block.
6. Add a custom margin and make sure it is reflected in the Editor and on the Frontend.
7. Go back to the theme.json and add settings.blocks.core/group.spacing.margin.true.
8. Navigate to a paragraph block in the Editor and the margin UI in the Dimension panel should be gone.

Screenshots

Types of changes

New Feature

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code follows the accessibility standards.
• I've tested my changes with keyboard and screen readers.
• My code has proper inline documentation.
• I've included developer documentation if appropriate.
• I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).

[kjellr]

Hey @ndiego, I had a similar PR for this last week, but the group collectively decided to hold off on it for the time being. More details here: #37105

[ndiego]

@kjellr oops thanks for pointing this out. I must have missed that as I was only looking at open PRs before I added this. Still feel strongly that this is needed and added a comment over there.

[ndiego]

I would like us to revisit this PR. This PR fixes the Group component of #38629, address a workaround for #37811, and is consistent with how Columns blocks currently handle margin (only top/bottom). Furthermore, theme developers can opt out in theme.json. I understand the greater concern about margin not being that intuitive in a blockGap world and that it's similar to padding to novice users. However, allowing top/bottom margin provides FSE designers with greater flexibility and actually allows for greater adoption of blockGap since margin support can easily correct for edge-cases that would be extremely challenging for blockGap to ever address.

Either way, I think a line needs to be drawn in the sand before 6.0.

Either we add margin to "container" blocks like Group and Cover along with Columns, or we don't ever add margin control and margin should be removed from Columns and other blocks that support it. If the later is the case, then we need to explore a more robust way to address the concerns of the community. Currently it is way too easy for users to say "X block supports margin, why doesn't Y block support margin?"

Obviously I am in the camp of margin control 😉, but I recommend we have a good conversation here about the merits of both approaches and chart a path forward.

Sorry for the pings, but elevating this to those who have participated in similar discussions in the past.
@kjellr, @richtabor, @jasmussen, @aaroncampbell, @youknowriad, @apeatling, @bgardner

[MaggieCabrera]

+1 to margin control on group blocks. You can't do margin-top: auto using padding, it's something I've recently wanted to do while developing a theme

[cr0ybot]

I'm struggling to come up with any reasons the group block should not support top/bottom margins. The blockGap argument seems pretty hollow, since other blocks still have margin controls.
As any web developer can attest, sometimes you just need an arbitrary container div to add some extra spacing/margin somewhere, and not even allowing that in the editor is absurd. Strict top-down rules never work the way we wish they would, some creative freedom is always necessary.
And, come to think of it, @MaggieCabrera's suggestion about auto margins would actually solve an issue I'm having literally right now with a block theme!

[kjellr]

👋 I know there are countless development-centric reasons to have margin support here (and globally, frankly) in the editor. So I'd like to clarify/remind folks that the reason margin has been left out here is that the feature is likely to be confusing to users. As noted in other threads:

• We don't have the UI yet to communicate the difference between Margin and Padding to users. If we present both on the same block, many folks won't understand the difference.
• We don't yet have guard rails to prevent users from breaking their site's default content alignment when they implement margins. Users generally aren't familiar with the concept of using auto margin to center something horizontally, so if they set the left margin to 100px, and all of a sudden their block veers off to the left, they'll be confused.
• In general, adding a spacer that you can drag to resize is far easier for users to understand than abstractly entering a margin value.

The blocker here isn't a technical one, it's a design one. We just haven't figured out those issues, and it wasn't a design priority for the 5.9 release.

[ndiego]

Thanks for including the additional reasoning behind the decision @kjellr. While I respect the reasoning, I would like to push back lightly on a few points.

We don't yet have guard rails to prevent users from breaking their site's default content alignment when they implement margins. Users generally aren't familiar with the concept of using auto margin to center something horizontally, so if they set the left margin to 100px, and all of a sudden their block veers off to the left, they'll be confused.

This PR is only advocating for top/bottom margin, not left/right. This change is consistent to the margin implementation currently on Columns blocks.

In general, adding a spacer that you can drag to resize is far easier for users to understand than abstractly entering a margin value.

Spacer blocks are very useful, but they do not address the situation when you need to set margin-top: 0 to counter blockGap. blockGap is awesome, but there are scenarios when you need to disable it. This is a fairly significant design issue that is impacting the development of FSE themes which is solved effortlessly with the implementation of margin support.

Finally, given that margin support can very easily be disabled in theme.json, developers who are concerned about their users being confused about both padding and margin, can just disable it in their themes.

[jasmussen]

Kjell's comment perfectly echoes my sentiments on the matter, the blocker is a design one. Notably for block theme templates in the site editor, it's likely that the increased in-canvas visibility the spacer provides will make spacing customization more intuitive for most people.

Here in the post 5.9 phase, we could start exploring paths forward, the key being to seek out in-canvas solutions. So long as those explorations are not hindered, in the mean time, we could open up the margin support for a few blocks as a non-default addition to the tools panel. What do you think?

[mtias]

There's a few levels to this:

• Let's add margin support to Group, the tools panel UI handles this fairly well in not exposing things by default.
• We need to think about how spacer could be deconstructed into adjacent margins at render time. This needs some thought.
• We need a better way than margin-top 0 to reset block gap. Generally block gap should be targeting block displayed in succession but not applied to the first one. Can you include some descriptions of cases where resetting block gap is necessary? I can think of cases where a heading wants to reset block gap after itself, for example. But we can probably think about the semantics of headings as sections to do this automatically or opt-in by the block. It'd be good to gather many more examples.

[MaggieCabrera]

I bet many of the cases for block gap issues could be handled directly by handling some exceptions too. This would also be a good example of that case: #38675

[mtias]

Yeah, even exposing a blockGapBefore and blockGapAfter flags could make sense.

[ndiego]

Here in the post 5.9 phase, we could start exploring paths forward, the key being to seek out in-canvas solutions. So long as those explorations are not hindered, in the mean time, we could open up the margin support for a few blocks as a non-default addition to the tools panel. What do you think?

💯 If margin is implemented, I believe it should always be "hidden" in the tools panel. That hopefully will decrease the confusion for novice users while also enabling this feature for others. I also completely agree that continued exploration should not be hindered.

Let's add margin support to Group, the tools panel UI handles this fairly well in not exposing things by default.

Thank you for being open to this @mtias! 🙏

We need a better way than margin-top 0 to reset block gap.

💯 Agree. This is just a side benefit of margin support while a more robust solution is found.

Can you include some descriptions of cases where resetting block gap is necessary?

The easiest example is probably two full-width Cover blocks, one after another. Perhaps both have background color and you want them to butt up against one another. This is not currently possible since blockGap will separate them.

Another example is a Header template part with a colored background and you want that to butt up against a full-width Featured Image block. Again they will be separated by the global blockGap value.

Thank you all for this thoughtful discussion. 🙌

[ramonjd]

Can you include some descriptions of cases where resetting block gap is necessary?

Adding a similar example to @ndiego's above.

I also came across this limitation when trying to stack colored layout blocks (Group, Column) on top of each other while creating a theme.

When declaring a "blockGap" value in theme.json, it appears the only way to stack layout blocks on top of each other without any separating margin-top, especially when they have background colours, is to use a Columns block and set the margin-top to 0.

Video.MP4.480x346.mp4

We need a better way than margin-top 0 to reset block gap. Generally block gap should be targeting block displayed in succession but not applied to the first one.

This is probably an overly-simplistic view, however I’m wondering if we could separate the “blockGap” we use at the block level, for example, in the Social Icons block, from the layout implemention.

That way we'd have a control under the Layout panel in the sidebar that is explicitly bound to the layout's usage of blockGap, which is margin-top in disguise.

So for example, into layout via __experimentalLayout

		"__experimentalLayout": {
			"allowSwitching": false,
			"allowInheriting": false,
			"default": {
				"type": "flex"
			},
                        "blockSpacing": "24px"
		}

I'm not sure, but it just might help to avoid situations such as these: #35411 where the layout's margin-top specificity overrides a child's block margin-top.

[ndiego]

Does anyone have additional concerns, or can this be approved? Happy to provide more examples/explanation if needed.
@mtias @jasmussen

[jasmussen]

It's late for me and I'm not able to review and approve this PR today, though I can take a look tomorrow.

Otherwise for any other reviewers, some quick high level thoughts: it seems fine to add margin to the group block if it's a tools panel addition to the dimensions panel, which is off by default. If that works as intended, 👍 👍 for moving forward.

[jasmussen]

Giving tentative approval to this one based on the commentary and the fact that it tests well for me:

Even shown in that GIF the collapsing of margins can make it confusing why a low margin has no effect, but the cause of this has been discussed ad nauseum, and is fine so long as it remains a non-default control and we continue to explore better and additional controls!

(For those explorations, and simply due to margin collapsing, it seems to suggest we need to highlight not just the margin of the selected block, but indeed the margin of adjacent blocks too)

Thanks for your patience (and PR)!

[ndiego]

Thanks @jasmussen! 🙏

I completely agree with you that margin is confusing in the Editor, especially if you don't know that blockGap is implemented by adding margin-top to all blocks within a container. Future refinement and exploration is needed and I will be thinking long about this.

In the meantime though, enabling margin functionality on Group blocks opens the doors to so many interesting patterns and designs without the need for custom CSS. It's so exciting. Thank you to everyone for your input, understanding, and flexibility on this topic!

I'll give it until Monday for others to share any last-minute feedback, for or against, and then merge.

[richtabor]

I've given this PR a run-through as well, looks good to me. 👍

[Mamaduka]

Hi, @ndiego

I just noticed that Static Analysis is failing for merge commit.

You probably will need to generate updated docs for Groups block.

[ndiego]

@Mamaduka on it!

[mtias]

@ndiego thanks for persisting with this :)

[ndiego]

@mtias haha perhaps too persistent. Thank you for being open to this enhancement. 🙏 It appears to be well received: https://twitter.com/nickmdiego/status/1495791576194428931

[skorasaurus]

Hi @ndiego thanks for all of your work on this; could you update your initial PR to read settings.appearanceTools.true instead of settings.spacing.appearanceTools.true in case someone else comes along and tries to learn how to implement the margins?

:)