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

Adjust paragraph block spacing to use standardised variables #14679

Merged

Conversation

@getdave
Copy link
Contributor

commented Mar 28, 2019

Description

Please see this discussion on the Section Block for context.

Essentially we need to ensure that all Blocks which apply padding when they have a background set have the same values set. To do this we need to create some new variables that match up with what is currently set on the paragraph Block.

Adjusting the paragraph Block values seems fraught with danger so we construct new vars to provide new vars which match the existing values.

Testing

To test just confirm that the values for padding on the <p class="wp-block-paragraph... have the same values as on master (ie: padding: 20px 30px).

getdave added some commits Mar 28, 2019

Adds vars for Block padding when background present
There are a number of cases where a backgroud is added to a Block which then requires that the Block has padding within the Editor to make it look visally acceptable. These values are often hardcoded. To standardise these, we add vars for vertical and hoz padding for Blocks which have background applied.
Adds new Block with background vars to paragraph Block
Replaces existing values with equivalents but as variables. This will allow us to utilise these standardised vars elsewhere such as the Section Block.

@getdave getdave self-assigned this Mar 28, 2019

@getdave getdave requested review from mapk and jasmussen Mar 28, 2019

@getdave getdave requested a review from chrisvanpatten as a code owner Mar 28, 2019

@getdave getdave referenced this pull request Mar 28, 2019

Merged

Add container block #13964

15 of 15 tasks complete

@getdave getdave requested a review from youknowriad Mar 28, 2019

@jasmussen

This comment has been minimized.

Copy link
Contributor

commented Mar 28, 2019

Thanks for creating this PR!

I love where you're coming from, and I think the instinct is right. But I think we should remember to separate UI from content. The variables you leverage are firmly UI values.

Perhaps we also don't need new variables just for the padding in paragraphs and section blocks, maybe we can just do this?

Before:

p.has-background {
	padding: 20px 30px;
}

After:

p.has-background {
	padding: $grid-size-large ($grid-size-large * 2);
}

That maps to padding: 16px 32px, but that seems fine to me. Alternately if we consider the 20/30px values backcompat, we could do this:

p.has-background {
	padding: ($grid-size-small * 5) ($grid-size-small * 8);
}

That gets us only 2px from the 30, but that should be close enough.

@getdave

This comment has been minimized.

Copy link
Contributor Author

commented Apr 1, 2019

@jasmussen Really appreciate your feedback here.

Thanks for creating this PR!

No problem 👍

But I think we should remember to separate UI from content. The variables you leverage are firmly UI values.

I didn't quite follow this. I am certainly considering them UI values. They are a core part of the UI which are needed elsewhere but are not captured in a variable. Therefore they represent a risk in the codebase if many disparate locations rely on them remaining consistent.

Perhaps we also don't need new variables just for the padding in paragraphs and section blocks

Ideally not. However, as you suggest for backwards compatibility reasons I'd be pretty nervous about changing these values. Who knows how many 3rd party Blocks rely on these? There are probably also locations within Gutenberg Core itself which rely on them remaining "as is".

only 2px from the 30, but that should be close enough.

Knowing the amount of work it took to get Section alignments working, I am not convinced that we can consider 2px not to be a problem. That's enough to make a layout look pretty broken.

I'm still not 100% sure why we can't introduce a new variable. A quick search in the Block library finds 4-5 uses of 20px and 2-3 of 30px. Maybe this isn't a sufficient threshold to make this a problem, but what if these use cases are important?

Not trying to be awkward here, just wanting to ensure we

  1. Shoot for 100% accuracy
  2. Ensure consistency
  3. Reduce risk

If we're not convinced then I'd be tempted to leave "as is" rather than reopen the Section block alignment again.

@jasmussen

This comment has been minimized.

Copy link
Contributor

commented Apr 2, 2019

I didn't quite follow this. I am certainly considering them UI values. They are a core part of the UI which are needed elsewhere but are not captured in a variable. Therefore they represent a risk in the codebase if many disparate locations rely on them remaining consistent.

I was a bit unclear here. By UI I'm referring to the editor — the left navigation, the top editor bar, the right sidebar, but not the editing canvas, which I'm referring to as "content".

Which actually means my own suggestion to use the $grid-size variable is a bad idea, and we can totally introduce new variables for this, so thanks for taking me to task.

But $block-padding is also editor UI, because it doesn't (and shouldn't) affect the footprint of a block, and it's UI that we draw to manipulate those blocks regardless of what theme or editor style you are using. For themes that do well in making the editing canvas look like the frontend theme through editor styles, you will very likely have situations where those default block paddings are heavily customized by said theme. That fact isn't changed by how the padding variables are put together, but simply keeping them separate from the editor specific variables feels like general code-quality and cleanliness. Also, if I can ever ship #14407, $block-spacing will hopefully be retired. In fact we may want to rename $block-padding to be something that's prefixed with editor or something, like $editor-ui__block-padding

So to summarize:

  • You were right, new variables are fine
  • Let's try and avoid using $block-padding and $block-spacing for anything but what we consider "the editor"

On a final nitpicky note, thanks for your patience — do we have some sort of BEM pattern we can follow for the variables? BEM expert @gziolo any thoughts? These feel a little long and exotic to me:

$block-with-background-v-padding
$block-with-background-h-padding

Maybe this $editor-style__background-padding-v and $editor-style__background-padding-h?

You can then keep the values 20px and 30px directly.

@gziolo

This comment has been minimized.

Copy link
Member

commented Apr 10, 2019

On a final nitpicky note, thanks for your patience — do we have some sort of BEM pattern we can follow for the variables? BEM expert @gziolo any thoughts? These feel a little long and exotic to me:

$block-with-background-v-padding
$block-with-background-h-padding

Maybe this $editor-style__background-padding-v and $editor-style__background-padding-h?

I personally don't have any issues with long names. v or h might be hard to read for those who look at those names for the first time. I don't know if there are any coding guidelines for SCSS variables but I see that the general pattern is $my-variable-name. If you are seeking for BEM experts @aduth is your friend 😄

@getdave

This comment has been minimized.

Copy link
Contributor Author

commented Apr 10, 2019

Thanks @gziolo. I've updated the variables. Unless @aduth objects I'd like to get this merged.

Could someone approve this?

@gziolo

This comment has been minimized.

Copy link
Member

commented Apr 10, 2019

I've updated the variables.

Did you push your changes or is it GitHub's cache issue?

@aduth

This comment has been minimized.

Copy link
Member

commented Apr 10, 2019

I don't know if there are any coding guidelines for SCSS variables but I see that the general pattern is $my-variable-name. If you are seeking for BEM experts @aduth is your friend 😄

I too am not aware of any guidelines around how to name variables. Nor do I feel particularly strongly about any.

@getdave

This comment has been minimized.

Copy link
Contributor Author

commented Apr 10, 2019

I've updated the variables.

Did you push your changes or is it GitHub's cache issue?

Caching 41286af

@aduth thanks for confirming. Could you @gziolo or @youknowriad approve this PR then? Much appreciated.

@gziolo

gziolo approved these changes Apr 10, 2019

Copy link
Member

left a comment

No rules 🤷‍♂️

🚢

@getdave getdave merged commit 410c3e0 into master Apr 10, 2019

1 check passed

Travis CI - Pull Request Build Passed
Details

@aduth aduth deleted the fix/adjust-paragraph-block-spacing-to-standardised-variables branch Apr 10, 2019

@youknowriad youknowriad added this to the 5.5 (Gutenberg) milestone Apr 12, 2019

mchowning added a commit to mchowning/gutenberg that referenced this pull request Apr 15, 2019

Adjust paragraph block spacing to use standardised variables (WordPre…
…ss#14679)

* Adds vars for Block padding when background present

There are a number of cases where a backgroud is added to a Block which then requires that the Block has padding within the Editor to make it look visally acceptable. These values are often hardcoded. To standardise these, we add vars for vertical and hoz padding for Blocks which have background applied.

* Adds new Block with background vars to paragraph Block

Replaces existing values with equivalents but as variables. This will allow us to utilise these standardised vars elsewhere such as the Section Block.

* Updates variable to BEM specification
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.