Extract common layout styles #35867
Extract common layout styles #35867
Conversation
youknowriad
added
the
[Type] Code Quality
youknowriad
requested review from
ajitbohra,
ellatrix,
oandregal,
spacedmonkey and
TimothyBJacobs
as code owners
| } | ||
|
|
||
| $style .= "$selector .alignleft { float: left; margin-right: 2em; }"; | ||
| $style .= "$selector .alignright { float: right; margin-left: 2em; }"; | ||
| if ( $has_block_gap_support ) { |
There was a problem hiding this comment.
This dynamic part can potentially be transformed to a static style if we introduce some kind of global .has-block-gap-support classname to the root of the block editor and the frontend. Thoughts?
There was a problem hiding this comment.
Extracting these sort of styles to its own file (also from lib/class-wp-theme-json-gutenberg.php) will help with #35736 (comment)
| @@ -89,3 +89,40 @@ | |||
| width: auto; | |||
| z-index: 100000; | |||
| } | |||
|
|
|||
| // Default Flow Layout | |||
| .wp-site-blocks > *, | |||
There was a problem hiding this comment.
wp-site-blocks is the root of all block templates, potentially we could add the wp-layout-default classname to the template output instead of duplicating these selectors.
|
Size Change: +1.58 kB (0%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
|
I'm seeing some auto margins wreaking some havoc in my theme. Before: After:
It's this rule, applying now: In my structure, I have .is-root-container > [header template part] > [navigation block], but it appears that the navigation block is now receiving the |
Well in my mental modal, it seems right :) I need to understand why in trunk it was not already receiving these styles dynamically (generated classname + style) |
|
Which blocks would receive the The reason I'm asking is that the auto margins applied to direct descendants of thoes blocks is quite high (!important), and we want to be sure centering the children of said blocks is actually what we want. In the case of the navigation block, I don't understand why we'd ever center descendant children — that could be because it isn't using the new flex system quite yet, but I imagine the vertical one wouldn't either. |
I guess all blocks that have the "default layout" meaning all blocks that use "inner blocks" that didn't specify an alternative layout (like flex). Maybe we had a mechanism in place to avoid that in trunk but I can't find it. |
|
@jasmussen Found it nevermind, I'll push a fix. |
|
Thanks, I'll take a look when I can. I'm still curious though, and I feel like we've already discussed this — the centered column which is the purpose of the auto margins feels most useful for top level blocks to create the initial main column, or for full-wide top level blocks. Centering every child for nested flow content makes less sense for me. 🤔 |
|
@jasmussen For me, every container block should have a layout. That said, it's not the case anymore anyway with my last commit (for backward compatibility reasons). For me the navigation block should have a "flex layout" which doesn't have the centering. |
|
I agree with both the layout and proper flex for navigation. My only challenge here is that I'm not sure centering child blocks of any flow content layout should be the default behavior. But it may be a non-issue in practice, per your last comment. |
It doesn't matter since by default there's no max width defined. so it doesn't have any impact. It's how it works now, if we want to remove these styles from the default flow layout, we can make them part of the "dynamic" styles (only included if widths are defined) |
|
Oh I just noticed that these margins are actually only applied when widths are defined on trunk (my bad), I'll restore that behavior. |
|
Just took it for a spin again this morning, and I'm seeing no difference between trunk and this branch in my testing. Which I suppose is good? Let me know if there's anything specific I can help with from this point on, or if I should be looking at something else. |
|
I just tested this with emptytheme and our markup to test alignments and it looks like full width alignments are broken. Seems like we have lost the kind of rule. The markup I used is here. This is what I see:
|
|
I've pushed a fix here for @MaggieCabrera's bug. That said, I feel like I'm not satisfied with this PR. While in principle extracting these common styles can be good, It creates a lot of subtle issues:
I'd be personally in favor of keeping things as they are in trunk. This means more inline |
Thank you! that looks like it fixes it! |
Right now Gutenberg supports two layout types "flow" (or default) and "flex" (row block). Blocks using these layouts generate inline
<style>tags because each block can have a different layout configuration.Things that make the layout styles dynamic:
That said, there are things that are common across layout types, this PR extracts these common styles to the
common.cssstylesheet that is loaded for all themes.Conceptually speaking, I still have a preference for the "all dynamic" approach we have in trunk but in terms of "purity" of the output, this is probably a bit better so I'm personally hesitant.