Block styles: only add auto margin to root blocks #19912
Conversation
ellatrix
requested review from
talldan,
youknowriad,
ajitbohra,
jorgefilipecosta,
mkaz and
Soean
as code owners
|
It might have something to do with the group block. let's make sure there's no regressions there vCard @kjellr @jasmussen |
| @@ -35,7 +35,7 @@ | |||
| } | |||
|
|
|||
| // The base width of blocks | |||
| .edit-post-visual-editor .block-editor-block-list__block { | |||
| .edit-post-visual-editor .block-editor-block-list__layout-root > .block-editor-block-list__block { | |||
There was a problem hiding this comment.
This is an interesting approach. I was about to suggest that the automatic left and right margin is the exception to the rule, i.e. something that is unique to post content, and even the whole structure of how wide and fullwide images are handled — in fact there's a max-width too.
And so this, in a way, does treat it as the exception, but it does raise the question: is this post-content only, or could we imagine other containers in an FSE future that would want this treatment?
|
Your efforts to clean up the code have not gone unnoticed, thank you. While this PR feels like a step in the right direction, it does come with some challenges, and it may suggest that it might be a bigger undertaking. I alluded to some of the questions in this comment: https://github.com/WordPress/gutenberg/pull/19912/files#r371627753 — but it goes a bit further. Right now, wide and fullwide images work because the main column is unconstrained, and blocks inside have a max-width. Which means the odd behavior for top level blocks is just not the margins, it's also that max-width. This raises the question: should other containers be able to opt into this treatment? Post content has it, should Footer have it too? Header? How about a fullwide group block? Right now that breaks. This branch:
Master:
It also raises the followup question: should this centering and max-width even be default? Should it be something that is applied if the theme opts into wide images? |
I do remember us discussing this and yes, I believe the "group" block should have an option to say whether it has the centering which means several things:
That behavior is unrelated with theme options for me, it's more an API for InnerBlocks to defined the supported alignments. |
|
Yes, let make an option. Does the fact that it's already the default now make it more difficult to switch? Let's make it an option on |
Would it make sense to leverage the existing option for post content? add_theme_support( 'align-wide' ); |
|
Only centre the post content if the theme supports wide alignment? |
|
Basically. It seems the appropriate thing to do long term because the purpose of the centering (and max-width, don't forget that one) is to enable wider-than-the-main-column images, but not sure if we're quite ready for it — it would potentially make the editing experience worse for themes that don't opt in, or don't instead provide alternate editor styles. |
|
I believe the centering is not the responsibility of InnerBlocks or BlockList, but it's the responsibility of their parent (canvas, or group block), but BlockList should have an API to define the available widths. What @jasmussen is suggesting is that this API/prop should inherit from theme theme supports for the post_content case. |
|
I'm not quite following. My intention here is remove it right now for all inner blocks because it does more harm than good. I could add styles for the group block so content is centred. We can think about how to best handle it for the post content later? |
That's probably the simplest way forward. My concern is whether third-party block containers relied on this behavior. We could check quickly some block plugins. |
ellatrix
force-pushed
the
try/margin-auto-root
branch
from
cb9d6c6
to
ba9255c
Compare
|
Size Change: -72 B (0%) Total Size: 865 kB
ℹ️ View Unchanged
|
|
@jasmussen I retested the group block with master, and the content doesn't seem to align with the main content when using full alignment.
|
|
Ooops, never mind. I checked out the wrong branch. |
| @@ -37,7 +37,7 @@ | |||
| } | |||
|
|
|||
| // The base width of blocks | |||
There was a problem hiding this comment.
Possibly out-of-scope of this PR, but this comment doesn't really make sense. It probably did at some point before a refactor, but now it's just confusing. I think it would be a good idea to update this comment to be more helpful.
ZebulanStanphill
added
the
CSS Styling
Description
Currently block margins on the left and right is set to
autofor every block including nested blocks. This seems unintended to me. I think it should only be applied to blocks at the root level, so they are centred within the editor canvas. It shouldn't be added for nested blocks.How has this been tested?
Test nested blocks.
Screenshots
Types of changes
Checklist: