Skip to content
This repository has been archived by the owner on Jan 26, 2022. It is now read-only.

Add negative margins to top-level group and cover blocks #336

Merged
merged 3 commits into from
Jan 13, 2022

Conversation

kjellr
Copy link
Collaborator

@kjellr kjellr commented Jan 13, 2022

Fixes #330.

I think we overlooked something important in #291: Top level Group blocks (when they have a background color) and Cover blocks should have negative margins applied in both the editor and the front end.

This came up while watching a user struggle with how to make an edge-to-edge header with a solid color background. This should be easy, but it wasn't. Our current rules only apply the negative margins to alignfull blocks, so they had to first add a top-level Group block in the site editor, set it to Inherit default layout, and then place a full-width group w/background color inside of that. It was not intuitive, and is the same hack noted in #330 (comment).

Group Blocks with a background color, and Cover blocks already come with have their own padding. So when they're on the top level of the site editor, they don't need to use the outer padding. They can just go edge-to-edge.

Doing this doesn't seem to have any negative effect on our existing layouts or patterns.

To Test:

  1. Add a top-level Group block in the site editor.
  2. Make sure it has left-right padding as expected.
  3. Add a background color to it.
  4. Ensure that the color goes edge-to-edge, and still has inner padding.
  5. Add a top-level cover block, ensure that goes edge-to-edge too.

Also, do a sweep of existing templates and patterns to ensure they're still behaving as intended.

Screenshots

Pattern Preview Before Pattern Preview After
Screen Shot 2022-01-13 at 8 52 24 AM Screen Shot 2022-01-13 at 8 51 59 AM

All blocks in these screenshots are at the top-level of the site editor:

Editor Before Editor After
Screen Shot 2022-01-13 at 8 46 23 AM Screen Shot 2022-01-13 at 8 46 01 AM
Front-end Before Front-end After
Screen Shot 2022-01-13 at 8 46 25 AM Screen Shot 2022-01-13 at 8 45 58 AM

@kjellr kjellr added the [Type] Bug Something isn't working label Jan 13, 2022
@kjellr kjellr added this to the RC 3 milestone Jan 13, 2022
@kjellr kjellr requested review from MaggieCabrera and jffng January 13, 2022 13:52
@kjellr kjellr self-assigned this Jan 13, 2022
@@ -99,6 +99,7 @@ a:active {
body > .is-root-container,
.edit-post-visual-editor__post-title-wrapper,
.wp-block-group.alignfull,
.wp-block-group.has-background,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This overwrites the editor's (random) left/right padding rules for group blocks that have a background color, so that content in there will line up with our usual site padding rules.

Before After
Screen Shot 2022-01-13 at 8 54 22 AM Screen Shot 2022-01-13 at 8 54 04 AM

@richtabor
Copy link
Member

richtabor commented Jan 13, 2022

Just to be clear, the expectation is that contents within an alignfull block are 100% edge-to-edge in the viewport, regardless of parent block padding? (Also may be unrelated to this PR 😬).

CleanShot 2022-01-13 at 09 23 57@2x

@richtabor
Copy link
Member

richtabor commented Jan 13, 2022

This came up while watching a user struggle with how to make an edge-to-edge header with a solid color background. This should be easy, but it wasn't.

With this PR active, how is this achievable? While editing the header part itself, I can see it's fullwidth, but within the Site Editor, it's not.

CleanShot 2022-01-13 at 09 38 02@2x

Do we perhaps need to target within block template parts as well?

CleanShot 2022-01-13 at 09 36 15@2x

And the frontend of the above:

CleanShot 2022-01-13 at 09 38 55@2x

@richtabor
Copy link
Member

I wonder if that particular issue (the header being difficult to make fullwidth) is more related to Gutenberg? As it's not possible to set a template part to fullwidth, or even top-level blocks within it. If we could set that Group block as fullwidth, then the left/right negative margin would be applied properly at the top level. Instead, we are required to double-up Group blocks, just to set the group > group (or group > row) with a background color.

Can't set a top-level block within a template part to fullwidth:

CleanShot 2022-01-13 at 09 40 24@2x

Non top-level blocks can be fullwidth:

CleanShot 2022-01-13 at 09 41 51@2x

Copy link
Collaborator

@jffng jffng left a comment

Choose a reason for hiding this comment

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

Gave this a pretty thorough shake, and in general I think the change looks good and works well. The CSS impact is minimal / highly targeted, and I don't notice any regressions when going through the patterns and templates.

When checking the pattern previews, I noticed something strange with the "About page with media on the left" pattern (which uses the media & text block, no group or cover). When we manually add the alignfull class to a block that is not inheriting layout, it will not appear as full width in the preview, but it will appear full width when inserted if it's added at the root because of the theme's alignment CSS:

Screen Shot 2022-01-13 at 10 03 09 AM

No alignment controls are present because the block is not inside a group that's inheriting layout. It's a bit weird to not have the ability to change the alignment, since the class is added manually. But maybe something to address / consider separately.

@jffng
Copy link
Collaborator

jffng commented Jan 13, 2022

With this PR active, how is this achievable? While editing the header part itself, I can see it's fullwidth, but within the Site Editor, it's not.

I think the header part needs to be set to inherit default layout, and the nested group block's alignment set to full.

@kjellr
Copy link
Collaborator Author

kjellr commented Jan 13, 2022

Just to be clear, the expectation is that contents within an alignfull block are 100% edge-to-edge in the viewport, regardless of parent block padding? (Also may be unrelated to this PR 😬).

I think that one is unrelated to this PR. But it sounds like a bug. Would you mind sharing the steps to reproduce?

@richtabor: Do we perhaps need to target within block template parts as well?

@jffng: I think the header part needs to be set to inherit default layout

Yeah, but it's not possible to set a template to inherit default layout anymore. So you'd have to use a group block wrapper in there.

In any case, I forgot about template parts initially. 🙃 Pull the new updates and give those a try. Should be better now.

template-part.mp4

@kjellr
Copy link
Collaborator Author

kjellr commented Jan 13, 2022

When we manually add the alignfull class to a block that is not inheriting layout, it will not appear as full width in the preview, but it will appear full width when inserted if it's added at the root because of the theme's alignment CSS

Yeah... We could write in some CSS to fix that, but since alignfull there isn't technically supported, I think it gets a little weird. I'm on the fence about it, but I feel like we could add that in separately from these changes and it would be fine.

Copy link
Collaborator

@jffng jffng left a comment

Choose a reason for hiding this comment

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

Yeah, but it's not possible to set a template to inherit default layout anymore. So you'd have to use a group block wrapper in there.

I see. I was testing against the 5.9 RC, not Gutenberg trunk. Adding the template part specific CSS fixes the issue caused by template parts no longer being able to inherit layout, and works fine in 5.9 RC too.

@kjellr
Copy link
Collaborator Author

kjellr commented Jan 13, 2022

I see. I was testing against the 5.9 RC, not Gutenberg trunk.

Ah yes — last I saw, the change that removes that feature from template parts is going to be back ported to the next RC. 👍

@kjellr
Copy link
Collaborator Author

kjellr commented Jan 13, 2022

I've done a bit more testing on my end, and it's still seeming solid. I'm going to merge it in. Thanks folks!

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

Successfully merging this pull request may close these issues.

Inappropriate padding left/right being applied within pattern BlockPreview
3 participants