The Wayback Machine - https://webcf.waybackmachine.org/web/20211204132709/https://github.com/WordPress/gutenberg/pull/37105
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

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from

Conversation

@kjellr
Copy link
Contributor

@kjellr kjellr commented on 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
Copy link
Contributor

@MaggieCabrera MaggieCabrera left a comment

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

Loading

@paaljoachim
Copy link
Contributor

@paaljoachim paaljoachim commented on Dec 3, 2021

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

Loading

@jasmussen
Copy link
Contributor

@jasmussen jasmussen commented on 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.

Loading

@kjellr
Copy link
Contributor Author

@kjellr kjellr commented on 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.

Loading

@jasmussen
Copy link
Contributor

@jasmussen jasmussen commented on Dec 3, 2021

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.

Loading

@kjellr
Copy link
Contributor Author

@kjellr kjellr commented on 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.

Loading

@jasmussen
Copy link
Contributor

@jasmussen jasmussen commented on Dec 3, 2021

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

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants