Skip to content
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.

Sign up for GitHub

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

Jump to bottom

BlockGap: Add support for spacing presets #43296

Merged
merged 6 commits into from
Aug 18, 2022
Merged

BlockGap: Add support for spacing presets #43296

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2d5ff2f
BlockGap: Add support for spacing presets
andrewserong Aug 17, 2022
8b978d1
Add JS implementation
andrewserong Aug 17, 2022
2f1d687
Fix output for root CSS variable
andrewserong Aug 18, 2022
a7562f1
Add JS tests
andrewserong Aug 18, 2022
55a62f4
Add PHP test
andrewserong Aug 18, 2022
8bfbd94
For consistency, use 0.5em fallback gap in sides that don't have a ga…
andrewserong Aug 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 22 additions & 5 deletions lib/block-supports/layout.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,13 @@ function gutenberg_get_layout_style( $selector, $layout, $has_block_gap_support
$gap_value = isset( $gap_value['top'] ) ? $gap_value['top'] : null;
}
if ( $gap_value && ! $should_skip_gap_serialization ) {
// Get spacing CSS variable from preset value if provided.
if ( is_string( $gap_value ) && strpos( $gap_value, 'var:preset|spacing|' ) !== false ) {
$index_to_splice = strrpos( $gap_value, '|' ) + 1;
$slug = _wp_to_kebab_case( substr( $gap_value, $index_to_splice ) );
$gap_value = "var(--wp--preset--spacing--$slug)";
}

array_push(
$layout_styles,
array(
Expand Down Expand Up @@ -150,12 +157,22 @@ function gutenberg_get_layout_style( $selector, $layout, $has_block_gap_support
);
}

if ( $has_block_gap_support ) {
if ( is_array( $gap_value ) ) {
$gap_row = isset( $gap_value['top'] ) ? $gap_value['top'] : $fallback_gap_value;
$gap_column = isset( $gap_value['left'] ) ? $gap_value['left'] : $fallback_gap_value;
$gap_value = $gap_row === $gap_column ? $gap_row : $gap_row . ' ' . $gap_column;
if ( $has_block_gap_support && isset( $gap_value ) ) {
$combined_gap_value = '';
$gap_sides = is_array( $gap_value ) ? array( 'top', 'left' ) : array( 'top' );

foreach ( $gap_sides as $gap_side ) {
$process_value = is_string( $gap_value ) ? $gap_value : _wp_array_get( $gap_value, array( $gap_side ), $fallback_gap_value );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like _wp_array_get is returning null for null values.

So, an incoming $gap_value of array(2) { ["top"]=> NULL ["left"]=> string(4) "38px" }

Will result in a $combined_gap_value of 38px.

Steps to reproduce:

  1. Insert social links block
  2. Add a block gap value for one side only.

The editor produces the following CSS:

    gap: var(--wp--preset--spacing--20) 92px;

The frontend

gap: 38px;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, ignore this. I refreshed the editor and I can't reproduce any more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤭

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, cool. We can always do more detailed testing with this behaviour once we've got the spacing controls in there, I imagine it'll make testing a little easier since we'll be doing less manual fiddling with markup values 🤔

Thanks for all the testing here!

// Get spacing CSS variable from preset value if provided.
if ( is_string( $process_value ) && strpos( $process_value, 'var:preset|spacing|' ) !== false ) {
$index_to_splice = strrpos( $process_value, '|' ) + 1;
$slug = _wp_to_kebab_case( substr( $process_value, $index_to_splice ) );
$process_value = "var(--wp--preset--spacing--$slug)";
}
$combined_gap_value .= "$process_value ";
}
$gap_value = trim( $combined_gap_value );

if ( $gap_value && ! $should_skip_gap_serialization ) {
$layout_styles[] = array(
'selector' => $selector,
Expand Down
14 changes: 7 additions & 7 deletions lib/compat/wordpress-6.1/class-wp-theme-json-6-1.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -793,8 +793,6 @@ function( $pseudo_selector ) use ( $selector ) {
}

if ( static::ROOT_BLOCK_SELECTOR === $selector ) {
$block_gap_value = _wp_array_get( $this->theme_json, array( 'styles', 'spacing', 'blockGap' ), '0.5em' );

if ( $use_root_padding ) {
$block_rules .= '.wp-site-blocks { padding-top: var(--wp--style--root--padding-top); padding-bottom: var(--wp--style--root--padding-bottom); }';
$block_rules .= '.has-global-padding { padding-right: var(--wp--style--root--padding-right); padding-left: var(--wp--style--root--padding-left); }';
Expand All @@ -808,8 +806,10 @@ function( $pseudo_selector ) use ( $selector ) {

$has_block_gap_support = _wp_array_get( $this->theme_json, array( 'settings', 'spacing', 'blockGap' ) ) !== null;
if ( $has_block_gap_support ) {
$block_rules .= '.wp-site-blocks > * { margin-block-start: 0; margin-block-end: 0; }';
$block_rules .= ".wp-site-blocks > * + * { margin-block-start: $block_gap_value; }";
$block_gap_value = static::get_property_value( $this->theme_json, array( 'styles', 'spacing', 'blockGap' ) );
ramonjd marked this conversation as resolved.
Show resolved Hide resolved
$block_rules .= '.wp-site-blocks > * { margin-block-start: 0; margin-block-end: 0; }';
$block_rules .= ".wp-site-blocks > * + * { margin-block-start: $block_gap_value; }";

// For backwards compatibility, ensure the legacy block gap CSS variable is still available.
$block_rules .= "$selector { --wp--style--block-gap: $block_gap_value; }";
}
Expand Down Expand Up @@ -1308,14 +1308,14 @@ protected function get_layout_styles( $block_metadata ) {
$block_gap_value = _wp_array_get( $block_type->supports, array( 'spacing', 'blockGap', '__experimentalDefault' ), null );
}
} else {
$block_gap_value = _wp_array_get( $node, array( 'spacing', 'blockGap' ), null );
$block_gap_value = static::get_property_value( $node, array( 'spacing', 'blockGap' ) );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need this also in get_styles_for_block here

Without it the body CSS rule with custom var isn't parsed:

body {
    --wp--style--block-gap: var:preset|spacing|60;
}

Copy link
Contributor

@glendaviesnz glendaviesnz Aug 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that will be what is breaking the gallery probably as it relies on --wp--style--block-gap as the fallback gap value if no block level gap is set

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice one, thanks for catching that! I'll update 👍

Good eyes! 🦅 👀

Copy link
Contributor Author

@andrewserong andrewserong Aug 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated in 2f1d687, and the Gallery block appears to be working now. I also removed the 0.5em fallback in that case, because it'll never be reached (the rule is only output if block gap is enabled, in which case, there will always be a real blockGap value because the root theme.json provides one)

It looks like the site editor was already parsing the value for the JS side of things, but let me know if you encounter any other issues!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A question: should the following block style in theme.json have any effect?

			"core/gallery": {
				"spacing": {
					"blockGap": "var:preset|spacing|80"
				}
			}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A question: should the following block style in theme.json have any effect?

No, the Gallery block isn't supported in global styles yet since it uses an ad hoc implementation for gap. But it's flagged in the Layout design tools consistency issue.

}

// Support split row / column values and concatenate to a shorthand value.
if ( is_array( $block_gap_value ) ) {
if ( isset( $block_gap_value['top'] ) && isset( $block_gap_value['left'] ) ) {
$gap_row = $block_gap_value['top'];
$gap_column = $block_gap_value['left'];
$gap_row = static::get_property_value( $node, array( 'spacing', 'blockGap', 'top' ) );
$gap_column = static::get_property_value( $node, array( 'spacing', 'blockGap', 'left' ) );
$block_gap_value = $gap_row === $gap_column ? $gap_row : $gap_row . ' ' . $gap_column;
} else {
// Skip outputting gap value if not all sides are provided.
Expand Down
9 changes: 7 additions & 2 deletions packages/block-editor/src/hooks/gap.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import {
* Internal dependencies
*/
import { __unstableUseBlockRef as useBlockRef } from '../components/block-list/use-block-props/use-block-refs';
import { getSpacingPresetCssVar } from '../components/spacing-sizes-control/utils';
import useSetting from '../components/use-setting';
import { AXIAL_SIDES, SPACING_SUPPORT_KEY, useCustomSides } from './dimensions';
import { cleanEmptyObject } from './utils';
Expand Down Expand Up @@ -54,8 +55,12 @@ export function getGapBoxControlValueFromStyle( blockGapValue ) {

const isValueString = typeof blockGapValue === 'string';
return {
top: isValueString ? blockGapValue : blockGapValue?.top,
left: isValueString ? blockGapValue : blockGapValue?.left,
top: isValueString
? getSpacingPresetCssVar( blockGapValue )
ramonjd marked this conversation as resolved.
Show resolved Hide resolved
: getSpacingPresetCssVar( blockGapValue?.top ),
left: isValueString
? getSpacingPresetCssVar( blockGapValue )
: getSpacingPresetCssVar( blockGapValue?.left ),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not a blocker, and I'm not sure there's much to be done about it in this PR anyway.

When updating a single axial value we overwrite both top and left values:

.wp-block-social-links.is-layout-flex {
    gap: var(--wp--preset--spacing--40) var(--wp--preset--spacing--20);
}

The fallback value differs between the editor and the frontend:

.editor-styles-wrapper .wp-block-social-links.wp-container-37 {
    gap: 0 7px;
}
.wp-block-social-links.wp-container-10 {
    gap: 0.5em 7px;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, good catch. Let's look at adding some consistency there in a follow-up.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just thinking about this a little more, unfortunately, because we're concatenating to the single gap property we do need to include some kind of fallback value there rather than letting the undefined side be undefined an inherit whatever's happening globally. Do you have an opinion on which one we should consolidate to? I'm a little torn 😅

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahhh I see. Maybe 0.5em since it's already in layout.php and will show up on the frontend regardless?

I don't know either!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ramonjd, I was leaning slightly toward 0.5em anyway. In this case, I think the fallback should only be used in the output of the Flex layout, to match the behaviour in the server-rendering (I think!), so in 8bfbd94 I've updated the flex layout to call the code with passing in 0.5em — that way the fallback here is defined in the caller, so we can have other values for other potential future layout types.

Does that sound reasonable to you as a short-term fix?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds great, thanks for doing that!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works a treat, thanks!

};
}

Expand Down
22 changes: 22 additions & 0 deletions packages/block-editor/src/hooks/test/gap.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,28 @@ describe( 'gap', () => {
...blockGapValue,
} );
} );
it( 'should unwrap var: values from a string into a CSS var() function', () => {
const expectedValue = {
top: 'var(--wp--preset--spacing--60)',
left: 'var(--wp--preset--spacing--60)',
};
expect(
getGapBoxControlValueFromStyle( 'var:preset|spacing|60' )
).toEqual( expectedValue );
} );
it( 'should unwrap var: values from an object into a CSS var() function', () => {
const expectedValue = {
top: 'var(--wp--preset--spacing--20)',
left: 'var(--wp--preset--spacing--60)',
};
const blockGapValue = {
top: 'var:preset|spacing|20',
left: 'var:preset|spacing|60',
};
expect( getGapBoxControlValueFromStyle( blockGapValue ) ).toEqual(
expectedValue
);
} );
} );
describe( 'getGapCSSValue()', () => {
it( 'should return `null` if argument is falsey', () => {
Expand Down
2 changes: 1 addition & 1 deletion packages/block-editor/src/layouts/flex.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ export default {
const blockGapValue =
style?.spacing?.blockGap &&
! shouldSkipSerialization( blockName, 'spacing', 'blockGap' )
? getGapCSSValue( style?.spacing?.blockGap )
? getGapCSSValue( style?.spacing?.blockGap, '0.5em' )
: undefined;
const justifyContent = justifyContentMap[ layout.justifyContent ];
const flexWrap = flexWrapOptions.includes( layout.flexWrap )
Expand Down
33 changes: 33 additions & 0 deletions phpunit/class-wp-theme-json-test.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,39 @@ public function test_get_stylesheet_generates_layout_styles( $layout_definitions
);
}

/**
* @dataProvider data_get_layout_definitions
*
* @param array $layout_definitions Layout definitions as stored in core theme.json.
*/
public function test_get_stylesheet_generates_layout_styles_with_spacing_presets( $layout_definitions ) {
$theme_json = new WP_Theme_JSON_Gutenberg(
array(
'version' => WP_Theme_JSON_Gutenberg::LATEST_SCHEMA,
'settings' => array(
'layout' => array(
'definitions' => $layout_definitions,
),
'spacing' => array(
'blockGap' => true,
),
),
'styles' => array(
'spacing' => array(
'blockGap' => 'var:preset|spacing|60',
),
),
),
'default'
);

// Results also include root site blocks styles.
$this->assertEquals(
'body { margin: 0; }.wp-site-blocks > .alignleft { float: left; margin-right: 2em; }.wp-site-blocks > .alignright { float: right; margin-left: 2em; }.wp-site-blocks > .aligncenter { justify-content: center; margin-left: auto; margin-right: auto; }.wp-site-blocks > * { margin-block-start: 0; margin-block-end: 0; }.wp-site-blocks > * + * { margin-block-start: var(--wp--preset--spacing--60); }body { --wp--style--block-gap: var(--wp--preset--spacing--60); }body .is-layout-flow > *{margin-block-start: 0;margin-block-end: 0;}body .is-layout-flow > * + *{margin-block-start: var(--wp--preset--spacing--60);margin-block-end: 0;}body .is-layout-flex{gap: var(--wp--preset--spacing--60);}body .is-layout-flow > .alignleft{float: left;margin-inline-start: 0;margin-inline-end: 2em;}body .is-layout-flow > .alignright{float: right;margin-inline-start: 2em;margin-inline-end: 0;}body .is-layout-flow > .aligncenter{margin-left: auto !important;margin-right: auto !important;}body .is-layout-flex{display: flex;}body .is-layout-flex{flex-wrap: wrap;align-items: center;}',
$theme_json->get_stylesheet( array( 'styles' ) )
);
}

/**
* @dataProvider data_get_layout_definitions
*
Expand Down