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

Add --wp--style--block-gap default for the button block #1738

Merged
merged 2 commits into from
Mar 23, 2022

Conversation

laurelfulford
Copy link
Contributor

All Submissions:

Changes proposed in this Pull Request:

Blocks now use the --wp--style--block-gap CSS variable to set the space between different block elements. Since the Newspack theme doesn't have a theme.json, it doesn't set its own values for these variables, but it does use the WordPress fallbacks, which look like they're often measured in ems.

For the buttons block specifically, this em value is affected when you change the font size of the individual buttons -- decreasing the font size can decrease the gap to the point where there is no longer space for the buttons to sit side by side. This PR adds a gap value fallback that no longer is affected by changing the button block font size, but without affecting other blocks that use the value that don't have this same issue (like the Columns block).

So far I haven't run into this issue for other blocks -- possibly because where the font styles are applied -- but it seems like something we'll want to fix on a case by case basis if there are others, just to make sure we don't add a spacing default that works in some cases, but not others!

Closes #1736

How to test the changes in this Pull Request:

  1. Add a Buttons block to the editor, and add at least two buttons.
  2. Make each button 50% of the available space; note how they site side by side on the front-end and in the editor:

image

  1. Decrease the font size of the first button to 15px.
  2. Note that the buttons now stack:

image

  1. Apply the PR and run npm run build.
  2. Confirm that the buttons now sit side-by-side as expected on the front-end and in the editor:

image

Other information:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes, as applicable?
  • Have you successfully ran tests with your changes locally?

@laurelfulford laurelfulford added the [Status] Needs Review The issue or pull request needs to be reviewed label Mar 22, 2022
@laurelfulford laurelfulford requested a review from a team as a code owner March 22, 2022 17:41
Copy link
Contributor

@dkoo dkoo left a comment

Choose a reason for hiding this comment

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

This fixes the issue of the gap. I tested with various combinations of font sizes and didn't see any problems with stacking.

Perhaps not totally related to this, but I notice that the discrepancy between editor font sizes (e.g. "normal" is 16px in the editor but 20px on the front-end) persists even if you set the font size as a pixel value. In other words, if you set the font size to 15px in the editor, it shows up as 15px on the front-end, but set it to 16px and it renders as 20px. A bit disconcerting but perhaps already a known bug?

@laurelfulford
Copy link
Contributor Author

Thanks @dkoo!

A bit disconcerting but perhaps already a known bug?

Unfortunately yes -- and a very annoying bug! 😞 There's a brief issue reported in this repo and a more in-depth one on the Newsletters side, since that's a contributor: Automattic/newspack-newsletters#755 The TL;DR: is that Gutenberg is picking up the font sizes set by Newsletters, and while dequeuing the FSE Global Styles is helping somewhat, things are still getting garbled where the font slug names overlap.

@laurelfulford laurelfulford merged commit 1cca280 into master Mar 23, 2022
@laurelfulford laurelfulford deleted the fix/1736-add-block-gap branch March 23, 2022 16:24
@dkoo
Copy link
Contributor

dkoo commented Mar 23, 2022

Ah yes, I remember that Newsletters issue now. Thanks for explaining!

matticbot pushed a commit that referenced this pull request Mar 31, 2022
# [1.57.0-alpha.1](v1.56.2...v1.57.0-alpha.1) (2022-03-31)

### Bug Fixes

* add a --wp--style--block-gap default for the button block ([#1738](#1738)) ([1cca280](1cca280))
* align square logos to the left on smaller screens ([#1747](#1747)) ([1aec323](1aec323))
* make cover block link styles more specific ([#1745](#1745)) ([e5a1d68](e5a1d68))
* remove bottom space from last block in cover block ([#1744](#1744)) ([180600c](180600c))
* update ad spacing and move ad background styles([#1742](#1742)) ([199df51](199df51))
* update approach to remove space with above footer ad ([#1746](#1746)) ([fe36561](fe36561))

### Features

* **newsletters:** custom newsletter styles ([#1730](#1730)) ([0a47ee1](0a47ee1))
@matticbot
Copy link
Contributor

🎉 This PR is included in version 1.57.0-alpha.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

matticbot pushed a commit that referenced this pull request Apr 5, 2022
# [1.57.0](v1.56.2...v1.57.0) (2022-04-05)

### Bug Fixes

* add a --wp--style--block-gap default for the button block ([#1738](#1738)) ([1cca280](1cca280))
* align square logos to the left on smaller screens ([#1747](#1747)) ([1aec323](1aec323))
* make cover block link styles more specific ([#1745](#1745)) ([e5a1d68](e5a1d68))
* remove bottom space from last block in cover block ([#1744](#1744)) ([180600c](180600c))
* update ad spacing and move ad background styles([#1742](#1742)) ([199df51](199df51))
* update approach to remove space with above footer ad ([#1746](#1746)) ([fe36561](fe36561))

### Features

* **newsletters:** custom newsletter styles ([#1730](#1730)) ([0a47ee1](0a47ee1))
@matticbot
Copy link
Contributor

🎉 This PR is included in version 1.57.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released on @alpha released [Status] Needs Review The issue or pull request needs to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

--wp--style--block-gap default value affected by button block font size
3 participants