Make WordPress Core

Opened 6 years ago

Closed 5 years ago

Last modified 5 years ago

#46750 closed enhancement (fixed)

Twenty Nineteen: Add styles to support the group block

Reported by: kjellr's profile kjellr Owned by: laurelfulford's profile laurelfulford
Milestone: 5.3 Priority: normal
Severity: normal Version:
Component: Bundled Theme Keywords: has-patch has-screenshots
Focuses: Cc:

Description (last modified by laurelfulford)

Gutenberg has recently merged in a new "Group" block:

https://github.com/WordPress/gutenberg/pull/13964
(Renamed "Group" here: https://github.com/WordPress/gutenberg/pull/14920)

This block acts as a container for other blocks. The Group block supports standard, wide and full alignments for both itself and its child blocks. The original wide + full styles for Twenty Nineteen only work properly for direct children of entry-content, so the theme requires a patch to ensure proper compatibility with this new block.

If it makes sense to you, I'd prefer to get this patch in as is, and then address those separately. I anticipate a lot of trial and error there

The Group block is currently assigned to the Gutenberg 5.5 milestone. This patch (or a similar one) should be merged into Twenty Nineteen alongside that release, to ensure that the Group block works as intended upon release.

The attach patch includes styles for the Group block, in both the front end and the editor. In general, standard, wide and full-aligned blocks inside of Group blocks should appear consistent with the way they appear outside of Group blocks. Since Group blocks themselves can in turn be standard, wide, or full-aligned, the behavior of child elements should roughly correspond to the recommendation laid out here:

https://github.com/WordPress/gutenberg/pull/13964#issuecomment-472562800

Here's a screenshot of what to expect in Twenty Nineteen (note that padding exists on the "Wide" block when the Section block has a background color in this screenshot):

https://cldup.com/-5A_Ni994H-3000x3000.png


Testing:

  1. Until there's a build for Gutenberg 5.5, the best way to test this is to compile the master branch of Gutenberg and run it on your core dev environment.
  1. Add a new post, and switch to the code view. In that view, paste in this sample content to test a wide variety of Group block alignments, alongside various child blocks: https://cloudup.com/cYDmpgD08FU
  1. Verify that all appears as expected, and that there is no horizontal scroll added to the page. Be sure to test across a wide range of breakpoints as well!

Props @get_dave for all the help getting this working initially.

Attachments (4)

46750.patch (22.7 KB) - added by kjellr 5 years ago.
46750.1.patch (33.4 KB) - added by kjellr 5 years ago.
46750.2.patch (36.7 KB) - added by kjellr 5 years ago.
46750.3.patch (36.8 KB) - added by kjellr 5 years ago.

Download all attachments as: .zip

Change History (27)

#1 @kjellr
6 years ago

Until there's a build for Gutenberg 5.5, the best way to test this is to compile the master branch of Gutenberg and run it on your core dev environment.

In case it helps anyone, here's a version of 46750.patch that can be applied directly totwentynineteen's root directory, for easier testing in existing Gutenberg dev environments: https://cloudup.com/cPLGSt0QVGj

This ticket was mentioned in Slack in #core-editor by kjell. View the logs.


6 years ago

This ticket was mentioned in Slack in #design by kjell. View the logs.


6 years ago

#4 @kjellr
5 years ago

  • Description modified (diff)
  • Summary changed from Twenty Nineteen: Add styles to support the section block to Twenty Nineteen: Add styles to support the group block

#5 @kjellr
5 years ago

Minor update: I've uploaded a new version of 46750.patch that adjusts class names so that this works with the new "Group" name for the Section block instead:

https://github.com/WordPress/gutenberg/pull/14920

I've also updated the title and description of this ticket to match.

#7 @kjellr
5 years ago

Now that Gutenberg 5.5 is in plugin form, it's easier than ever to test this patch. I've updated the instructions above, but I'm also posting them down here too:

  1. Gutenberg 5.5 is now in plugin form, so the easiest way to test this is to install the Gutenberg plugin on your dev environment: https://wordpress.org/plugins/gutenberg/
  2. Once that's activated, add a new post, and switch to the code view. In that view, paste in this sample content to test a wide variety of Group block alignments, alongside various child blocks: https://cloudup.com/cYDmpgD08FU
  3. Verify that all appears as expected, and that there is no horizontal scroll added to the page. Be sure to test across a wide range of breakpoints as well!

This ticket was mentioned in Slack in #core-themes by kjell. View the logs.


5 years ago

This ticket was mentioned in Slack in #core-editor by kjell. View the logs.


5 years ago

@kjellr
5 years ago

#10 @kjellr
5 years ago

Small update: I updated 46750.patch to allow .alignright blocks to extend beyond the width of the container like they do outside of the Group block:

Before:

https://cldup.com/Q8Tgy9LjfW-3000x3000.png

After:

https://cldup.com/9Gn_YHjBXW-3000x3000.png

I also stumbled upon a small issue where non-aligned images were appearing too wide, so I fixed that too.

Note that depending on the outcome of this GitHub issue, this patch may need to be rewritten:

https://github.com/WordPress/gutenberg/issues/15042

#11 @mapk
5 years ago

  • Keywords needs-testing removed

I just tested this patch both in the editor and on the frontend and it works great! Excellent work!

@kjellr
5 years ago

#12 @kjellr
5 years ago

A recent change to Gutenberg has implemented a new wp-block-group__inner-container container inside of the Group block.

https://github.com/WordPress/gutenberg/pull/15210

As a result, this patch requires substantially less code (about 60% 🎉), by allowing themes to reuse existing block styles for full-width group blocks. I've uploaded a new version of the patch that supports this change: 46750.1.patch

This should be ready for review!

#13 @kjellr
5 years ago

I originally forgot to share this here, but here's a brand new set of sample blocks to make testing this easier. Just copy/paste into the code editor of a new post:

https://cloudup.com/cAKkp5zrRLe

#14 @dianeco
5 years ago

I'm testing the patch, I'm seeing small issues inside style.css:

1. By default Core Gutenberg set the margin top and bottom to 0 when a background is set. This propery is overridden by Twenty Nineteen style.
Commonly full-width section with background have no margins. To accomplish it we'd add:

.entry .entry-content .wp-block-group.has-background.alignfull{
 margin-top:0;
 margin-bottom: 0;
}

.entry .entry-content > .wp-block-group > .wp-block-group__inner-container > *:first-child {
  margin-top: 0;
}

.entry .entry-content > .wp-block-group > .wp-block-group__inner-container > *:last-child {
  margin-bottom: 0;
}

2. In the patch the following line make the content touch the top and bottom edges:

https://i.imgur.com/STbenBE.png

.entry .entry-content .wp-block-group.has-background.alignfull {
    padding: 0;
}

It should be replaced by the following line which will leave a vertical padding of 1rem:

.entry .entry-content .wp-block-group.has-background.alignfull {
    padding-left: 0;
    padding-right: 0;
}

3. Not sure of the utility of these lines which impact column inside columns and break their vertical alignment:

.entry .entry-summary > .wp-block-group > .wp-block-group__inner-container > * > *:first-child,
.entry .entry-content > .wp-block-group > .wp-block-group__inner-container > * > *:first-child {
  margin-top: 0;
}

.entry .entry-summary > .wp-block-group > .wp-block-group__inner-container > * > *:last-child,
.entry .entry-content > .wp-block-group > .wp-block-group__inner-container > * > *:last-child {
  margin-bottom: 0;
}

I think they should be replaced by these lines which were in the previous patch:

.entry .entry-summary > .wp-block-group > .wp-block-group__inner-container > *:first-child,
.entry .entry-content > .wp-block-group > .wp-block-group__inner-container > *:first-child {
  margin-top: 0;
}

.entry .entry-summary > .wp-block-group > .wp-block-group__inner-container > *:last-child,
.entry .entry-content > .wp-block-group > .wp-block-group__inner-container > *:last-child {
  margin-bottom: 0;
}

4. Finally the following lines which are inserted in this patch, but not related to group, break the image inside columns:

@media only screen and (min-width: 768px) {
  .entry .entry-content .wp-block-image:not(.alignwide):not(.alignfull) > img {
    max-width: calc(8 * (100vw / 12) - 28px);
  }
}

@media only screen and (min-width: 1168px) {
  .entry .entry-content .wp-block-image:not(.alignwide):not(.alignfull) > img {
    max-width: calc(6 * (100vw / 12) - 28px);
  }
}

https://i.imgur.com/OUqr2Wu.png

@kjellr
5 years ago

#15 @kjellr
5 years ago

Excellent testing, @dianeco. Thank you for being so thorough. 46750.2.patch should take care of all those issues.

  1. By default Core Gutenberg set the margin top and bottom to 0 when a background is set. This propery is overridden by Twenty Nineteen style.

✅ Fixed!

  1. In the patch the following line make the content touch the top and bottom edges:

✅ Fixed!

  1. Not sure of the utility of these lines which impact column inside columns and break their vertical alignment:

You're right, these aren't needed here. They were originally bundled in with the inherited entry-content/entry-summary styles I pulled in for the block.

✅ I've broken them out separately in the updated patch, so they are no longer applied when they don't need to be.

  1. Finally the following lines which are inserted in this patch, but not related to group, break the image inside columns:

Ah yes. That was a bug that I had to fix so that I didn't reproduce it inside of the Group block too. Basically — standard-aligned images were previously not appearing at the correct width on the front end at all. 😄

✅ Good catch on the issue within columns. I've included a little fix for that too now.

#16 @dianeco
5 years ago

@kjellr This patch works great! The only thing that I think misses is the ability to remove margins between 2 consecutive full-width group. Moreover if we want to separate them, we can use the spacer.

By default Core Gutenberg set the margin top and bottom to 0 when a background is set. This property is overridden by Twenty Nineteen style."

.entry .entry-content .wp-block-group.has-background.alignfull{
 margin-top:0;
 margin-bottom: 0;
}

Currently we have:

https://i.imgur.com/NgQhDBF.png

It would be great to have:

https://i.imgur.com/cDkm2bG.png

@kjellr
5 years ago

#17 @kjellr
5 years ago

Good find, @dianeco. It's a little weird to that Gutenberg does that by default right now, since it inevitably results in a mismatch between the front and back end:

Editor:

https://cldup.com/1Vp6mcpwn3-2000x2000.png

Front end:

https://cldup.com/8Cs5Ft3OWa-2000x2000.png

In any case, we might as well follow that lead for now. 46750.3.patch Makes this change. 👍

#18 @laurelfulford
5 years ago

This is looking awesome, @kjellr! I'm excited to see this block on the horizon, and working so nicely in the theme:

I found a few odds and ends while testing:

In the editor, the full-width image blocks bleed out of the group for me:

Wide:
https://cldup.com/q8XsNoMCJW-3000x3000.png

Full:
https://cldup.com/xjB-V1wLHe-3000x3000.png

I've got a bit of side-scrolling too (not specific to these changes or this block, it looks like) so that might be related.

In the editor, full-width columns are slightly indented in the standard and wide-width groups (but display as expected in full-width groups):

https://cldup.com/Z3O7E3Xnnl-3000x3000.png

If the group has floated items, they can 'pop out' of it -- I'm not sure if it should have a "clearfix" or not (and if this should be a theme-level thing, or a block-level one).

https://cldup.com/rlUbm78Q8x-3000x3000.png

On mobile-sized screens, a few of the blocks -- Gallery, Cover, and Media & Text -- don't display at full-width when nested in standard or wide-width group blocks when they have a background colour (they do when nested in full-width group blocks):

https://cldup.com/l9y_IMMEcp-3000x3000.png

If the group block doesn't have a background colour, they do display full width.

And this might be getting into the weeds, but Twenty Nineteen has styles that switch text to white when you set a darker background colour. It looks like this works sometimes with the group block -- like tables, paragraphs and lists all switch to white. Some -- like the verse -- won't switch in the editor, but do on the front-end. Others -- captions, citations -- don't switch at all.

I also can't get the pullquote to preview a changed colour in the editor when nested in a group block with a background -- it stays white in the editor, but will display correctly on the front-end.

Examples in the editor:

https://cldup.com/8rqoSIBAlE-3000x3000.png

Trying to change the pullquote to black:

https://cldup.com/2nvfzF2G0B-3000x3000.png

Same blocks on the front-end:

https://cldup.com/CcebI2XyP6-3000x3000.png

I feel like this one may be a bit of a whack-a-mole to fix, so I wanted to raise it and just get your thoughts.

Just let me know if you have any questions about the above!

#19 @kjellr
5 years ago

Thank you so much for testing, @laurelfulford! I truly appreciate it.

In the editor, the full-width image blocks bleed out of the group for me

I think this is actually just related to this Gutenberg issue:

https://github.com/WordPress/gutenberg/issues/16284

Should be all set as of Gutenberg v6.0 — would you mind giving those another test?

I've got a bit of side-scrolling too (not specific to these changes or this block, it looks like) so that might be related.

I do see a _super_ tiny bit of side scrolling in the editor too, at least when using the test data. I'll dig in a bit and see if I can find the source (and whether or not it's due to this patch or not).

In the editor, full-width columns are slightly indented in the standard and wide-width groups (but display as expected in full-width groups)

Yeah, I think this is a core Gutenberg bug, as it's showing up all themes I've tried. I've filed a bug for it here:

https://github.com/WordPress/gutenberg/issues/16321

In the meantime, I don't think we should make any adjustments for it here.

And this might be getting into the weeds, but Twenty Nineteen has styles that switch text to white when you set a darker background colour. It looks like this works sometimes with the group block -- like tables, paragraphs and lists all switch to white. Some -- like the verse -- won't switch in the editor, but do on the front-end. Others -- captions, citations -- don't switch at all.

I also can't get the pullquote to preview a changed colour in the editor when nested in a group block with a background -- it stays white in the editor, but will display correctly on the front-end.

I feel like this one may be a bit of a whack-a-mole to fix, so I wanted to raise it and just get your thoughts.

Ugh, those are really annoying. I agree that it's likely a whack-a-mole sort of fix. If it makes sense to you, I'd prefer to get this patch in as is, and then address those separately. I anticipate a lot of trial and error there. 😅

I'll take a look at the horizontal scrolling issue before we do a final pass — I'll have an update on that in the next day or two.

Thanks again!

#20 @kjellr
5 years ago

I do see a _super_ tiny bit of side scrolling in the editor too, at least when using the test data. I'll dig in a bit and see if I can find the source (and whether or not it's due to this patch or not).

Following up on this: I think the only block that has this horizontal scrolling is the full screen Gallery block. If I turn those off in the test data, the scrollbars disappear. This is an upstream bug:

https://github.com/WordPress/gutenberg/issues/14704

Given that, I think this is good to go. 👍 Mind giving it one more test, @laurelfulford?

Thank you!

#21 @laurelfulford
5 years ago

  • Description modified (diff)

Thanks @kjellr! The patch is looking good! Following up on few comments from above:

In the editor, the full-width image blocks bleed out of the group for me

I think this is actually just related to this Gutenberg issue... Should be all set as of Gutenberg v6.0 — would you mind giving those another test?

Yes, looking good now!

In the editor, full-width columns are slightly indented in the standard and wide-width groups (but display as expected in full-width groups)

Yeah, I think this is a core Gutenberg bug, as it's showing up all themes I've tried. I've filed a bug for it here... In the meantime, I don't think we should make any adjustments for it here.

Thanks for digging into that! I agree that it makes sense not to address that in the themes if it's happening in all.

And this might be getting into the weeds, but Twenty Nineteen has styles that switch text to white when you set a darker background colour...

If it makes sense to you, I'd prefer to get this patch in as is, and then address those separately. I anticipate a lot of trial and error there.

That makes sense to me! I hate issues like this, too, they're a pain to fix -- I agree it'll be easier to dig into/test separately.

All my concerns have been addressed, so I'll work on getting this committed.

#22 @laurelfulford
5 years ago

  • Owner set to laurelfulford
  • Resolution set to fixed
  • Status changed from new to closed

In 45605:

Twenty Nineteen: Add styles for the new Group block.

Add styles for the new Group block to the theme, to make sure nested blocks display correctly when using the wide and full alignments.

Props @kjellr, @dianeco.
Fixes #46750.

#23 @razvanonofrei
5 years ago

Hello there,

I'm a little late to the party, but just wanted to chime in and give my two cents on this and also hear what you think about it.

My main concern is related to the fact that having a full width container with some inner blocks that should keep the content width of the blocks outside the container is going to become a popular pattern soon, but keeping plugins and themes compatible is already pretty touch.

For example, the core Cover Block does pretty much the same thing as the Group Block (speaking in terms of layout), but the way they are styled with CSS is totally different: the cover uses padding while group uses margin to center its content.

Wouldn't it be more useful to make use of the already established class naming convention wp-block-*__inner-container and use less specific CSS selectors like [class*="wp-block"] > [class*="__inner-container"]

This would allow less CSS in some situations but would also allow developers to build blocks that would seemingly integrate with Twenty Nineteen. I believe that in doing so we would give a chance to having more consistency and compatibility across the most popular themes and plugins out there. What do you think?

Best regards,
Razvan

This ticket was mentioned in Slack in #core-editor by mapk. View the logs.


5 years ago

Note: See TracTickets for help on using tickets.