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

Layout: Add default fallback gap value in block.json, and set to 2em for Columns blocks #41098

Merged
merged 6 commits into from
May 20, 2022
Merged

Layout: Add default fallback gap value in block.json, and set to 2em for Columns blocks #41098

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
56152c6
Layout: Try adding a default fallback gap value in block.json
andrewserong May 17, 2022
a9dd741
Implement in the editor, too
andrewserong May 17, 2022
a2cc158
Try moving the default value to sit under the blockGap key in spacing…
andrewserong May 19, 2022
426f67a
Update doc comment
andrewserong May 19, 2022
e779a9f
Add explanatory comments
andrewserong May 19, 2022
7774b7c
Add __experimental prefix to default property.
andrewserong May 19, 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
15 changes: 9 additions & 6 deletions lib/block-supports/layout.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,11 @@ function gutenberg_register_layout_support( $block_type ) {
* @param boolean $has_block_gap_support Whether the theme has support for the block gap.
* @param string $gap_value The block gap value to apply.
* @param boolean $should_skip_gap_serialization Whether to skip applying the user-defined value set in the editor.
* @param string $fallback_gap_value The block gap value to apply.
*
* @return string CSS style.
*/
function gutenberg_get_layout_style( $selector, $layout, $has_block_gap_support = false, $gap_value = null, $should_skip_gap_serialization = false ) {
function gutenberg_get_layout_style( $selector, $layout, $has_block_gap_support = false, $gap_value = null, $should_skip_gap_serialization = false, $fallback_gap_value = '0.5em' ) {
$layout_type = isset( $layout['type'] ) ? $layout['type'] : 'default';

$style = '';
Expand Down Expand Up @@ -102,14 +103,14 @@ function gutenberg_get_layout_style( $selector, $layout, $has_block_gap_support
$style .= 'display: flex;';
if ( $has_block_gap_support ) {
if ( is_array( $gap_value ) ) {
$gap_row = isset( $gap_value['top'] ) ? $gap_value['top'] : '0.5em';
$gap_column = isset( $gap_value['left'] ) ? $gap_value['left'] : '0.5em';
$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;
}
$gap_style = $gap_value && ! $should_skip_gap_serialization ? $gap_value : 'var( --wp--style--block-gap, 0.5em )';
$gap_style = $gap_value && ! $should_skip_gap_serialization ? $gap_value : "var( --wp--style--block-gap, $fallback_gap_value )";
$style .= "gap: $gap_style;";
} else {
$style .= 'gap: 0.5em;';
$style .= "gap: $fallback_gap_value;";
}

$style .= "flex-wrap: $flex_wrap;";
Expand Down Expand Up @@ -184,10 +185,12 @@ function gutenberg_render_layout_support_flag( $block_content, $block ) {
$gap_value = $gap_value && preg_match( '%[\\\(&=}]|/\*%', $gap_value ) ? null : $gap_value;
}

$fallback_gap_value = _wp_array_get( $block_type->supports, array( 'spacing', '__experimentalDefaultValues', 'blockGap' ), '0.5em' );

// If a block's block.json skips serialization for spacing or spacing.blockGap,
// don't apply the user-defined value to the styles.
$should_skip_gap_serialization = gutenberg_should_skip_block_supports_serialization( $block_type, 'spacing', 'blockGap' );
$style = gutenberg_get_layout_style( ".$class_name", $used_layout, $has_block_gap_support, $gap_value, $should_skip_gap_serialization );
$style = gutenberg_get_layout_style( ".$class_name", $used_layout, $has_block_gap_support, $gap_value, $should_skip_gap_serialization, $fallback_gap_value );
// This assumes the hook only applies to blocks with a single wrapper.
// I think this is a reasonable limitation for that particular hook.
$content = preg_replace(
Expand Down
14 changes: 11 additions & 3 deletions packages/block-editor/src/layouts/flex.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import {
arrowDown,
Copy link
Contributor

Choose a reason for hiding this comment

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

what about flow.js? no fallback support there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I noticed that too when I went to work on this feature — the Flow layout currently doesn't use any fallback value at all, so I didn't inject one where there isn't one already (both in layout.php and flow.js). From looking over the code again, it appears that blockGap styles in the Flow layout are only ever output if the blockGap has been opted into, in which case there'll always be a value, so a fallback value will never be reached. E.g. in the following block:

gutenberg/packages/block-editor/src/layouts/flow.js

Line 168 in 6de1698

if ( hasBlockGapStylesSupport ) {

It was the Flex layout that currently has a 0.5em hard-coded fallback, and where I think this feature is useful based on the current implementations.

But, please do let me know if I missed something here! 🙂

} from '@wordpress/icons';
import { Button, ToggleControl, Flex, FlexItem } from '@wordpress/components';
import { getBlockSupport } from '@wordpress/blocks';

/**
* Internal dependencies
Expand Down Expand Up @@ -109,14 +110,21 @@ export default {
save: function FlexLayoutStyle( { selector, layout, style, blockName } ) {
const { orientation = 'horizontal' } = layout;
const blockGapSupport = useSetting( 'spacing.blockGap' );
const fallbackValue =
getBlockSupport( blockName, [
'spacing',
'__experimentalDefaultValues',
'blockGap',
] ) || '0.5em';

const hasBlockGapStylesSupport = blockGapSupport !== null;
// If a block's block.json skips serialization for spacing or spacing.blockGap,
// don't apply the user-defined value to the styles.
const blockGapValue =
style?.spacing?.blockGap &&
! shouldSkipSerialization( blockName, 'spacing', 'blockGap' )
? getGapCSSValue( style?.spacing?.blockGap, '0.5em' )
: 'var( --wp--style--block-gap, 0.5em )';
? getGapCSSValue( style?.spacing?.blockGap, fallbackValue )
: `var( --wp--style--block-gap, ${ fallbackValue } )`;
const justifyContent =
justifyContentMap[ layout.justifyContent ] ||
justifyContentMap.left;
Expand All @@ -143,7 +151,7 @@ export default {
${ appendSelectors( selector ) } {
display: flex;
flex-wrap: ${ flexWrap };
gap: ${ hasBlockGapStylesSupport ? blockGapValue : '0.5em' };
gap: ${ hasBlockGapStylesSupport ? blockGapValue : fallbackValue };
${ orientation === 'horizontal' ? rowOrientation : columnOrientation }
}

Expand Down
3 changes: 3 additions & 0 deletions packages/block-library/src/columns/block.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,9 @@
"padding": true,
"__experimentalDefaultControls": {
"padding": true
},
"__experimentalDefaultValues": {
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if it makes sense to use the proposed block.json changes from https://github.com/WordPress/gutenberg/pull/34180/files#diff-dba084b81a05295345ce9149af1a17ab073ddef43a393e851bcd3bdae85d3f32R81 for default values. (PR: Prototype: merge block CSS with theme.json styles #34180)

I don't know how far that particular investigation will go, but it could save some lines to "join forces"
cc @oandregal

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, neat idea!

There's quite a lot of similarity here — for the case of blockGap we only want to use the value as a fallback value if none is set anywhere else, rather than always outputting it, so that might be a subtle difference between a default / fallback value and a value set in blockStyles. But keen for feedback or ideas on how to combine things if you have any @oandregal!

I suppose the caveat here is that this PR will likely need to be backported to 6.0 RC whereas it looks like #34180 is a bit more forward-looking and something potentially for 6.1?

Copy link
Member

Choose a reason for hiding this comment

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

I suppose the caveat here is that this PR will likely need to be backported to 6.0 RC whereas it looks like #34180 is a bit more forward-looking and something potentially for 6.1?

Ah, very good point.

__experimentalDefaultValues does also do what it says on the tin as you say.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just blockGap: { default: "something" } instead of blockGap: true? (We do these object configs for a lot of block supports already)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The individual feature properties of the spacing support are currently boolean | array and the array is treated as a list of sides that are opted into, with logic like the following function (and the below useIsDimensionsSupportValid:

gutenberg/packages/block-editor/src/hooks/dimensions.js

Line 167 in 2ce8e8d

export function useCustomSides( blockName, feature ) {

It'd be a more complex change to refactor those props to also support an object (with a nested sides key), so I thought using a parallel object to set default values might make for a cleaner change given that this PR could be backported. There's a precedent in the Layout support, too, which has a default object and then within it, it specifies the properties:

gutenberg/packages/block-library/src/columns/block.json

Line 45 in 56152c6

"default": {

We could always rename this from __experimentalDefaultValues to default, and it'd look like the following:

		"spacing": {
			"blockGap": true,
			"margin": [ "top", "bottom" ],
			"padding": true,
			"__experimentalDefaultControls": {
				"padding": true
			},
			"default": {
				"blockGap": "2em"
			}
		},

What do you think?

Copy link
Member

@ramonjd ramonjd May 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Also we'd have to find a model to define default side values too, right? Well, maybe not initially. Still that would involve some data massaging I suppose.

"margin": [ "top", "bottom" ]

To something like

"margin": { default: { "top": "1em", ... } }

🤷

The individual feature properties of the spacing support are currently boolean | array and the array is treated as a list of sides that are opted into

It's possibly not the cleanest way, but I guess we could check for the existence of a default key (?) if we had to?

	if (
		! support ||
		typeof support[ feature ] === 'boolean' ||
		!! support?.[ feature ]?.default
	) {
		return;
	}

I'm not saying it's pretty, and I kinda also wonder if the complexity is worth it.

Copy link
Contributor

Choose a reason for hiding this comment

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

There's a precedent in the Layout support, too, which has a default object and then within it, it specifies the properties:

Yeah, that's the one I was thinking about, since you can also do. __experimentalLayout: true (a defined object is the same thing as "true"). I don't think we should bother changing the others (padding, margin) now, but would be good to decide on an approach. I'd vote for the object approach personally but wouldn't mind the alternative used here.

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 for the feedback here! It turns out it wasn't as complex to deal with as I'd thought given that we only appear to look up which sides are opted into in the one place (useCustomSides).

In a2cc158, I've gone with the approach of:

		"spacing": {
			"blockGap": {
				"default": "2em"
			},

And we can also specify a sides key, too, in that object if we need to.

"blockGap": "2em"
}
},
"__experimentalLayout": {
Expand Down