Fix Site Logo block alignment issues #36627

ndiego · 2021-11-18T18:48:10Z

Description

Fixes #36606. I have identified a handful of alignment issues with the Site Logo block that make it quite unwieldy in FSE themes.

Alignment Issues

When a Site Logo block is placed inside of another block, such as a Columns block, the Site Logo will not align center on the frontend, but will appear centered in the Block Editor.
However when aligned right while placed in another block, the Site Logo will not align right in the Block Editor, but will appear aligned right on the frontend. (This is related to a larger issue, see: alignRight/Left not displaying properly in Column and Cover blocks (maybe others as well) #36624)
When a Site Logo block is placed in a post or page, and is not inside of another block, the default alignment displays the block centered, rather than left aligned. On the frontend, the Site Logo is left aligned as expected.

Related Issues
In exploring these issues, there were a couple other discoveries as well.
A. The alignment class is output twice in the blockProps markup.
B. The default Site Logo size is set both in the Editor CSS as well as in the component itself. The CSS in the editor is not needed.

This PR addresses issues 1 and 3 of the alignment issues and A and B of the related issues. Issue 2 will need to be addressed in a separate PR and impacts more than just the Site Logo block.

PR Notes
There are a couple things to note with the changes proposes. When the Site Logo is not aligned, the focus “border” when the block is selected takes up the full width of the container, rather than just highlighting the logo itself. This is consistent with the Image block and therefore I don’t think this needs to be remedied in this PR.

When the Site Logo is reset, the Placeholder can appear different than expected. If the logo had not been manually resized, the placeholder looks correct. If it had been resized prior to being reset, the block no longer has the .is-default-size class resulting in some unique behavior depending on how the Site Logo was aligned. This was true prior to this PR, but is exacerbated a bit when the Site logo has no set alignment after this PR. I would recommend the placeholder be iterated on in another PR, but can work on it here as well if requested.

How has this been tested?

This was tested using Twenty Twenty-Two and emptytheme in Gutenberg 11.9.1 and WordPress 5.8.2.

Screenshots

Types of changes

Bug fix

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
I've tested my changes with keyboard and screen readers.
My code has proper inline documentation.
I've included developer documentation if appropriate.
I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).

carolinan · 2021-11-19T08:54:02Z

I am struggling with reproducing all 3 issues with WordPress trunk 5.9-alpha-51272-src and current Gutenberg trunk.
I am getting a different result. These screenshots are without the PR applied:

Sample code, logo inside another block:

<!-- wp:group -->
<div class="wp-block-group"><!-- wp:site-logo /-->

<!-- wp:site-logo {"align":"center"} /-->

<!-- wp:site-logo {"align":"right"} /--></div>
<!-- /wp:group -->

Editor:

Front:

Sample code

<!-- wp:site-logo /-->

<!-- wp:site-logo {"align":"center"} /-->

<!-- wp:site-logo {"align":"right"} /-->

Editor:

Front:
It is notable how the right aligned image is outside the content area of the post.

carolinan · 2021-11-19T09:04:54Z

With the PR applied.
Editor:

Front:

carolinan · 2021-11-19T09:12:57Z

Front, markup without the PR:

Default, no alignment:
<div class="is-default-size wp-block-site-logo"><a href="http://localhost:8889/" class="custom-logo-link" rel="home"><img></a></div>

Centered:
<div class="aligncenter is-default-size aligncenter wp-block-site-logo"><a href="http://localhost:8889/" class="custom-logo-link" rel="home"><img></a></div>

I do not see the is-style-default class, so I am wondering if there is a reproduction step missing, like applying the rounded style and then setting it back to default?

Front, markup with the PR

Default, no alignment:
<div class="is-default-size wp-block-site-logo"><a href="http://localhost:8889/" class="custom-logo-link" rel="home"><img></a></div>

Centered:
<div class="is-default-size aligncenter wp-block-site-logo"><a href="http://localhost:8889/" class="custom-logo-link" rel="home"><img></a></div>

ndiego · 2021-11-19T11:24:46Z

@carolinan I apologize for any confusion. I should have outlined the steps more clearly.

Issue 1:
If you look at the first set of screenshots you posted, the second Site Logo is configured to be centered. It is centered in the Block Editor, but it's left-aligned on the frontend. With the PR, it's centered.

Issue 2:
This is a larger issue (#36624), and to replicate you need to place the Site Logo in a Columns or Cover block.

Issue 3:
This issue can be replicated if the Site Logo is just placed directly in a post/page. So not inside of a Group, or any other block. Your second batch of screenshots in the first comment illustrate this. In the Block Editor, the first Site Logo is centered even though it has no alignment, and on the frontend it is left-aligned as it should be.

Related issue A:
In the frontend markup you posted, aligncenter is printed twice without this PR and is only printed once with the PR.

Let me know if I can answer any additional questions or you find any other issues!

carolinan · 2021-11-19T11:32:37Z

As far as I can tell, the PR solves the issues, but it needs a retest with the column or cover block.

ndiego · 2021-11-20T17:37:13Z

I have added the backport tag as I really believe we need to get a solution in place prior to 5.9 Beta 1. Without a fix, it's nearly impossible to configure certain header layouts in an FSE theme.

aaronrobertshaw

Nice work @ndiego 👍

Alignments, in general, are a bit rough but in my testing, this PR does improve them for the Site Logo block.

✅ Alignments (except right-alignment with Columns) were consistent between editor and frontend
✅ Alignments were consistent regardless of current theme type (Block or Classic)
✅ No duplicate alignment classes in markup

Tested with:

TwentyTwentyTwo
TT1 Blocks
Emptytheme
TwentyTwentyOne
TwentyTwenty

However when aligned right while placed in another block, the Site Logo will not align right in the Block Editor, but will appear aligned right on the frontend.

As you note in #36624, when within a Columns block the aligned block wrapper doesn't receive the float: right style. I believe this could be due to the Columns block not supporting layouts through block supports as the Group block does. The layout block support might provide further clues.

A separate PR to address this sounds good to me.

When the Site Logo is not aligned, the focus “border” when the block is selected takes up the full width of the container, rather than just highlighting the logo itself. This is consistent with the Image block and therefore I don’t think this needs to be remedied in this PR.

Agreed. It would be good though to put this into an issue so it doesn't fall through the cracks.

The focus border/outline in this PR does appear consistent with the image block but less than ideal.

When the Site Logo is reset, the Placeholder can appear different than expected.

This could possibly be improved by adding setAttributes( { width: undefined } ); within the onRemoveLogo callback. Without the width value set it picks up the is-default-size CSS class again.

If resetting the logo image, it makes sense to me to also reset any custom width for it. I don't have a lot of context in terms of the history here so I don't feel strongly about it.

aaronrobertshaw · 2021-12-01T06:35:53Z

packages/block-library/src/site-logo/editor.scss

@@ -1,4 +1,5 @@
 .wp-block[data-align="center"] > .wp-block-site-logo {
+	display: table;
 	margin-left: auto;
 	margin-right: auto;
 	text-align: center;


I'm not sure this text-align: center; is required in the editor. With or without the resizable box inside the site logo block wrapper it stays aligned to the center for me.

Good point, I will test further and remove later today.

ndiego · 2021-12-01T15:05:33Z

This could possibly be improved by adding setAttributes( { width: undefined } ); within the onRemoveLogo callback. Without the width value set it picks up the is-default-size CSS class again.

If resetting the logo image, it makes sense to me to also reset any custom width for it. I don't have a lot of context in terms of the history here so I don't feel strongly about it.

Thanks for testing and the feedback @aaronrobertshaw. I will look into this today and also add a separate issue concerning the border. I will also look into another PR for the missing float: right style.

carolinan · 2021-12-14T07:09:36Z

Hi, is this PR ready to be reviewed again? I see some comments that are not addressed?
If not, do we need to remove it from the 5.9 must haves?

ndiego · 2021-12-14T11:07:55Z

@carolinan admittedly, this fell off my radar. I will review again today. I believe this PR is good, but I need to create separate issues/PRs for things that surfaced in testing this PR.

ndiego · 2021-12-15T16:44:15Z

I have removed the unnecessary text alignment CSS that @aaronrobertshaw highlighted and thoroughly retested this. All works as expected and this is ready to be merged/backported unless anyone has objections.

I am working on separate issues for the border issue (not directly related to this PR) and right/left alignment is covered here #36624.

ndiego · 2021-12-16T01:02:08Z

This could possibly be improved by adding setAttributes( { width: undefined } ); within the onRemoveLogo callback. Without the width value set it picks up the is-default-size CSS class again.

I have gone ahead and also added this fix to the PR as well. Since the user is "reseting" the image, I think it makes sense to also reset the width to default.

This PR is ready to merge unless there are any further objections. The alignRight/alignLeft issue is noted in #36624 and will be addressed post-5.9.

ntsekouras

Thanks for the PR @ndiego!

I tested a bit and it seems to work well. I would also defer to @jasmussen about the style changes as he recently worked on them and could have feedback for the focus issue when no align is set.

packages/block-library/src/site-logo/index.php

ntsekouras · 2021-12-16T09:12:00Z

packages/block-library/src/site-logo/editor.scss

@@ -10,20 +10,6 @@
 		pointer-events: none;
 	}

-	&:not(.is-default-size) {


Why did you remove these styles? Is it because this change const currentWidth = width || defaultWidth; ? You have kept the addition of this class in code though - should we remove the code as well?

Yeah that's correct. However .is-default-size is used on the placeholder for when there is no set Site Logo, or the user resets the logo. This class is added to blockProps which wraps both the placeholder and the actual Site logo when set, so I just opted to leave it. On the frontend, this class is added if no custom size is set.

jasmussen · 2021-12-16T09:34:38Z

Thanks for the PR. High level, it makes sense; the site logo when unaligned is block level (unless set to table as it was), and technically the border going edge to edge is true to that:

If the table property was causing headaches, then let's remove it 👍 👍

When the Site Logo is aligned, or used inside a flex container, the focus style adapts as it should. Shown here inside a Row block with a navigation on the right:

ndiego · 2021-12-16T13:47:32Z

Thanks for testing @ntsekouras and @jasmussen. I have added comments above and made changes based on your suggestions.

ntsekouras

Thanks again Nick! Since the style changes made sense to Joen as well, I think this is good to go!

Wait for green CI and 🚢

ndiego · 2021-12-16T17:00:23Z

@ntsekouras it looks like one of the tests is failing, but I can't figure out why. The error message does not seem to refer to the Site Logo block. Any insight on this?

ntsekouras · 2021-12-16T17:55:18Z

@ntsekouras it looks like one of the tests is failing, but I can't figure out why. The error message does not seem to refer to the Site Logo block. Any insight on this?

The. failures seem completely unrelated. I restarted the tests, so let's see.. 😄

ndiego · 2021-12-16T18:51:59Z

@ntsekouras looks like all the tests have passed! I have not merged a Gutenberg PR before. I assume "Squash and merge" or should it be "Rebase and merge"? Appreciate your help 😉

aaronrobertshaw · 2021-12-16T23:42:21Z

looks like all the tests have passed! I have not merged a Gutenberg PR before. I assume "Squash and merge" or should it be "Rebase and merge"? Appreciate your help 😉

Nice work on this PR @ndiego!

Merge this using the "Squash and merge" option, tidy up the commit message as appropriate, and enjoy.

I'll let you do the honors 🙂

ndiego · 2021-12-17T00:41:13Z

Haha thanks!

* Remove duplicate alignment class. * Copy editor aligncenter styling from editor to frontend. * Ensure default width is set properly and remove conflicting CSS. * Revert some CSS changes after testing. * Remove unnecessary text alignment CSS. * Set the Site Logo width to default when reset. * Remove unnecessary align attribute.

ndiego added 4 commits November 17, 2021 11:46

Remove duplicate alignment class.

ea765ca

Copy editor aligncenter styling from editor to frontend.

3e95759

Ensure default width is set properly and remove conflicting CSS.

0512633

Revert some CSS changes after testing.

c1cdeeb

ndiego requested a review from ajitbohra as a code owner November 18, 2021 18:48

ndiego added [Type] Bug An existing feature does not function as intended [Block] Site Logo Affects the Site Logo Block labels Nov 18, 2021

ndiego self-assigned this Nov 18, 2021

ndiego added the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Nov 20, 2021

aaronrobertshaw reviewed Dec 1, 2021

View reviewed changes

This was referenced Dec 2, 2021

Block Spacing setting causes conflict with blockGap (Columns, Navigation, etc.) #36521

Closed

alignRight/Left not displaying properly in Column and Cover blocks (maybe others as well) #36624

Closed

ndiego added 2 commits December 15, 2021 11:28

Merge branch 'WordPress:trunk' into fix/site-logo-alignment

3d36b7d

Remove unnecessary text alignment CSS.

2f06808

ndiego requested a review from aaronrobertshaw December 15, 2021 16:44

ndiego added 2 commits December 15, 2021 15:06

Merge branch 'WordPress:trunk' into fix/site-logo-alignment

1de81f2

Set the Site Logo width to default when reset.

1a13b0e

ntsekouras reviewed Dec 16, 2021

View reviewed changes

Remove unnecessary align attribute.

c6f47a4

ntsekouras approved these changes Dec 16, 2021

View reviewed changes

ndiego merged commit 1534439 into WordPress:trunk Dec 17, 2021

ndiego deleted the fix/site-logo-alignment branch December 17, 2021 00:40

github-actions bot added this to the Gutenberg 12.3 milestone Dec 17, 2021

tellthemachines removed the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Dec 21, 2021

simison mentioned this pull request Oct 10, 2022

Site logo block: upload button misalinged when block is center-aligned #44816

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Site Logo block alignment issues #36627

Fix Site Logo block alignment issues #36627

ndiego commented Nov 18, 2021 •

edited

Loading

carolinan commented Nov 19, 2021

carolinan commented Nov 19, 2021

carolinan commented Nov 19, 2021

ndiego commented Nov 19, 2021

carolinan commented Nov 19, 2021

ndiego commented Nov 20, 2021 •

edited

Loading

aaronrobertshaw left a comment

aaronrobertshaw Dec 1, 2021

ndiego Dec 1, 2021

ndiego commented Dec 1, 2021

carolinan commented Dec 14, 2021 •

edited

Loading

ndiego commented Dec 14, 2021

ndiego commented Dec 15, 2021

ndiego commented Dec 16, 2021

ntsekouras left a comment

ntsekouras Dec 16, 2021

ndiego Dec 16, 2021

jasmussen commented Dec 16, 2021

ndiego commented Dec 16, 2021

ntsekouras left a comment

ndiego commented Dec 16, 2021

ntsekouras commented Dec 16, 2021

ndiego commented Dec 16, 2021

aaronrobertshaw commented Dec 16, 2021

ndiego commented Dec 17, 2021

Fix Site Logo block alignment issues #36627

Fix Site Logo block alignment issues #36627

Conversation

ndiego commented Nov 18, 2021 • edited Loading

Description

How has this been tested?

Screenshots

Types of changes

Checklist:

carolinan commented Nov 19, 2021

Front:

carolinan commented Nov 19, 2021

carolinan commented Nov 19, 2021

Front, markup without the PR:

Front, markup with the PR

ndiego commented Nov 19, 2021

carolinan commented Nov 19, 2021

ndiego commented Nov 20, 2021 • edited Loading

aaronrobertshaw left a comment

Choose a reason for hiding this comment

aaronrobertshaw Dec 1, 2021

Choose a reason for hiding this comment

ndiego Dec 1, 2021

Choose a reason for hiding this comment

ndiego commented Dec 1, 2021

carolinan commented Dec 14, 2021 • edited Loading

ndiego commented Dec 14, 2021

ndiego commented Dec 15, 2021

ndiego commented Dec 16, 2021

ntsekouras left a comment

Choose a reason for hiding this comment

ntsekouras Dec 16, 2021

Choose a reason for hiding this comment

ndiego Dec 16, 2021

Choose a reason for hiding this comment

jasmussen commented Dec 16, 2021

ndiego commented Dec 16, 2021

ntsekouras left a comment

Choose a reason for hiding this comment

ndiego commented Dec 16, 2021

ntsekouras commented Dec 16, 2021

ndiego commented Dec 16, 2021

aaronrobertshaw commented Dec 16, 2021

ndiego commented Dec 17, 2021

ndiego commented Nov 18, 2021 •

edited

Loading

ndiego commented Nov 20, 2021 •

edited

Loading

carolinan commented Dec 14, 2021 •

edited

Loading