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

Buttons: Add variations for vertical layout #27297

Merged
merged 5 commits into from
Dec 2, 2020
Merged

Buttons: Add variations for vertical layout #27297

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
92e24f2
Add buttons variations for vertical layout
ntsekouras Nov 26, 2020
06b9744
fix vertical appender styles
ntsekouras Nov 26, 2020
a7798f7
remove box-shadow from buttons on focus
ntsekouras Nov 26, 2020
6d50534
regenerate fixtures
ntsekouras Nov 27, 2020
0dcb174
move focus styling to Buttons
ntsekouras Dec 2, 2020
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
2 changes: 1 addition & 1 deletion packages/block-library/src/button/editor.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
}

// Add outline to button on focus to indicate focus-state
&:focus {
&:focus:not(.wp-block-buttons &) {
ntsekouras marked this conversation as resolved.
Show resolved Hide resolved
box-shadow: 0 0 0 1px $white, 0 0 0 3px var(--wp-admin-theme-color);

// Windows' High Contrast mode will show this outline, but not the box-shadow.
Expand Down
4 changes: 4 additions & 0 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 @@ -5,6 +5,10 @@
"attributes": {
"contentJustification": {
"type": "string"
},
"orientation": {
"type": "string",
"default": "horizontal"
}
},
"supports": {
Expand Down
5 changes: 3 additions & 2 deletions packages/block-library/src/buttons/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,18 +23,19 @@ const ALLOWED_BLOCKS = [ buttonBlockName ];
const BUTTONS_TEMPLATE = [ [ 'core/button' ] ];

function ButtonsEdit( {
attributes: { contentJustification },
attributes: { contentJustification, orientation },
setAttributes,
} ) {
const blockProps = useBlockProps( {
className: classnames( {
[ `is-content-justification-${ contentJustification }` ]: contentJustification,
'is-vertical': orientation === 'vertical',
} ),
} );
const innerBlocksProps = useInnerBlocksProps( blockProps, {
allowedBlocks: ALLOWED_BLOCKS,
template: BUTTONS_TEMPLATE,
orientation: 'horizontal',
orientation,
__experimentalLayout: {
type: 'default',
alignments: [],
Expand Down
27 changes: 17 additions & 10 deletions packages/block-library/src/buttons/editor.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,25 @@
.wp-block-buttons > .wp-block {
// Override editor auto block margins.
margin-left: 0;
}

.wp-block > .wp-block-buttons {
display: flex;
flex-wrap: wrap;
}

.wp-block-buttons {
> .wp-block {
// Override editor auto block margins.
margin-left: 0;
margin-top: $button-margin;
}
> .block-list-appender {
display: inline-flex;
align-items: center;
}
&.is-vertical {
> .block-list-appender .block-list-appender__toggle {
justify-content: flex-start;
}
}
}

.wp-block[data-align="center"] > .wp-block-buttons {
align-items: center;
justify-content: center;
Expand All @@ -16,8 +28,3 @@
.wp-block[data-align="right"] > .wp-block-buttons {
justify-content: flex-end;
}

.wp-block-buttons > .block-list-appender {
display: inline-flex;
align-items: center;
}
2 changes: 2 additions & 0 deletions packages/block-library/src/buttons/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import transforms from './transforms';
import edit from './edit';
import metadata from './block.json';
import save from './save';
import variations from './variations';

const { name } = metadata;

Expand Down Expand Up @@ -40,4 +41,5 @@ export const settings = {
transforms,
edit,
save,
variations,
};
5 changes: 4 additions & 1 deletion packages/block-library/src/buttons/save.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,15 @@ import classnames from 'classnames';
*/
import { InnerBlocks, useBlockProps } from '@wordpress/block-editor';

export default function save( { attributes: { contentJustification } } ) {
export default function save( {
attributes: { contentJustification, orientation },
} ) {
return (
<div
{ ...useBlockProps.save( {
className: classnames( {
[ `is-content-justification-${ contentJustification }` ]: contentJustification,
'is-vertical': orientation === 'vertical',
} ),
} ) }
>
Expand Down
21 changes: 21 additions & 0 deletions packages/block-library/src/buttons/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,17 @@
flex-direction: row;
flex-wrap: wrap;

&.is-vertical {
flex-direction: column;
> .wp-block-button {
/*rtl:ignore*/
margin-right: 0;
&:last-child {
margin-bottom: 0;
}
}
}

// Increased specificity to override blocks default margin.
> .wp-block-button {
display: inline-block;
Expand All @@ -18,10 +29,16 @@

&.is-content-justification-left {
Copy link
Member

@ZebulanStanphill ZebulanStanphill Nov 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Perhaps this className should have been is-content-justification-start, because "left" doesn't make sense in the vertical context, and the actual value of align-items that gets set is flex-start anyway. Same for is-content-justification-right which should probably be is-content-justification-end.

Copy link
Member

Choose a reason for hiding this comment

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

Just realized the above comment was incorrect, since I got horizontal/vertical controls confused in my head.

Anyway, there is an actual problem here, though: this class is being used to align-items for vertical Buttons blocks, but the class name implies it can only set justify-content. Additionally, the justify-content styles are being applied to both vertical and horizontal orientations. That's not technically correct.

Copy link
Contributor Author

@ntsekouras ntsekouras Nov 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Why not? You can have vertically stacked buttons and align them for example to the right. What do you propose exactly? If it's about the term/component names, do you think it's okay to have in a follow up PR?

Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't be setting justify-content for the vertical case... only align-items. But also, the class "is-content-justification-xxx" is kind of confusing if it's actually applying item alignment in the vertical cases. I think ideally, we'd want to tackle this stuff sooner rather than later to reduce the number of deprecations and legacy styles we have to keep on the front-end.

Copy link
Member

Choose a reason for hiding this comment

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

The problem here is basically that I named the is-content-justification-xxx classes with the assumption that they would only be used to apply different justify-content styles, but that's no longer true in this case.

Maybe a new class name that can work both cases would be appropriate. I'm not sure what the correct term is, though.

Or maybe the solution is to apply different classes for the vertical cases, e.g. is-item-alignment-xxx. I'm not sure.

Copy link
Member

@ZebulanStanphill ZebulanStanphill Dec 1, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

personally I think of alignment as the positioning of the block itself, in this case the container block, and justificiation as the positioning of content inside.

It's worth remembering that in addition to align-items and justify-content, there's also align-content and justify-items, though justify-items doesn't do anything in Flexbox.

By looking at how these properties work, we can learn the following about how CSS is using these terms:

In a horizontally-flowing container, "align" refers to vertical positioning and "justify" refers to horizontal positioning. In a vertically-flowing container, "align" refers to horizontal positioning and "justify" refers to vertical positioning. In other words, "justify" refers to positioning along the primary axis that content is flowing, and "align" refers to the secondary (AKA "cross") axis.

"content" applies to all the children as a single block within the container, and "items" applies to each child as an individual unit.

Technically, the kind of alignment/justification we want for the horizontal variant is actually conceptually different from the control we want on the vertical variant. Technically, justify-content's cross-axis equivalent is align-content, not align-items. 😖

Copy link
Member

@ZebulanStanphill ZebulanStanphill Dec 1, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

If I had to come up with a better class name than what we're using right now, I guess something like is-content-horizontal-placement-xxx might work. I'm drawing the "placement" term here from the shorthand properties place-content and place-items. The CSS "placement" term is apparently its generic way of referring to both "alignment" and "justification".

Of course, in my suggested class name here, the meaning of "content" is slightly different from what CSS uses it for, since in the case of my suggested class name, it can refer to either CSS's "content" or "items". But I guess it's good enough for our purposes?

Copy link
Contributor

Choose a reason for hiding this comment

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

For me it's too late to change the class names, this is already shipped and themes might be relying on it. The renaming doesn't seem to add a lot of value.

Copy link
Member

Choose a reason for hiding this comment

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

As far as I can tell, it's only shipped in the plugin, not core, right? Does that change anything?

Copy link
Contributor

Choose a reason for hiding this comment

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

Does that change anything?

It does, we have more luxury to change things that way but are we certain of that? It's also a bit late if it's in WP 5.6 as we're on RC2 and the stable is supposed to happen next week.

We need to do a few things if we want to rename the classnames:

  • try to check usage of the classname in the plugins/theme repository
  • backport the fix to WP 5.6 (so it needs to be in its own separate PR and not block this one)

cc @tellthemachines

justify-content: flex-start;
&.is-vertical {
align-items: flex-start;
}
}

&.is-content-justification-center {
justify-content: center;
&.is-vertical {
align-items: center;
}
}

&.is-content-justification-right {
Expand All @@ -38,6 +55,10 @@
margin-left: 0;
}
}

&.is-vertical {
align-items: flex-end;
}
}

// Kept for backward compatibiity.
Expand Down
24 changes: 24 additions & 0 deletions packages/block-library/src/buttons/variations.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
/**
* WordPress dependencies
*/
import { __ } from '@wordpress/i18n';

const variations = [
{
name: 'buttons-horizontal',
isDefault: true,
title: __( 'Horizontal' ),
description: __( 'Buttons shown in a row.' ),
attributes: { orientation: 'horizontal' },
scope: [ 'transform' ],
},
{
name: 'buttons-vertical',
title: __( 'Vertical' ),
description: __( 'Buttons shown in a column.' ),
attributes: { orientation: 'vertical' },
scope: [ 'transform' ],
},
];

export default variations;
1 change: 1 addition & 0 deletions packages/e2e-tests/fixtures/blocks/core__buttons.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
"isValid": true,
"attributes": {
"contentJustification": "center",
"orientation": "horizontal",
"align": "wide"
},
"innerBlocks": [
Expand Down
4 changes: 3 additions & 1 deletion packages/e2e-tests/fixtures/blocks/core_buttons__simple__deprecated.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,9 @@
"clientId": "_clientId_0",
"name": "core/buttons",
"isValid": true,
"attributes": {},
"attributes": {
"orientation": "horizontal"
},
"innerBlocks": [
{
"clientId": "_clientId_0",
Expand Down