-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Add block spacing gap config to theme.json and add support for this CSS variable to the "flow/default" layout. #33812
Changes from all commits
cbec7d2
7e580c1
0a1a049
2b39b04
a694bab
80fb0b6
c2e1a00
904742c
cd57781
43f2647
06b9dc0
fe2f26a
f862f1d
7cdb879
6172295
2339961
3cf82d2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -58,18 +58,21 @@ function gutenberg_get_layout_style( $selector, $layout ) { | |
$style .= '}'; | ||
|
||
$style .= "$selector > .alignwide { max-width: " . esc_html( $wide_max_width_value ) . ';}'; | ||
|
||
$style .= "$selector .alignfull { max-width: none; }"; | ||
} | ||
|
||
$style .= "$selector .alignleft { float: left; margin-right: 2em; }"; | ||
$style .= "$selector .alignright { float: right; margin-left: 2em; }"; | ||
$style .= "$selector > * + * { margin-top: var( --wp--style--block-gap ); margin-bottom: 0; }"; | ||
} elseif ( 'flex' === $layout_type ) { | ||
$style = "$selector {"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In my tests the block gap works inside a group block with the default layout but not with the flex layout I think it is expected for now given the PR title. But I guess we can easily also add a rule on the flex layout that uses the --wp--style--block-gap variable. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I actually added it to the client but forgot to add it to the server :) |
||
$style .= 'display: flex;'; | ||
$style .= 'column-gap: 0.5em;'; | ||
$style .= 'gap: var( --wp--style--block-gap, 0.5em );'; | ||
$style .= 'flex-wrap: wrap;'; | ||
$style .= 'align-items: center;'; | ||
$style .= '}'; | ||
|
||
$style .= "$selector > * { margin: 0; }"; | ||
} | ||
|
||
return $style; | ||
|
@@ -83,13 +86,14 @@ function gutenberg_get_layout_style( $selector, $layout ) { | |
* @return string Filtered block content. | ||
*/ | ||
function gutenberg_render_layout_support_flag( $block_content, $block ) { | ||
$block_type = WP_Block_Type_Registry::get_instance()->get_registered( $block['blockName'] ); | ||
$support_layout = gutenberg_block_has_support( $block_type, array( '__experimentalLayout' ), false ); | ||
if ( ! $support_layout || ! isset( $block['attrs']['layout'] ) ) { | ||
$block_type = WP_Block_Type_Registry::get_instance()->get_registered( $block['blockName'] ); | ||
$support_layout = gutenberg_block_has_support( $block_type, array( '__experimentalLayout' ), false ); | ||
$has_innner_blocks = count( $block['innerBlocks'] ) > 0; | ||
if ( ! $support_layout && ! $has_innner_blocks ) { | ||
return $block_content; | ||
} | ||
|
||
$used_layout = $block['attrs']['layout']; | ||
$used_layout = isset( $block['attrs']['layout'] ) ? $block['attrs']['layout'] : array(); | ||
if ( isset( $used_layout['inherit'] ) && $used_layout['inherit'] ) { | ||
$tree = WP_Theme_JSON_Resolver_Gutenberg::get_merged_data( array(), 'theme' ); | ||
$default_layout = _wp_array_get( $tree->get_settings(), array( 'layout' ) ); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -61,8 +61,9 @@ class WP_Theme_JSON_Gutenberg { | |
'text' => null, | ||
), | ||
'spacing' => array( | ||
'margin' => null, | ||
'padding' => null, | ||
'margin' => null, | ||
'padding' => null, | ||
'blockGap' => null, | ||
), | ||
'typography' => array( | ||
'fontFamily' => null, | ||
|
@@ -233,6 +234,7 @@ class WP_Theme_JSON_Gutenberg { | |
'padding-right' => array( 'spacing', 'padding', 'right' ), | ||
'padding-bottom' => array( 'spacing', 'padding', 'bottom' ), | ||
'padding-left' => array( 'spacing', 'padding', 'left' ), | ||
'--wp--style--block-gap' => array( 'spacing', 'blockGap' ), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The gap works a little bit differently from the other properties because there is inheritance. If I apply margin to a block the descendent blocks don't get that margin but if I apply a gap the descendants even if not direct descendants get that gap because of how CSS variables work. I guess we should probably document somewhere that gap is inherited/propagated contrary to what happens with a margin or padding. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good observation, I went back and forth here. I think inheriting is good here but I'm still hesitant a bit.. WDYT? Agreed about the documentation. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think for this case inheritance makes sense, otherwise, something like a global gap would have almost no noticeable effect. As the direct descendants in a global context are header a content area and a footer most of the time. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. unless I'm missing something, I didn't find a reference for all the styles that we can add the theme.json configs. Maybe we should consider creating one separately. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps within the "styles" section? https://developer.wordpress.org/block-editor/how-to-guides/themes/theme-json/#styles There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep, I think that would be a good place for it. |
||
'text-decoration' => array( 'typography', 'textDecoration' ), | ||
'text-transform' => array( 'typography', 'textTransform' ), | ||
); | ||
|
@@ -835,6 +837,10 @@ private function get_block_styles( $style_nodes, $setting_nodes ) { | |
$selector = $metadata['selector']; | ||
$declarations = self::compute_style_properties( $node ); | ||
$block_rules .= self::to_ruleset( $selector, $declarations ); | ||
|
||
if ( self::ROOT_BLOCK_SELECTOR === $selector ) { | ||
$block_rules .= '.wp-site-blocks > * + * { margin-top: var( --wp--style--block-gap ); margin-bottom: 0; }'; | ||
jorgefilipecosta marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
} | ||
|
||
$preset_rules = ''; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -236,5 +236,8 @@ | |
} | ||
} | ||
} | ||
}, | ||
"styles": { | ||
"spacing": { "blockGap": "24px" } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm noticing an issue if I set a gap for 60px. And I add two top-level paragraphs in a post. On the front end, the gap between paragraphs is 60px, on the template editor too but on the post editor, the gap is not the 60px. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Which theme are you using, I can take a look. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The tt1-blocks from theme-experiments repo. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For me it works properly 🤔 |
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,7 +2,5 @@ | |
&.has-background { | ||
// Matches paragraph Block padding | ||
padding: $block-bg-padding--v $block-bg-padding--h; | ||
margin-top: 0; | ||
margin-bottom: 0; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was conflicting with the this consistent margin styling. @jasmussen maybe you know where in our code base we define the block margins and whether we should move these to classic.css now. |
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -233,7 +233,14 @@ export default function VisualEditor( { styles } ) { | |
</div> | ||
) } | ||
<RecursionProvider> | ||
<BlockList __experimentalLayout={ layout } /> | ||
<BlockList | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm catching up on some layout/template editor things so I may miss tons of context: the effect of this seems that we'll show the block gap everywhere (front, site editor, post editor with template editor) except in the post editor when the theme doesn't have a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the idea of the change here is that the editor Dom should mimic the frontend and in the case of the "post editor", when we're in "template mode" we're showing the whole template meaning the canvas wrapper need to have the Incidentally, this has the effect of making sure the newly added style targeting the top level items to apply the "gap" get applied properly in the template mode. |
||
className={ | ||
isTemplateMode | ||
? 'wp-site-blocks' | ||
: undefined | ||
} | ||
__experimentalLayout={ layout } | ||
/> | ||
</RecursionProvider> | ||
</MaybeIframe> | ||
</motion.div> | ||
|
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.
Not related to this PR but a possible improvement for the future: Rules like $selector .alignleft { float: left; margin-right: 2em; }, and $selector > * + * { margin-top: var( --wp--style--block-gap ); margin-bottom: 0; } etc don't rely on any user value. If we have 10 group blocks on a page all with the same layout we repeat these rules as inline styles 10 times. Could we just have a class with these styles not relying on user values one time and then just inject the class when needed? The way the styles would only be added one time.
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 thought about this a few time it would force us to have generic
.container-flow
classNames. Worth a try I think but we should be careful about these extra classes.