-
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
Try adding an inner container to the Group block. #15210
Conversation
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. |
I'm pleased to see this. Note this approach was rejected from the original |
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 Also — if someone has a moment (assuming this ends up moving forward), I could use a hand getting these tests to pass. 🙌 |
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 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: The snapshot end-to-end tests are also failing for the same reason, they can be updated by running I'm also happy to help and can push a commit to fix those things if needed. |
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.
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.
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. |
To make theme styling easier.
68e9405
to
2a6214a
Compare
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. |
🎉 They passed! Thanks @talldan! How do we handle the block depreciation? I think that's the last bit here. |
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 We've started adding tests for all the block deprecations as part of the I'll add those in a commit now. |
Nice PR. I have some thoughts to share on this, and they originate from this train of thought here. Then yesterday I discussed them with @youknowriad and he suggested an interesting idea that I'd like to surface with a summary of our conversation here. This summary is not necessarily a blocker for the PR, but I'm sharing because I think it's important we think about where do we want to go, with wide, fullwide, and centered columns, and what role does the group block play here? In any other application that has a grouping feature, you select a bunch of things, press ⌘G, and it’s now a group. If you resize that group, content inside resizes with it. The block editor group feature is not a direct equivalent, but the behavior is still inverse: if you make a group fullwide, the content stays in a centered column. For anyone used to the former behavior, the latter is confusing. It even makes some layouts impossible, such as having a fullwide paragraph inside that group block. The block editor is built by us, we're all bloggers, we're all pro users. The "centered main column" design pattern is so ingrained in us, so you can well imagine where this behavior came from. But are we sure this should be the only behavior for the group block? Are we even sure it should be the default behavior? The most excellent counterpoint to this train of thought is: what does "full width" alignment mean in a block inside a group that is already full-width? To parse that one, let's step back a little bit and remember that we're dealing with HTML. Blocks are divs, divs are born block level and take up all the available horizontal space. The full-wide buttons are intended to "break out" of those boundaries. But the counterpoint is solid: if there's nothing to break out of, should they be there? If we consider wide and fullwide as properties only of Now imagine that toggle on the group block as well. This toggle button could even have this icon, or a variant of it: Or this one: It could work like this:
The idea of thinking about post_content as a group block, and the "center column toggle" as an identical feature for both, may be important for us, as future iterations of Gutenberg are likely to include additional block areas than just post_content. I would personally love to see header and footer block areas, and such a "center column" toggle would be interesting for those too, but less so for a "sidebar" block area. Additionally, this would allow us to accomplish the following use case: Where currently only this is possible: This is a long way of asking: does this PR optimize further for the all-or-nothing center column behavior, or will it still be possible in the future to build such a center-column toggle button? For example, if in the future this nested centered column depends on the presence of a Additional questions to think about: how do we define the widths of the center column, wide, and even full-wide blocks in such a world? The above definitely suggests that fullwide inside a group block would only ever be as wide as the group itself, and not necessarily always as wide as post_content. Curious to hear your thoughts. |
If I try to translate @jasmussen's message into a list of changes to how the group block works, It would be:
What does it mean for the rool level (post_content)
|
I worked on the original PR for the If I recall, I believe my original thinking during these discussions was more inline with what @jasmussen and @youknowriad and now proposing here. We ultimately went in a different direction however. 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. 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. To confirm, I don't think these changes would be impossible. The one that seems the most tricky might be:
|
That would be awesome ❤️
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. |
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 |
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 The Gutenberg Starter theme behaves this way. Full width blocks are 100% width, and the theme uses Instead, most themes today treat In this example, the 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 |
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!
|
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! |
If we merge this PR as is, it means that the future "has-centered-container" attribute is going to be "on" by default right? |
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. 😂) |
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! |
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.
Looks solid.
I tested:
- That the deprecation still works, it does.
- Rebased locally onto master and tested that the new group/ungroup feature works, it does.
- 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 👍
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! |
I opened #16197 to track the other "non-centered" option we discussed above. 👍 |
Really pleased to see this land. Great work! 🎉 |
@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. :) |
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.
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 Does that make sense? |
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 similarwp-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.