Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add margin support to group block #37105

Closed
wants to merge 1 commit into from

Conversation

kjellr
Copy link
Contributor

@kjellr kjellr commented Dec 3, 2021

The group block currently supports padding, but not margin. This adds it in. The general reasoning for this is to bring the Group block on par with the spacing controls that used to exist for the Template Part block.

Here's a specific use case though: In Twenty Twenty-Two, we used to have some margin applied to the bottom of the header template part. This was necessary to ensure consistent content/title spacing across pages. When support for Margin was removed from the Template Part block in #36571, we had to move those settings over to a Group block instead. This worked fine, but the margin control does not appear in the editor since the block does not "officially" support margin. That looks like a bug, and it prevents users from clearing that margin if they'd like to.

Before After
Screen Shot 2021-12-03 at 10 50 40 AM Screen Shot 2021-12-03 at 10 49 57 AM

@kjellr kjellr added [Type] Enhancement A suggestion for improvement. [Block] Group Affects the Group Block labels Dec 3, 2021
@kjellr kjellr requested a review from ajitbohra as a code owner December 3, 2021 15:58
@kjellr kjellr self-assigned this Dec 3, 2021
@kjellr kjellr requested review from jffng and scruffian December 3, 2021 16:06
Copy link
Contributor

@MaggieCabrera MaggieCabrera left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many themes use this and I think the group block totally should have these controls. Unless there's an important reason why we shouldn't, I think we should ✅ this

@paaljoachim
Copy link
Contributor

@aaronrobertshaw and @andrewserong I think you might want to take a look at this PR.

@jasmussen
Copy link
Contributor

jasmussen commented Dec 3, 2021

The main concern is that the controls for managing margins look the same as those for padding, that's driven my rule-of-thumb-so-far to only have one at the time, at least until we can better visualize the two.

Is this primarily for patterns, and could you still accomplish what you need without the control? My instinct to not do this yet is not a strong one, but it also doesn't feel like the addition is an obvious win.

@kjellr
Copy link
Contributor Author

kjellr commented Dec 3, 2021

The main concern is that the controls for managing margins look the same as those for padding, that's driven my rule-of-thumb-so-far to only have one at the time, at least until we can better visualize the two.

Is this primarily for patterns, and could you still accomplish what you need without the control? My instinct to not do this yet is not a strong one, but it also doesn't feel like the addition is an obvious win.

It's for templates, not patterns in this case. One workaround would be to use a spacer block, but that still doesn't support the dynamic sizes we're using here.

Or we could use a second, empty Group block to wrap the first, and add some padding inside of it, but that seems like it would be even more confusing to users who want to change this setting.

In general, if the concern is that these two controls look similar I think we need to sort that out on the Gutenberg end.

@jasmussen
Copy link
Contributor

One of the reasons why I've defaulted to one or the other when it comes to padding vs. margin is that especially when they sit next to each other and without margin visualization like in #33221, it really isn't obvious what effect the margin control has on content, especially on large layouts. Even when the margin is used on its own, I actually find the spacer block to be clearer and a better user experience.

@kjellr
Copy link
Contributor Author

kjellr commented Dec 3, 2021

I agree that the controls could be presented better, and I also think it's weird to only allow one or the other per block. I'd happily punt this and just use a spacer block for the Twenty Twenty-Two use case, if the spacer block was a reasonable solution. But it still only accepts pixel values: #22956. 😞

I don't think it makes sense to merge this late on a Friday anyway, so I'll hold off on merging either way.

@jasmussen
Copy link
Contributor

I'll see if I can help #36186 along.

@aaronrobertshaw
Copy link
Contributor

Unfortunately, there are some issues with opting into margin support for blocks, such as the Group block, that also opt into layouts support.

There was a recent PR that attempted to opt into margins for the group block and it was abandoned in favor of the idea to use the block gap support instead. Adding this to the confusion around the padding/margin controls and their effect, it might be better to push ahead updating the Spacer block to fit Twenty Twenty-Two's needs.

@mtias
Copy link
Member

mtias commented Dec 6, 2021

I think the spacer block can provide a better user experience when dealign with creating space — the interactions are more direct and intuitive. A user can drag the handle and change it. In contrast, margin can be quite hard to visualize. Dynamic sizes is something we need to build in if we still want to allow resizing as an action.

@kjellr
Copy link
Contributor Author

kjellr commented Dec 6, 2021

Ok, sounds good. I'll close this out then, and we can focus on trying to get the spacer block support expanded.

@kjellr kjellr closed this Dec 6, 2021
@kjellr kjellr deleted the add/margin-support-to-group-block branch December 6, 2021 14:28
@ndiego
Copy link
Member

ndiego commented Dec 14, 2021

I know this has been closed, and I mistakenly added my own PR for this here #37344. But I do want to surface an important use-case for margin control. More often than not, margin control is useful in removing or overriding default theme margin settings as opposed to adding space (which the Spacer block can do). All of the "magic classes" and custom CSS that I am using removes /modifies block margin as opposed to adding large spaces. Default margin settings are great 90% of the time, but there are certain designs/patterns that need margin tweaks and whatnot. The Spacer block and blockGap cannot handle such use-cases. While a bit less relevant for the Group block, outside of overriding blockGap top margin, margin control is extremely useful/important for blocks like the Paragraph block: #37300

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Group Affects the Group Block [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants