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

Add flex layout to Buttons block and new layout type. #35819

Merged
merged 9 commits into from
Nov 3, 2021
Merged

Add flex layout to Buttons block and new layout type. #35819

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
edb0d3d
Add flex layout to Buttons block and new layout type.
tellthemachines Oct 21, 2021
191176d
Make orientation a property of flex layout
tellthemachines Oct 26, 2021
f4acac2
Update save function and add deprecation
tellthemachines Oct 26, 2021
f0127c7
Update fixtures
tellthemachines Oct 26, 2021
9bf62af
Fix php lint warnings.
tellthemachines Oct 26, 2021
ee74ef8
Fix bad conflict resolution
tellthemachines Nov 2, 2021
0105f2f
Set orientation in layout tools
tellthemachines Nov 2, 2021
ce2ffed
Improve orientation controls
tellthemachines Nov 3, 2021
45cbc76
Change orientation to opt-out
tellthemachines Nov 3, 2021
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
37 changes: 24 additions & 13 deletions lib/block-supports/layout.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,13 +69,18 @@ function gutenberg_get_layout_style( $selector, $layout, $has_block_gap_support
$style .= "$selector > * + * { margin-top: var( --wp--style--block-gap ); margin-bottom: 0; }";
}
} elseif ( 'flex' === $layout_type ) {
$layout_orientation = isset( $layout['orientation'] ) ? $layout['orientation'] : 'horizontal';

$justify_content_options = array(
'left' => 'flex-start',
'right' => 'flex-end',
'center' => 'center',
'space-between' => 'space-between',
'left' => 'flex-start',
'right' => 'flex-end',
'center' => 'center',
);

if ( 'horizontal' === $layout_orientation ) {
$justify_content_options += array( 'space-between' => 'space-between' );
}

$flex_wrap_options = array( 'wrap', 'nowrap' );
$flex_wrap = ! empty( $layout['flexWrap'] ) && in_array( $layout['flexWrap'], $flex_wrap_options, true ) ?
$layout['flexWrap'] :
Expand All @@ -89,14 +94,21 @@ function gutenberg_get_layout_style( $selector, $layout, $has_block_gap_support
$style .= 'gap: 0.5em;';
}
$style .= "flex-wrap: $flex_wrap;";
$style .= 'align-items: center;';
/**
* Add this style only if is not empty for backwards compatibility,
* since we intend to convert blocks that had flex layout implemented
* by custom css.
*/
if ( ! empty( $layout['justifyContent'] ) && array_key_exists( $layout['justifyContent'], $justify_content_options ) ) {
$style .= "justify-content: {$justify_content_options[ $layout['justifyContent'] ]};";
if ( 'horizontal' === $layout_orientation ) {
$style .= 'align-items: center;';
/**
* Add this style only if is not empty for backwards compatibility,
* since we intend to convert blocks that had flex layout implemented
* by custom css.
*/
if ( ! empty( $layout['justifyContent'] ) && array_key_exists( $layout['justifyContent'], $justify_content_options ) ) {
$style .= "justify-content: {$justify_content_options[ $layout['justifyContent'] ]};";
}
} else {
$style .= 'flex-direction: column;';
if ( ! empty( $layout['justifyContent'] ) && array_key_exists( $layout['justifyContent'], $justify_content_options ) ) {
$style .= "align-items: {$justify_content_options[ $layout['justifyContent'] ]};";
}
}
$style .= '}';

Expand Down Expand Up @@ -204,4 +216,3 @@ function( $matches ) {
remove_filter( 'render_block', 'wp_restore_group_inner_container', 10, 2 );
}
add_filter( 'render_block', 'gutenberg_restore_group_inner_container', 10, 2 );

121 changes: 86 additions & 35 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 @@ -8,7 +8,12 @@ import {
justifyRight,
justifySpaceBetween,
} from '@wordpress/icons';
import { Button, ToggleControl } from '@wordpress/components';
import {
Button,
ToggleControl,
__experimentalToggleGroupControl as ToggleGroupControl,
__experimentalToggleGroupControlOption as ToggleGroupControlOption,
} from '@wordpress/components';

/**
* Internal dependencies
Expand All @@ -17,13 +22,21 @@ import { appendSelectors } from './utils';
import useSetting from '../components/use-setting';
import { BlockControls, JustifyContentControl } from '../components';

// Used with the default, horizontal flex orientation.
const justifyContentMap = {
left: 'flex-start',
right: 'flex-end',
center: 'center',
'space-between': 'space-between',
};

// Used with the vertical (column) flex orientation.
const alignItemsMap = {
left: 'flex-start',
right: 'flex-end',
center: 'center',
};

const flexWrapOptions = [ 'wrap', 'nowrap' ];

export default {
Expand All @@ -39,6 +52,12 @@ export default {
layout={ layout }
onChange={ onChange }
/>
{ layout?.orientation && (
Copy link
Contributor

Choose a reason for hiding this comment

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

This check is interesting. It basically means that if the current layout doesn't have any orientation, do not display the control to switch "orientation". That said one can argue that the "no orientation" is just equivalent to "horizontal orientation" meaning we could just always show the toggle.

The problem with always showing the toggle is that it will show up in other blocks using the "flex" layout like the "Group" block (or its Row variation and potentially others). While one could argue that this is wrong since is a "row" right, conceptually it's not wrong. For instance, what about blocks like "Social Icons" and "Query Pagination" also using the "flex" layout, should we allow them to switch orientation? It might make sense.

So the thing here is that I'm hesitant between:

  • Keep things as they are in the PR: making the "orientation" undefined or not in the default layout value the actual flag that indicates whether to show this control or not.
  • Introduce a dedicated opt-out flag in the layout support config (and potentially only use it in the Group block)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

An opt-out flag would be nice, especially if there's only one block where we really don't want to enable vertical orientation. Perhaps something like "allowOrientation": false?

It also occurred to me that if orientation were enabled for all blocks, we wouldn't need Row as a variation of Group at all. But Group is using flow layout; would there be any issues in migrating it to flex with vertical orientation?

Copy link
Member

@ZebulanStanphill ZebulanStanphill Nov 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Well, a flexbox layout doesn't let children use float, so yes, there would definitely be issues if "flow" did not remain as an option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oooh good point. I guess we'd better leave it as it is then.

<OrientationControl
layout={ layout }
onChange={ onChange }
/>
) }
<FlexWrapControl layout={ layout } onChange={ onChange } />
</>
);
Expand All @@ -62,6 +81,7 @@ export default {
);
},
save: function FlexLayoutStyle( { selector, layout } ) {
const { orientation = 'horizontal' } = layout;
const blockGapSupport = useSetting( 'spacing.blockGap' );
const hasBlockGapStylesSupport = blockGapSupport !== null;
const justifyContent =
Expand All @@ -70,6 +90,17 @@ export default {
const flexWrap = flexWrapOptions.includes( layout.flexWrap )
? layout.flexWrap
: 'wrap';
const rowOrientation = `
flex-direction: row;
align-items: center;
justify-content: ${ justifyContent };
`;
const alignItems =
alignItemsMap[ layout.justifyContent ] || alignItemsMap.left;
const columnOrientation = `
flex-direction: column;
align-items: ${ alignItems };
`;
return (
<style>{ `
${ appendSelectors( selector ) } {
Expand All @@ -80,9 +111,7 @@ export default {
: '0.5em'
};
flex-wrap: ${ flexWrap };
align-items: center;
flex-direction: row;
justify-content: ${ justifyContent };
${ orientation === 'horizontal' ? rowOrientation : columnOrientation }
}

${ appendSelectors( selector, '> *' ) } {
Expand All @@ -91,57 +120,35 @@ export default {
` }</style>
);
},
getOrientation() {
return 'horizontal';
getOrientation( layout ) {
const { orientation = 'horizontal' } = layout;
return orientation;
},
getAlignments() {
return [];
},
};

const justificationOptions = [
{
value: 'left',
icon: justifyLeft,
label: __( 'Justify items left' ),
},
{
value: 'center',
icon: justifyCenter,
label: __( 'Justify items center' ),
},
{
value: 'right',
icon: justifyRight,
label: __( 'Justify items right' ),
},
{
value: 'space-between',
icon: justifySpaceBetween,
label: __( 'Space between items' ),
},
];
function FlexLayoutJustifyContentControl( {
layout,
onChange,
isToolbar = false,
} ) {
const { justifyContent = 'left' } = layout;
const { justifyContent = 'left', orientation = 'horizontal' } = layout;
const onJustificationChange = ( value ) => {
onChange( {
...layout,
justifyContent: value,
} );
};
const allowedControls = [ 'left', 'center', 'right' ];
if ( orientation === 'horizontal' ) {
allowedControls.push( 'space-between' );
}
if ( isToolbar ) {
return (
<JustifyContentControl
allowedControls={ [
'left',
'center',
'right',
'space-between',
] }
allowedControls={ allowedControls }
value={ justifyContent }
onChange={ onJustificationChange }
popoverProps={ {
Expand All @@ -152,6 +159,31 @@ function FlexLayoutJustifyContentControl( {
);
}

const justificationOptions = [
{
value: 'left',
icon: justifyLeft,
label: __( 'Justify items left' ),
},
{
value: 'center',
icon: justifyCenter,
label: __( 'Justify items center' ),
},
{
value: 'right',
icon: justifyRight,
label: __( 'Justify items right' ),
},
];
if ( orientation === 'horizontal' ) {
justificationOptions.push( {
value: 'space-between',
icon: justifySpaceBetween,
label: __( 'Space between items' ),
} );
}

return (
<fieldset className="block-editor-hooks__flex-layout-justification-controls">
<legend>{ __( 'Justification' ) }</legend>
Expand Down Expand Up @@ -187,3 +219,22 @@ function FlexWrapControl( { layout, onChange } ) {
/>
);
}

function OrientationControl( { layout, onChange } ) {
const { orientation = 'horizontal' } = layout;
return (
<ToggleGroupControl
label="Orientation"
value={ orientation }
onChange={ ( value ) => {
onChange( {
...layout,
orientation: value,
} );
} }
>
<ToggleGroupControlOption value="horizontal" label="Horizontal" />
<ToggleGroupControlOption value="vertical" label="Vertical" />
</ToggleGroupControl>
);
}
17 changes: 8 additions & 9 deletions packages/block-library/src/buttons/block.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,6 @@
"description": "Prompt visitors to take action with a group of button-style links.",
"keywords": [ "link" ],
"textdomain": "default",
"attributes": {
"contentJustification": {
"type": "string"
},
"orientation": {
"type": "string",
"default": "horizontal"
}
},
"supports": {
"anchor": true,
"align": [ "wide", "full" ],
Expand All @@ -25,6 +16,14 @@
"__experimentalDefaultControls": {
"blockGap": true
}
},
"__experimentalLayout": {
"allowSwitching": false,
"allowInheriting": false,
"default": {
"type": "flex",
"orientation": "horizontal"
}
}
},
"editorStyle": "wp-block-buttons-editor",
Expand Down
76 changes: 73 additions & 3 deletions packages/block-library/src/buttons/deprecated.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,79 @@
/**
* External dependencies
*/
import classnames from 'classnames';
/**
* WordPress dependencies
*/
import { InnerBlocks } from '@wordpress/block-editor';
import { InnerBlocks, useBlockProps } from '@wordpress/block-editor';

/**
* @param {Object} attributes Block's attributes.
*/
const migrateWithLayout = ( attributes ) => {
if ( !! attributes.layout ) {
return attributes;
}

const { contentJustification, orientation } = attributes;
Copy link
Contributor

Choose a reason for hiding this comment

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

We should remove these attributes here and keep only the rest. You can also see them appear in the fixtures.

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'm not sure I understand. These are used here to migrate any legacy settings to the new layout, e.g. if there's a deprecated block with "orientation" set we use that value in layout when we update the block. Or is there another way of doing it?

In the fixtures these attributes only appear in the deprecated versions of Buttons. That should be expected, right?

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 wait, you mean I should be removing them from the updated attributes?

Copy link
Contributor

Choose a reason for hiding this comment

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

I should be removing them from the updated attributes?

Yes. We extract them here and use them for the layout but since the current version doesn't contain them, these attributes shouldn't exist at the final migrated version of the block.


const updatedAttributes = {
...attributes,
};

if ( contentJustification || orientation ) {
Object.assign( updatedAttributes, {
layout: {
type: 'flex',
justifyContent: contentJustification || 'left',
orientation: orientation || 'horizontal',
},
} );
}

return updatedAttributes;
};

const deprecated = [
{
attributes: {
contentJustification: {
type: 'string',
},
orientation: {
type: 'string',
default: 'horizontal',
},
},
supports: {
anchor: true,
align: [ 'wide', 'full' ],
__experimentalExposeControlsToChildren: true,
spacing: {
blockGap: true,
margin: [ 'top', 'bottom' ],
__experimentalDefaultControls: {
blockGap: true,
},
},
},
isEligible: ( { layout } ) => ! layout,
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should have isEligible here. The thing is that when this function is checked it gets the attributes for this deprecation. This ends up having a newly created buttons block (using its default layout) and then in migrate the orientation:horizontal exists and applies the defaults from the migration.

You can check this by adding a different default in buttons block.json like:

"default": {
	"type": "flex",
	"orientation": "vertical",
	"justifyContent": "right"
}

Add a Buttons block, save and then reload.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oooh, interesting. How does it work in the Social Links deprecation?

I can't remove isEligible altogether or the deprecation won't work, but I'll change the check to explicitly look for the contentJustification and orientation attributes, as these are the only ones we need to migrate. If they don't exist we can leave the block as is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All feedback has now been addressed in #36192. Thanks for the review!

Copy link
Contributor

Choose a reason for hiding this comment

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

How does it work in the Social Links deprecation?

Because:

The specific handling by className below is needed because itemsJustification
was introduced in https://github.com/WordPress/gutenberg/pull/28980/files and wasn't
declared in block.json.

So I didn't remove any attributes there.

migrate: migrateWithLayout,
save( { attributes: { contentJustification, orientation } } ) {
return (
<div
{ ...useBlockProps.save( {
className: classnames( {
[ `is-content-justification-${ contentJustification }` ]: contentJustification,
'is-vertical': orientation === 'vertical',
} ),
} ) }
>
<InnerBlocks.Content />
</div>
);
},
},
{
supports: {
align: [ 'center', 'left', 'right' ],
Expand All @@ -20,7 +90,7 @@ const deprecated = [
return align && [ 'center', 'left', 'right' ].includes( align );
},
migrate( attributes ) {
return {
return migrateWithLayout( {
...attributes,
align: undefined,
// Floating Buttons blocks shouldn't have been supported in the
Expand All @@ -30,7 +100,7 @@ const deprecated = [
// As for center-aligned Buttons blocks, the content justification
// equivalent will create an identical end result in most cases.
contentJustification: attributes.align,
};
} );
},
},
];
Expand Down
Loading