-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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 block spacing gap config to theme.json and add support for this CSS variable to the "flow/default" layout. #33812
Conversation
@@ -2,7 +2,5 @@ | |||
&.has-background { | |||
// Matches paragraph Block padding | |||
padding: $block-bg-padding--v $block-bg-padding--h; | |||
margin-top: 0; | |||
margin-bottom: 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was conflicting with the this consistent margin styling. @jasmussen maybe you know where in our code base we define the block margins and whether we should move these to classic.css now.
Size Change: +5.45 kB (+1%) Total Size: 1.04 MB
ℹ️ View Unchanged
|
@ellatrix This config doesn't seem to work in the site editor for some reason, not sure I understand why. |
I find this concept more confusing than simply having margin support. |
Nice, thanks for working on this. I'm a big fan of gap! (See also #32366). As far as the instructions in this PR, this also works as advertised, however it doesn't appear to be the
Would I get true The margin-based gap as I see here is useful enough on its own, but the reason I'm longing for "true" Basically with using only padding and gap (Figma calls it "Space between items"), you can quite easily create consistent spacing. This is especially useful in masonry galleries, where you might have to tackle gnarly nth-child rules. |
This is inspired by on some advanced layout work you can read about here https://every-layout.dev/layouts/stack/ |
Margin is not something that will solve this issue consistently, theme authors will need to be able to target, first and last blocks... Also, it doesn't scale well to all types of layouts. |
The key mental shift here is that margin, conceptualized as the spacing between blocks, is something extrinsic to the block itself and better imagined as a property of the container. Individual blocks can overwrite margin values, but it should be sporadic and considered more advanced (it can be difficult to handle for a user and break visual rhythm). I think we might want to avoid Having control over this globally and per container should simplify how you need to deal with blocks and margins, as it should handle dealing with the top / bottom blocks by default, which has been a bit painful in columns already. In the "spacing" panel we should also be clear when a block is overwriting the default gap setting. |
I added support for the blockGap to the flex layout as well. |
I think this looks very cool, I like the idea of using gap to control the spacing between blocks, and the CSS variable approach looks like a clever way to do it!
Separately, I've been chipping away at a PR to add in a gap block support feature in #33991 as a way of trying to come up with a solution for #32366. My PR is split out from the ToolsPanel / DimensionsPanel work in #32392 and adds in UI / block support with a BoxControl in the inspector controls and global styles for controlling the gap. I was imagining this as a way of handling gap (and It looks like this PR will provide a more general solution and approach for layouts, so it looks like it might supersede my one. But I just thought I'd check in case there's anything useful in #33991 that you might want to pull in here or if there's a way to combine things. I'm happy to help any way I can! With this PR, do you imagine the layout types will be rolled out to all other container blocks (e.g. Buttons, etc) or would we have a different approach for those sorts of blocks versus, say Group? It could also be good to include the ability to separately control row vs column gap, depending on where we roll the feature out, to support different kinds of layouts. |
Thank you, that's helpful! Sounds like the CSS-first refactor can move forward and be revisited further.
I ran into wanting to create a classic header menu that featured a site logo and a navigation block left and right as part of the Header template part, and finding that the stack-margin ruleset was applied even though I went for a horizontal layout: But that use case is already met by #33316, since this can now just be a single navigation block inside the header. But it does support the separate conversation we were having about allowing the group block to become a horizontal flex container. |
A thing I noticed as well. We output only the Since in vertical flows, those margins commonly collapse, it's been fine so far. But it seems like we should? I can open a separate ticket. |
I think that makes sense 👍 |
From #33991, I got the impression that this feature is opt-in with settings.spacing.blockGap, but it seems like the |
The current reasoning is that it's intentional, the style is not opt-in, the per block UI is. |
Got it. That might cause some issues for themes that have different margins for different elements (for example, larger margin for headings, tables, separators etc. than paragraphs) and at different breakpoints, since the specificity on the styles overwriting |
it only applies to themes with theme.json though which I think has just been release on 5.8. So hopefully not a lot of themes. The dev note is supposed to clarify that. Do you think that's a fine shipping plan? |
I'm also curious about the use-case. Would things like this solve it?
I'm also thinking it might not be the best approach because we don't want that "margin" value when the block is inside a horizontal/flex container for instance. |
I think being able to opt out would lower the barrier for existing themes adding theme.json pretty significantly, since most themes would probably need to modify their margin styles a lot to overwrite the default gap value (in the cases where they want to overwrite them). I think there are cases where themes built with theme.json from the ground up would want to opt out as well, if many elements in the design have different margins for different breakpoints.
Being able to set the gap on a block level would definitely go a long way. I'll ask for some input in the theme review channel on Slack. |
The issue I have with this is that blocks shouldn't have intrinsic margins because blocks shouldn't be assuming that they are inserted in a "flow" container, they can be on a flex container, horizontal... and those margins won't make sense anymore. I don't think a theme author should have the responsibility to define all these use-cases because it will break very quickly. We can add a way to "not output these styles", but I fear that this is just making things worse for themes and break some blocks in different contexts. So how to solve this use-case more holistically? |
I just checked this and |
Yeah, it's a difficult balance to strike. Most theme authors will want to have some degree of control over margins though, at least on the base element level (paragraphs, lists, headings, etc.), and it can pretty quickly lead to specificity wars. For example, the margin styles generated for the h1 element by the code below get overwritten by the new
So if a theme would want to have a different margin than the blockGap for h1, they would need to add CSS looking something like this to overwrite it:
|
Should theme authors be able to opt-out? If this is ever a question that comes up, the answer is always: Absolutely, 100%, yes! The only assumption we should make is that a theme author will want to do something custom. If we start from that position, these sorts of conversations will happen less often. This block gap feature has created issues across the board for me. My plan is to work with it as much as possible, but I am already seeing places where it will be problematic and force specificity battles. Doing this in a per-block level would ease some of that. For example, my Heading elements tend to have a different top margin than Paragraph elements (this was actually the first thing I noticed that was broken). Edit: On the topic of horizontal, flex, etc. containers: those are exceptions to the rule. For example, I target |
I disagree with this take. This means that everything should be optional in WordPress and goes against the decisions not options. some things need to be options but not everything. After the discussion we had here, I agree that for now the block gap styles need to be opt-out because we don't have answers for all the use cases yet but I don't think it should be a rule to have an opt-out for everything personally. For instance for structural styles, I'd rather have the themes rely on Core always instead of reinventing their own. Themes are here to bring personality and design but not to define what "horizontal alignment" means for instance.
I'd love to learn mode about the use-cases to be able to come up with the best abstractions that solve this. If we always fallback to CSS to solve these issues, specificity issues will continue forever and adaptation as Core changes structural changes will continue to be a nightmare. Theme.json is an attempt to solve that, so we should try to reduce the need for these custom CSS (other than things that bring personality) from themes.
👍 Yes that use-case has been raised above and makes sense but it's also important to understand that it only makes sense in the "flow" layout. If you insert a heading block inside a horizontal container, it shouldn't have any margins. For this particular reason, we need to be more smart about it:
|
I can think of other cases where you'd want a different gap per block. One example would be the "small" blocks that have post meta content such as post date or post tags, we encountered this on skatepark, where the margin bottom (or gap) for an h1 could be 5 times or more the spacing between these smaller blocks: |
Seeing the recent comments here, I was dreading how this might affect the block-based theme I'm currently working on. The effects ended up being minimal due to already adopting the "styling the context" approach every-layout.dev endorses, but at a different location: the A few examples from my current theme where elements require different "gaps" in a flow context:
Very much agree from a theming perspective.
I've gone back and forth since 5.0 with adding to core styles vs. 100% custom styles, and, overall, I've seen custom styles be the only way to achieve layouts I'm handed, as well as being the easiest to maintain over the long term (the other reason is, using custom styles has allowed me to use things like CSS Grid for layout now rather than waiting for it to appear as a WordPress-provided option). Working with WordPress post-5.0 has meant learning to write styles in a much more defensive and resilient way, something I see as a positive outcome. However, I do often feel idiotic for writing my own style-sheets. Perhaps it's time to send a clear signal to someone like me by removing the option to dequeue core styles? |
I would like to be able to opt out for the theme global gap and opt-in on specific blocks. |
Yes, yes, yes!!! I think this is an absolute must, since a general rule might work for some smaller projects, but it does not leave enough choice/control for theme developers to make custom decisions. At the moment I get the feeling in general that we might need more clarity on "what goes where" as I struggle to understand what is meant to be block 'territory' and what is theme territory and so on. |
I opened #34716 to start documenting use cases that need improvement |
Folks have asked for some time now for a consistent way to tweak the margin/gap between blocks. This PR tries to solve that.
Decisions and choices made on this PR
Notes
For block templates a global CSS style applied to the root of the block templates is added by default to the theme.json generated stylesheet. Unfortunately for classic templates, we can't generate that styles automatically in the frontend because the "wrapper" for post-content doesn't have a consistent className (like
.entry-content
or something). This is also the exact same reason we ask these theme-authors to add "layout" (wide/full alignments) styles on their own to the frontend. We could potentially try a solution for this separately by relying on a convention.Testing instructions
"spacing": { "gap": "some value" }
in your theme.json file