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.

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 block]: Add gap and vertical margin support #34546

Merged
merged 14 commits into from
Oct 14, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 0 additions & 1 deletion packages/block-library/src/button/editor.scss
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,6 @@
.is-selected & {
height: auto;
overflow: visible;
margin-top: $grid-unit-20;
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you know what this margin was for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was added here, looks like it was applied to the LinkControl in the block toolbar. It's been refactored since then and I'm fairly confident this isn't getting used at all anymore, actually 🤔

I have tested adding/removing a button's link and the controls look good to me.

}
}

Expand Down
55 changes: 16 additions & 39 deletions packages/block-library/src/button/style.scss
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ $blocks-block__margin: 0.5em;
}
}

// Increased specificity needed to override margins
stacimc marked this conversation as resolved.
Show resolved Hide resolved
// Increased specificity needed to override margins.
.wp-block-buttons > .wp-block-button {
&.has-custom-width {
max-width: none;
Expand All @@ -50,58 +50,35 @@ $blocks-block__margin: 0.5em;
}

&.wp-block-button__width-25 {
width: calc(25% - #{ $blocks-block__margin });
width: calc(25% - (var(--wp--style--block-gap, #{$blocks-block__margin}) * 0.75));
}

&.wp-block-button__width-50 {
width: calc(50% - #{ $blocks-block__margin });
width: calc(50% - (var(--wp--style--block-gap, #{$blocks-block__margin}) * 0.5));
}

&.wp-block-button__width-75 {
width: calc(75% - #{ $blocks-block__margin });
width: calc(75% - (var(--wp--style--block-gap, #{$blocks-block__margin}) * 0.25));
}

&.wp-block-button__width-100 {
width: calc(100% - #{ $blocks-block__margin });

&:only-child {
margin-right: 0;
width: 100%;
}
width: 100%;
flex-basis: 100%;
}
}

// If the browser supports column-gap, use that instead of the margins above.
@supports ( column-gap: #{ $blocks-block__margin } ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Speaking of browser support, it's so nice to see the important ones green in caniuse. But I also recall something about Safari that was context for adding this check in the first place. Do you recall, @kjellr?

.wp-block-buttons {

> .wp-block-button,
&.is-content-justification-right > .wp-block-button, {
// Added (duplicate) specificity needed to override the default button margin.
&.wp-block-button {
margin-right: 0;
margin-left: 0;
}
}

> .wp-block-button {
&.wp-block-button__width-25 {
width: calc(25% - #{ $blocks-block__margin * 0.75 });
}

&.wp-block-button__width-50 {
width: calc(50% - #{ $blocks-block__margin * 0.5 });
}
// For vertical buttons, gap is not factored into width calculations.
.wp-block-buttons.is-vertical > .wp-block-button {
&.wp-block-button__width-25 {
width: 25%;
}

&.wp-block-button__width-75 {
width: calc(75% - #{ $blocks-block__margin * 0.25 });
}
&.wp-block-button__width-50 {
width: 50%;
}

&.wp-block-button__width-100 {
width: auto;
flex-basis: 100%;
}
}
&.wp-block-button__width-75 {
width: 75%;
}
}

Expand Down
9 changes: 8 additions & 1 deletion packages/block-library/src/buttons/block.json
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,14 @@
"supports": {
"anchor": true,
"align": [ "wide", "full" ],
"__experimentalExposeControlsToChildren": true
"__experimentalExposeControlsToChildren": true,
"spacing": {
"blockGap": true,
"margin": ["top", "bottom" ],
"__experimentalDefaultControls": {
"blockGap": true
}
}
},
"editorStyle": "wp-block-buttons-editor",
"style": "wp-block-buttons"
Expand Down
12 changes: 8 additions & 4 deletions packages/block-library/src/buttons/editor.scss
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,16 @@ $blocks-block__margin: 0.5em;
}

.wp-block-buttons {
// Override editor auto block margins for button as well as the block appender.
> .wp-block {
// Override editor auto block margins.
margin-left: 0;
andrewserong marked this conversation as resolved.
Show resolved Hide resolved
margin-top: $blocks-block__margin;
margin-right: $blocks-block__margin;
margin: 0;
}

// Specificity needed in some themes to override editor auto block margins for the button.
> .wp-block-button.wp-block-button.wp-block-button.wp-block-button.wp-block-button {
margin: 0;
}

> .block-list-appender {
display: inline-flex;
align-items: center;
Expand Down
50 changes: 2 additions & 48 deletions packages/block-library/src/buttons/style.scss
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,11 @@ $blocks-block__margin: 0.5em;
display: flex;
flex-direction: row;
flex-wrap: wrap;
column-gap: $blocks-block__margin;
gap: var(--wp--style--block-gap, $blocks-block__margin);

&.is-vertical {
flex-direction: column;
> .wp-block-button {
/*rtl:ignore*/
margin-right: 0;
&:last-child {
margin-bottom: 0;
}
Expand All @@ -21,16 +19,7 @@ $blocks-block__margin: 0.5em;
// Increased specificity to override blocks default margin.
> .wp-block-button {
display: inline-block;
/*rtl:ignore*/
margin-left: 0;
/*rtl:ignore*/
margin-right: $blocks-block__margin;
margin-bottom: $blocks-block__margin;

&:last-child {
/*rtl:ignore*/
margin-right: 0;
}
margin: 0;
Copy link
Contributor

@andrewserong andrewserong Oct 8, 2021

Choose a reason for hiding this comment

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

Unfortunately, it looks like this rule causes a styling regression on the front end of a site running TwentyTwenty theme, as its vertical margin on individual button blocks is overridden:

Before After
image image

Is this one we should defer to the theme, rather than handling in this PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

In questions like these, the best way in most cases is too create a Trac ticket with a suggested fix and have it fixed in the theme, IMO.

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 agree this is unfortunate but I think it's one of the fundamental tradeoffs we were hoping to make by going back to using row-gap instead of 'manually' setting margins to achieve row spacing 😞 Trying to solve this problem was actually one of the reasons I initially reverted back to margins in an earlier iteration.

In order to start using row-gap properly, we remove all the margin top/bottom styles that were previously establishing that row spacing. The tradeoff is that this reduces the gap between the entire Buttons container and adjacent blocks, because unlike margin, row-gap isn't added above/below the first/last rows.

For blocks themes, I added the margin block support to the Buttons container to make this easily configurable. For classic themes, I thought about adding extra margin to the container to compensate -- but if the margin is on the container, it collapses into margin of adjacent blocks. It's another tricky one -- if we add margins to the individual buttons in addition to row-gap, we'll end up artificially increasing the spacing between rows, and the only other idea I had was adding another wrapping div (👎).

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for exploring all the options and explaining the trade-offs in detail @stacimc! That satisfies me when it comes to this issue, and I think the benefits are worth it. I'm supportive of proceeding with this PR and opening Trac tickets where needed as Joen has suggested 👍

}

&.is-content-justification-left {
Expand All @@ -50,18 +39,6 @@ $blocks-block__margin: 0.5em;
&.is-content-justification-right {
justify-content: flex-end;

> .wp-block-button {
/*rtl:ignore*/
margin-left: $blocks-block__margin;
/*rtl:ignore*/
margin-right: 0;

&:first-child {
/*rtl:ignore*/
margin-left: 0;
}
}

&.is-vertical {
align-items: flex-end;
}
Expand All @@ -75,28 +52,6 @@ $blocks-block__margin: 0.5em;
&.aligncenter {
text-align: center;
}
&.alignleft .wp-block-button {
/*rtl:ignore*/
margin-left: 0;
/*rtl:ignore*/
margin-right: $blocks-block__margin;

&:last-child {
/*rtl:ignore*/
margin-right: 0;
}
}
&.alignright .wp-block-button {
/*rtl:ignore*/
margin-right: 0;
/*rtl:ignore*/
margin-left: $blocks-block__margin;

&:first-child {
/*rtl:ignore*/
margin-left: 0;
}
}

// Back compat: Inner button blocks previously had their own alignment
// options. Forcing them to 100% width in the flex container replicates
Expand All @@ -116,7 +71,6 @@ $blocks-block__margin: 0.5em;
/* stylelint-enable indentation */
margin-left: auto;
margin-right: auto;
margin-bottom: $blocks-block__margin;
width: 100%;
}
}
Expand Down