Try adding an inner container to the Group block.

[kjellr]

Fixes #15042.

This PR adds a wp-block-group__inner-container container within the Group block. This follows the example of the Cover block, which has a similar wp-block-cover__inner-container block inside of it.

This idea is explained in more detail in the ticket, but in a few words: for themes that already add margin/padding/max-width to entry-content, this change has the potential to make theme styling easier, if themes are following the recommended behavior for wide + full child blocks outlined in #13964 (comment).

To illustrate the potential benefits, here's a CodePen using some simplified code for wide + full alignments:

https://codepen.io/kjellr/pen/WWKEqN

Today, this scenario requires 19 lines of code to implement the group block + all of its child block alignments:

https://codepen.io/kjellr/pen/QPBgJV

With a container div, it would take just 6 lines of code instead:

https://codepen.io/kjellr/pen/eoPQde

---

Note that since this changes the markup for the Group block, any themes that have rushed to implement the Group block in the past couple weeks would need to patch to support it. The container block would be unstyled though, so if anything, they'd likely just need to change some CSS selectors.

As of the opening of this PR, the two Core themes that need patches to support the group block do not have approved patches yet (46750-Trac, 46778-Trac). The currently in-progress patches would need to be rewritten, but this change should end up making those implementations simpler.

[kjellr]

Just a quick note: I spent a little time seeing how this would impact Twenty Nineteen, and I was able to get a generally-working solution pretty quickly. It looks like adding a container block like this could potentially require about 60% less code than the existing solution.

[getdave]

I'm pleased to see this. Note this approach was rejected from the original core/section PR but I can't quite recall why. I think @youknowriad had concerns about the editor as a canvas?

[kjellr]

I'm pleased to see this. Note this approach was rejected from the original core/section PR but I can't quite recall why. I think @youknowriad had concerns about the editor as a canvas?

Worth noting that this shouldn't necessarily effect anything in the editor, since (this PR at least) doesn't actually change much there. It just adds an extra, unstyled div, which is mostly for use on the front-end, as per the in-depth discussion in the issue: #15042

Also — if someone has a moment (assuming this ends up moving forward), I could use a hand getting these tests to pass. 🙌

[talldan]

Hey @kjellr, the first thing I noticed is that this will require that a block deprecation is added, since it changes the output of the save function. To test that, you can try creating a post with a group block on master, and then switching to this branch and loading that post into the editor. Without the deprecation the block validation fails and an error is displayed.

Looks like the full-content integration tests are failing, they also test the output of the save function. Some instructions for those is in this README:
https://github.com/WordPress/gutenberg/tree/master/packages/e2e-tests/fixtures/blocks#full-post-content-test-fixtures

The snapshot end-to-end tests are also failing for the same reason, they can be updated by running npm run test-e2e:watch. Once the jest prompt shows up, I usually filter to the run only the failing test using the p option, the snapshots can then be updated by pressing u when prompted.

I'm also happy to help and can push a commit to fix those things if needed.

[getdave]

I don't see any issues with this change other than adding the deprecation as @talldan suggestions. From a Theme author perspective, I can see how this change will make styling a lot easier.

[kjellr]

@talldan or @getdave — any chance either of you have a moment to help with the failing tests and the block depreciation here?

For the tests, I followed the instructions from @talldan above (those were really helpful, thank you!) but then things seem to have gone haywire after merging master in. 🤔

[talldan]

edit - actually, I'm not sure the below statement is accurate, I don't think the failures are related to the WP version. Though I'm not sure why they're failing.

---

Hey @kjellr - I've fixed a linting error thrown up by one of the tests, but it looks like the end to end tests are all now failing. I recall that this happens whenever a new version of WordPress is released 😄

I'll see if I can find some instructions on how to fix it.

[talldan]

Turns out there were some issues with the e2e tests resulting from the 5.2 release, just not the ones I'd originally thought.

They were fixed in master, so I've rebased the PR. Hopefully the tests will pass now.

[kjellr]

They were fixed in master, so I've rebased the PR. Hopefully the tests will pass now.

🎉 They passed! Thanks @talldan!

How do we handle the block depreciation? I think that's the last bit here.

[talldan]

Hey @kjellr—I can help out with that.

The actual deprecation is pretty straightforward in this case, it's just copying the attributes object, save function and supports object as they were before you made any changes into a new deprecated property of the block specification. It gets a bit trickier if the block is changed radically (or renamed). Some more info here:
https://wordpress.org/gutenberg/handbook/designers-developers/developers/block-api/block-deprecation/

We've started adding tests for all the block deprecations as part of the full-content tests mentioned before.

I'll add those in a commit now.

[youknowriad]

Personally, I find this is all quite difficult to think about entirely in the abstract. Could/should we create a POC PR so we can see how this might "feel"? This may allow us to work out if this is truly a course we should look to pursue.

That would be awesome ❤️

Another thing that comes to mind is, how would we handle a migration where we're fundamentally changing the way the group block works. It seems to me that we can't easily anticipate how this change will cause knock-ons bugs in sites that already make use of Group. I may be wrong however.

That is true, we could probably create a deprecated version with a migrate function to enable that setting by default. That said, the group block is still in the plugin phase and not shipped in Core. With good communication and deprecations, we can still change its behavior.

[jasmussen]

I just had a quick chat with Kjell about next steps specifically around this inner container div, and we talked through the potential future explorations outlined in the comments above (starting here).

The quick summary is that it doesn't feel like adding an inner container to the group block at this time blocks us from exploring improvements in the future, and at the same time it does take some of the vinegar out of supporting wide and full wide nested blocks inside that group, for theme authors today. So from me, 👍 👍, no woes about proceeding.

To elaborate a little bit — some of the discussions revolved around how in the future we might be able to leverage CSS Grid to create more advanced layouts, simpler wide and fullwide blocks, and even "pulled to the side" blocks, while at the same time adhering to a responsive grid system in a userfriendly way. It's a tall order, but the bones of a path forward are slowly forming themselves into a skeleton.

In a CSS grid world, such an inner div is not explicitly desired — a flatter hierarchy is usually best. But at the same time, such an inner div is easily worked around: just make it full-wide. But at the same time, we might not be able to just spring CSS grid on the block editor like that — just like how wide and fullwide blocks require theme opt in via add_theme_support, so probably would a CSS grid layout system. And in such a case, if the extra inner div ever became a problem, we could presumably decide to just not output it, if the theme opts in to grid layouts.

[kjellr]

Thanks @jasmussen (and @youknowriad + @getdave for your input as well). I can expand on the thinking here a little too.

First, I'm very much into the idea of having a "centered" column vs. full-width content toggle.

The full-width mode would be excellent, and wouldn't require any additional work from theme developers. Since every interior block would be full-width, we can just write code to enforce that ourselves. No extra work needed. 🙌 I'll note that in this mode, having an inner div won't necessarily break anything — it can be full width, and have no margin or padding. Semantically, it's not used in this case, but it won't break anything.

Regarding the "centered" mode (which will need a better name... not all themes will necessarily "center" this on the page), I think having the inner div is essential for now at least. Here's why: today, a small portion of themes treat entry-content as a 100% full-width container:

The Gutenberg Starter theme behaves this way. Full width blocks are 100% width, and the theme uses * selectors to enforce max widths on all children. In this case, if we were to think of entry-content as just a Group block, it won't need an inner div. This is also true for themes that use css-grid to implement wide and full, but there aren't many (any?) of those that work well with Gutenberg right now, because the editor itself doesn't fully work with CSS grid yet (😩 floats 😩).

Instead, most themes today treat entry-content as a nested child of the body container:

In this example, the body is (obviously) full-width, and entry-content is the inner container, with its own max-width and margins. Wide and full blocks break out of the inner container as needed (usually using negative margins). If we think forward to a future where entry-content is essentially just a group block that lives amongst other group blocks on the site, we'd want to account for both the full-width parent container and the child container the block itself to avoid wide/full blocks expanding beyond the uncontrollable area beyond the boundaries of the Group block itself.

So anyway, the toggle sounds great. I'd love to see a PR for these two modes. I don't think this PR enforces one way over the other in the meantime, and I think it's also something that could theoretically be depreciated or relegated to an optional theme setting once we move into a css-grid future where this "centered" column isn't really necessary anymore.

[mapk]

Wow, really great insightful thoughts around the implementation of an inner div. Thanks for exploring this in detail. I'm in favor of the inner div based on @joen's and @kjellr's comments above.

These three notes are key!

1. It makes it easier to style for theme devs.
2. It won't break anything and can be ignored for full-width settings.
3. It doesn't force us down an opinionated path that becomes difficult to readjust.

[kjellr]

Sounds like @mapk and @jasmussen are both on board with this approach now. @youknowriad — I'd just like to get one last gut check from you as well. I'd like to get this decided one way or another soon, as it's blocking the core themes Group block patches. Thanks all!

[youknowriad]

If we merge this PR as is, it means that the future "has-centered-container" attribute is going to be "on" by default right?

[youknowriad]

What I'm trying to clarify is how do we maintain backward compatibility once we introduce the "centering" option. If the "group" block's outer div remains styled similarily if it's centered or not, then we're probably fine.

[kjellr]

What I'm trying to clarify is how do we maintain backward compatibility once we introduce the "centering" option. If the "group" block's outer div remains styled similarily if it's centered or not, then we're probably fine.

Yeah, if we were going to introduce that mode, I think it would be enabled by default. That's not because of this inner container, but because that's the way this works today. Currently, in the editor, interior blocks are already centered by default if you're using a theme with no theme styles. This PR doesn't make any CSS changes beyond just selector updates, so it doesn't change that. The container mostly just helps replicate in the front end what you're already seeing in the editor.

(Edit: Whoops, hit the wrong button and closed this by mistake. 😂)

[kjellr]

Following up on a discussion on Slack, this PR is just about ready to go. Would someone mind giving it a review? cc @youknowriad @getdave

Once this lands, I'll open an issue for the followup: toggle between "centered" content and full-width content.

Thank you!

[talldan]

Looks solid.

I tested:

1. That the deprecation still works, it does.
2. Rebased locally onto master and tested that the new group/ungroup feature works, it does.
3. Compared appearance of editor and post content against master (in gutenberg starter theme) and it looks good.

I did notice that in Twenty Nineteen things don't look quite right at full width—is that because the theme needs to be updated to account for the new div, @kjellr?

If so, I think this is good to go 👍

[kjellr]

I did notice that in Twenty Nineteen things don't look quite right at full width—is that because the theme needs to be updated to account for the new div, @kjellr?

That's right! I'll revise this Trac ticket with an updated patch shortly, and then we should be all set:

https://core.trac.wordpress.org/ticket/46750

Thanks everyone!

[kjellr]

I opened #16197 to track the other "non-centered" option we discussed above. 👍

[getdave]

Really pleased to see this land. Great work! 🎉

[AlchemyUnited]

@jasmussen My apologizes to being so late to your 23 May comment.

Some thoughts...One of the issues I immediately notice with GB was that Block Alignment and Block Width were combined. That's a flaw. Alignment is not width. To your point about different layouts, I put some thought (and block building time) into this issue. In my mind, there's the block, within the block os a box, and within that box is the actual content.

For example, you can have a full-width block with a red background, the box within it (which has it's own width and alignment) could be blue, and within that an image (also positioned - left center right / top middle bottom) within the blue box). If it helps, imagine if each of those were not only colored, but animated. Red block slides in, followed by the blue box, and the white text fades in.

That's kinda the reality of pattern. That's is block > box > content.

As for grouping. To me, it's strictly a way to keep things together without having to worry about losing their struture (if you will). Can design / styling be added (e.g., background color)? Sure that makes a lot of sense. That said, I would think / hope, the children would be aware of their status as a child to a parent group and those child might respond accordingly. That is, for extra, the group could override children color settings. But now the group is kinda sorta changing - for the worse? - it's key role. That is, to group.

Finally, where does Group meet HTML5 semantic tags? That makes more sense then Group taking on design responsibilities.

Yeah, that was a bit rambling. Sorry. :)

[jasmussen]

Some thoughts...One of the issues I immediately notice with GB was that Block Alignment and Block Width were combined. That's a flaw. Alignment is not width. To your point about different layouts, I put some thought (and block building time) into this issue. In my mind, there's the block, within the block os a box, and within that box is the actual content.

I don't entirely agree. Float left, center, and float right, could be argued to be layout changes, just like the wide alignments are. For example when you right-float in TwentyNineteen, it pulls out the block from the main column. If the block is small enough, text won't even wrap around the item. That is a distinct layout change which themes are welcome to do with the alignment buttons.

I've been fiddling with CSS grid lately. This grid affords us some interesting opportunities with regards to layout, that haven't been very utilized with classic webdesign layouts. In a CSS grid world, the classic centered main column is simply a container that is attached to a grid start column and a grid width column. Wide and fullwide would simply attach to different grid start and end columns, same as the left and right alignments.

I'm not trying to rewrite history — the alignments most definitely have always been alignments. But I imagine in the mind of a user, they change the layout, and in that mindset, making a block span the full width is a related layout change.

So as a user interface, the alignments box feels like an obvious place to provide layout changing affordances.

As for grouping. To me, it's strictly a way to keep things together without having to worry about losing their struture (if you will).

I quite agree with this. That was the reason for my may 23rd comment, and also the source of my may 24th comment, where despite being concerned about the additional div, it does expand the flexibility of the group as it is today, without shackling us from future explorations that change how the group works to be more generic.

Kjell actually summarizes some of the key reasoning better in his comment. The idea he's referring to is that you could consider post_content as a group that supports layouts including wide alignments. When a group supports wide alignments, content inside is in a centered column, and buttons for aligning wide or fullwide become available. Right now groups don't have such a toggle, it's all or nothing. But such a toggle would offer the best of both worlds — you could have the generic group, perhaps even by default, but you could toggle on a centered column which enabled the wide and fullwide buttons for child blocks, if you so desired. This is something to explore, but if we like this approach, the additional inner div does not block us from doing so.

Does that make sense?