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

Site Logo: add padding and margin support #43520

Merged
merged 20 commits into from
Sep 7, 2022
Merged

Site Logo: add padding and margin support #43520

merged 20 commits into from
Sep 7, 2022

Conversation

colorful-tones
Copy link
Member

Related:

What?

Add padding and margin support to the Site Logo block.

Why?

To create consistency across blocks.

How?

Added the relevant block support in block.json and updated Core Block Reference docs.

Testing Instructions

  1. Go to Appearance > Editor (beta)
  2. Add the Site Logo block to the template
  3. Configure margin and padding and verify the output.

site-logo-supports

@colorful-tones colorful-tones self-assigned this Aug 23, 2022
@colorful-tones colorful-tones added [Type] Enhancement A suggestion for improvement. [Feature] Design Tools Tools that impact the appearance of blocks both to expand the number of tools and improve the experi [Block] Site Logo Affects the Site Logo Block labels Aug 23, 2022
@colorful-tones colorful-tones requested review from ndiego, aaronrobertshaw and glendaviesnz and removed request for ndiego August 23, 2022 12:40
@@ -725,7 +725,7 @@ Display a graphic to represent this site. Update the block, and the changes appl

- **Name:** core/site-logo
- **Category:** theme
- **Supports:** align, color (~~background~~, ~~text~~), ~~alignWide~~, ~~html~~
- **Supports:** align, color (~~background~~, ~~text~~), ~~alignWide~~, ~~html~~, spacing (margin, padding)
Copy link
Member

Choose a reason for hiding this comment

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

This is minor, but can you alphabetize the active supports followed by the alphabetized excluded supports? So spacing would come after color.

Copy link
Member Author

Choose a reason for hiding this comment

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

Nothing too minor. 👍 I addressed this and alpha sorted. Thanks!

@colorful-tones colorful-tones requested a review from ndiego August 26, 2022 17:58
@carolinan
Copy link
Contributor

For this block, I was not able to get top and bottom margin to work.

@colorful-tones
Copy link
Member Author

For this block, I was not able to get top and bottom margin to work.

@carolinan I pulled in the latest from trunk and tested again and all margin and padding axes are working. 🤔

Here is a video:

site-logo-padding-margin-support.mp4

My local testing environment:

  • WordPress 6.0.2
  • Twenty Twenty-Two 1.2

@carolinan
Copy link
Contributor

theme.json only, empty theme, logo is not nested:
Screen Shot 2022-09-02 at 03 59 27

theme.json only, TT2, logo in the default position inside the nested header, front view
Screen Shot 2022-09-02 at 04 02 10

global styles, padding and margin set to medium spacing preset, TT2:
Screen Shot 2022-09-02 at 04 05 09

global styles, padding and margin set to medium spacing preset, TT2, site logo not nested (its between the site header and main query)
Screen Shot 2022-09-02 at 04 08 43

@carolinan
Copy link
Contributor

Global styles, spacing preset 80, logo is not nested, Twenty Twenty-Three:
Screen Shot 2022-09-02 at 04 25 02

@colorful-tones
Copy link
Member Author

Thanks for the additional testing @carolinan

It took me a bit to decipher whether your testing was offering successful or failure in moving this issue along, but I see that there needs to be additional CSS cascade resolution for this to work.

The .wp-blocks > * + * {} seems to be the culprit.

Copy link
Contributor

@carolinan carolinan left a comment

Choose a reason for hiding this comment

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

These margin issues has to be bigger than these two blocks, and not caused by the PR.
My concern is that these changes will go into 6.1 without the margin begin fixed, and it may appear broken to many users...

@colorful-tones
Copy link
Member Author

These margin issues has to be bigger than these two blocks, and not caused by the PR.

👍

My concern is that these changes will go into 6.1 without the margin begin fixed, and it may appear broken to many users...

I share these concerns as well and I'm glad you are keeping them on the radar. Do you happen to know if there is an existing issue related to the over-specificity for margin @carolinan ?

@aaronrobertshaw
Copy link
Contributor

Copy link
Contributor

@andrewserong andrewserong left a comment

Choose a reason for hiding this comment

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

Thanks for opening a PR for this one @colorful-tones, this is testing nicely for me, too. It looks like the margin styling conflicts issue is the one that @aaronrobertshaw linked to in #43404.

From my perspective, we shouldn't let that issue block merging in these opt-in PRs, as we will still have time to investigate bugs in style output after the 6.1 feature freeze. Dealing with specificity in the layout support and top margins is quite tricky to untangle as sometimes folks will want the Layout support to take precedence, and other times the block-level setting in theme.json. Since the margin support does support block-level in post content overriding Layout styles, in most cases, folks should be able to still come up with manual workarounds, so I don't think it's a blocker for these opt-ins.

LGTM!

@andrewserong
Copy link
Contributor

@colorful-tones one other thing that would be good to add before landing this, is the box-sizing: border-box rule like you added in #43519 (though we can always add it after the fact). Most of the time I think site logos will typically be included in a Row block where this isn't so noticeable, but if we have this block sitting next to a Group block with padding, then it's noticeable that the padding behaves differently in TwentyTwentyTwo:

image

@colorful-tones
Copy link
Member Author

one other thing that would be good to add before landing this, is the box-sizing: border-box rule like you added in #43519

@andrewserong good call! I added the box-sizing: border-box for this block as well. Thanks for the testing and feedback. ❤️

Copy link
Contributor

@andrewserong andrewserong left a comment

Choose a reason for hiding this comment

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

Thanks for updating @colorful-tones, this is working nicely for me now! I'll merge this in and update the tracking issue 👍

@andrewserong andrewserong merged commit d209629 into WordPress:trunk Sep 7, 2022
@github-actions github-actions bot added this to the Gutenberg 14.1 milestone Sep 7, 2022
@femkreations femkreations added the Needs User Documentation Needs new user documentation label Sep 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Site Logo Affects the Site Logo Block [Feature] Design Tools Tools that impact the appearance of blocks both to expand the number of tools and improve the experi Needs User Documentation Needs new user documentation [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants