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

Varia: Extra vertical spacing for Layout Grid block in Editor #1798

Open
iamtakashi opened this issue Feb 12, 2020 · 19 comments
Open

Varia: Extra vertical spacing for Layout Grid block in Editor #1798

iamtakashi opened this issue Feb 12, 2020 · 19 comments
Labels
[Theme] Varia [Type] Bug Something isn't working
Milestone

Comments

@iamtakashi
Copy link
Contributor

Steps to replicate

  • Add a group block as the outermost block.
  • Add a background colour to the group block.
  • Add a Layout Grid block inside the group block
  • Add a block inside the column.
  • Add another group block as the outermost block.
  • Add a different background colour to the group block.
  • Add a column block inside the group block
  • Add a block inside the column.

Screenshot 2020-02-12 at 15 59 21

Notice the difference in the vertical spacing. The one with the core column block is expected. This is how it looks on the front of the site.

Screenshot 2020-02-12 at 16 06 48

@ianstewart
Copy link
Contributor

Will this cascade out to all the Varia child themes if we correct it here?

@iamtakashi
Copy link
Contributor Author

Unfortunately, the child themes need to recompile their CSS and to be deployed :/

@ianstewart
Copy link
Contributor

Ah — but once fixed, child themes re-compiled and deployed, we'll have the fix across 20+ themes. Seems like a good priority fix since it affects the 1:1 fidelity of front-end vs Editor. cc @kjellr and @allancole for thoughts.

@allancole
Copy link
Contributor

Looks like a fairly easy fix to sort out. Shouldn’t be too hard to recompile the fix into all the child-themes either (this can be automated). The hardest part will be getting all 20+ themes tested and deployed to dotcom. That part has to be done manually for each theme 1-by-1.

@nielslange nielslange added this to the Varia milestone Feb 17, 2020
@iamtakashi
Copy link
Contributor Author

Something like this should make the editor view better.

[data-block][data-type="jetpack/layout-grid-column"] {
	margin-top: 0;
	margin-bottom: 0;
}

[data-block][data-type="jetpack/layout-grid-column"] .block-editor-block-list__layout *:first-child {
	margin-top: 0;
}

[data-block][data-type="jetpack/layout-grid-column"] .block-editor-block-list__layout *:last-child {
	margin-bottom: 0;
}

What do you think, @allancole? Any chance you could move this forward soon?

@iamtakashi
Copy link
Contributor Author

@kjellr Could we get some attention to this issue if it's possible?

As we're using more Layout Grid block for layouts and block patterns, more customers are having a difficult time to edit the page because of the disparity between the editor and the front of the site. Obviously, this affecting both of the previews for the layouts and the patterns.

For example:
Screenshot 2020-04-29 at 17 25 15

When it should look like something like:
Pattern 67

There will probably better way to fix this than my quick suggestion above :) Thanks in advance!

@kjellr
Copy link
Contributor

kjellr commented Apr 30, 2020

@iamtakashi We have some higher-priority items at the moment, but I'll try to get someone on this next week.

@iamtakashi
Copy link
Contributor Author

@kjellr Thank you! 🙇

@kjellr
Copy link
Contributor

kjellr commented May 5, 2020

Just a heads up that I looked into this briefly, and I'm actually seeing the same extra margins on the columns block (and all other nested blocks, frankly):

Screen Shot 2020-05-05 at 09 32 55

Tested with Gutenberg v8.0.0.

I think this is related to the fact that nested Gutenberg blocks aren't collapsing their margins — they end up building up like that, the deeper you go. I don't know that the solution is actually removing the margins entirely, since nested blocks should have margins to prevent them from bumping up against adjacent inner blocks. So this will take a little adjusting.

@iamtakashi
Copy link
Contributor Author

iamtakashi commented May 7, 2020

Thanks for looking into it @kjellr!

As you said, the core column block now also has the extra margins, and I understand why they are needed.

I agree that removing all of them wouldn't be a good solution, but what could we do to make this situation better? A quick comparison shows the one in the editor has a double the height that we see on the front of the site.

Screenshot 2020-05-07 at 01 04 29

Could we reduce the margins for some blocks? I imagine we don't need the same vertical margins for the blocks that don't have the margin on the front of the site.

@kjellr
Copy link
Contributor

kjellr commented May 7, 2020

I wonder if this is something that should be handled in Gutenberg, since it technically effects all themes. I've also been burned in the past by targeting too many Gutenberg default styles in a theme... it often leads to breakage eventually.

cc @jasmussen for an extra set of eyes here — you tend to be much more in tune with all the margins in Gutenberg than I am.

@jasmussen
Copy link
Member

Yes, I can confirm that the layout grid has 1 extra unnecessary instance of margin, which makes it not look the same as the frontend.

Because of that fact, I would consider it a bug in the Layout Grid block, and one that we should fix. Takashi is indeed right, in my preliminary testing, this fixes it:

[data-type="jetpack/layout-grid"] {
    margin-top: 0;
    margin-bottom: 0;
}

I will add that to the layout grid block, because I also have to think about how to handle this:

Screenshot 2020-05-07 at 14 26 20

Permit me to We have the following rules in the editor:

.block-editor-block-list__block {
    margin-top: 28px;
    margin-bottom: 28px;
}

That is a "baseline margin" that is applied to every single block ever, regardless of nesting level. It is meant to be overridden and customize, but provide a "fallback minimum margin" in cases where a block does not provide any margin.

That also explains why the group block adds additional margins — it's also a block, even if it is often just an invisible nesting container. When margins don't collapse, as they don't do in cases of flex, that causes the extra issue. I will investigate what a good fix might be there, but it's nontrivial.

There's a farther out solution, it's going to get long-winded, but if you have time, I could use your advice. Ready?

👇
👇

Why not just remove that rule entirely, and allow every individual block, or the theme itself, to specify the margins of each block? Well, it's complicated. Outside of a rough transition (some blocks will break because they assume those margins, or even compensate for them), there are some big and fundamental questions, and I would love your themer thoughts on them.

Given a block can be wrapped in anything, a figure, a p, a div or section or span or even nothing at all, are we sure we want to remove this margin entirely? You can imagine most of those blocks lying flush next to each other if we removed it, and others, like the figure, might have wildly uneven margins.

If we did remove that margin — should each individual block really provide those margins instead? For example our Image Compare block is basically like an Image, so it should have the same margin as an image, right? But how would margins across blocks be consistent?

More problematically, if we did provide, say, this as part of the paragraph block:

p {
margin-top: 1em;
margin-bottom: 1em;
}

then we would effectively be creating new rules that themes need to override anyway, just vestigal stuff for the browser to parse, understand and then discard.

We also can't really provide a globally inherited CSS block margin rule, as this would require new CSS classes for the frontend, for every block, even paragraphs. And even then, it would just make it even harder to override.

Ultimately it seems like the path forward is to remove the "baseline margin" entirely, even if it will become uncomfortable. Because ultimately, it feels the purview of the theme alone to provide a vertical rhythm. And it will get uncomfortable, here's what TwentyTwenty does:

figure {
	display: block;
	margin: 0;
}

We'll probably need some new rule that applies for the vanilla editor style, the one that shows up when a theme does not register an editor style. That's its own challenge.

@jasmussen
Copy link
Member

Here's a more visual example:

layout grid

As background, this is in a theme that customizes the paragraph margin to be 1em top and bottom, which is less than the 28px top and bottom margin that the editor provides as a baseline.

When the GIF starts, it shows the in-progress fix for the layout grid, which removes top and bottom margins from the block. Things flow nicely.

Then that layout grid is turned into a group, and suddenly the margins that are applied to the group kick in — those are bigger than the default paragraph margin, so while they collapse, it does make for bigger spacing.

Question time. Do we:

  1. Remove that baseline margin entirely, with the challenges it will bring?
  2. Add `.wp-block-group { margin-top: 0; margin-bottom: 0; } to the group block editor style?

It's worth clarifying, that baseline margin is editor only, it does not appear on the frontend.

@jasmussen
Copy link
Member

Layout grid patch here: Automattic/block-experiments#90

@iamtakashi
Copy link
Contributor Author

@jasmussen, thanks for your insight and moving this conversation forward. Also, thank you for pushing the fix to the Layout Grid block!

Removing the baseline margin is a very interesting idea. There seem to be a few steps to get there, but that's definitely something we want to keep discussing! But in the meantime, I'd +1 on removing the outermost margins from the Group block editor style.

@jasmussen
Copy link
Member

Thanks for the feedback!

I've created WordPress/gutenberg#22208 that removes the baseline margin entirely — I do not expect it to be merged, but hopefully it can generate some good discussion!

I also created WordPress/gutenberg#22209, which should have a much better chance of getting merged, as it's a mostly harmless change to just the group block.

@kjellr
Copy link
Contributor

kjellr commented May 8, 2020

Thank you for digging into this so thoroughly, @jasmussen! I'll weigh in on those Gutenberg tickets.

@pbking
Copy link
Contributor

pbking commented Jun 3, 2021

Took another look at this today. Still appears to be a problem despite the items noted above being merged already. Doesn't appear to be theme-specific though and I would recommend against trying to overcome it in any of the themes. Seems to be specific to the block and should be reported there instead.

@jasmussen
Copy link
Member

We're releasing a new version of Layout Grid soon, so I wanted to see if I could bundle a fix. But from what I can tell, it appears to be both an editor issue, and to a smaller extent, a theme issue (more like a consequence of the editor):

currently

Notice how the group has padding + background color, and this correct between editor and frontend. But on the frontend, the p has zero top and bottom margin, for some reason, whereas in the editor it does have top and bottom margin. So it's two issues:

Firstly, for classic themes, the editor outputs top and bottom margins for every block (as defined by the [data-block] attribute). Recent classic themes take control of that in this way:

Screenshot 2021-06-04 at 08 43 14

It's a bit clunky, and for that reason, while TwentyTwentyOne theoretically could output an editor style to match the frontend and fix this discrepancy, I understand why that's not really trivial to do.

The good news is: block themes don't have this issue, because default top and bottom margins aren't output by the editor. If the theme stylesheet is loaded in the editor, it's just the same with no difference. Here's editor and frontend in my block theme:

Screenshot 2021-06-04 at 08 30 13

Screenshot 2021-06-04 at 08 30 18

Here's editor and frontend in TT1 blocks:

Screenshot 2021-06-04 at 08 31 12

Screenshot 2021-06-04 at 08 31 18

The bad news is, we probably can't fix this in the Layout Grid plugin. The top and bottom margins for blocks inserted should be controlled by the theme, even if the theme is at the mercy of the editor styles. In that vein, I think the fix, ultimately, will be to move towards block themes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Theme] Varia [Type] Bug Something isn't working
Projects
None yet
Development

No branches or pull requests

7 participants