Add border support to site logo #48354
Conversation
carolinan
added
[Block] Site Logo
|
Size Change: +343 B (+0.02%) Total Size: 1.81 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 1e7f619. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/10346022782
|
There was a problem hiding this comment.
Thank you for working on this! I've tested it out, and in my testing, it's working really well.
- Confirm that there is a new button for opening the border styles panel. ✅
- Confirm that the border displays correctly when an image is selected. ✅
- Confirm that the logo and border shows correctly. ✅
- The settings override the border from the global styles. ✅
- Tested with various custom border colours, styles, and surrounding global styles like padding and margin. ✅
There was a problem hiding this comment.
Thank you for taking the time to put this PR together @carolinan 👍
There are a few issues we'll need to address before this can be merged.
- The block needs to take border width into account with its sizing
- The inline cropping should also reflect the border
- The image class name can get
undefinedwithin it
Screen.Recording.2023-06-23.at.2.31.06.pm.mp4
|
Thank you. I did not realize the border needed to be included in the width; I expected it to be on the outside so that the image doesn't shrink.
I used the image block as a reference but did not want to copy code without fully understanding it. Width: I added the 
Crop: I added the border to the cropper. I don't know this feature well enough to test if it works correctly now. |
There was a problem hiding this comment.
Appreciate the effort and patience iterating on this one. I know there are a lot of edge cases around borders that can make these things a bit of a grind.
Problem: the right side resize handle is displayed correctly, but the bottom handle is not in the correct position, so I assume I need to adjust something related to the height.
The resizable box doesn't appear to be getting a height that matches the image. I wonder if tweaking the lockAspectRatio prop the same as the image block does would help?
However, when the block is centered, all resize handles are displayed correctly.
Looking closer, it appears that the difference between the centred block and one without alignment is that the resizable box element gets a display: table style. Blindly adding that in via dev tools appears to put the resizable box's bottom handle where it should be.
I haven't gotten to explore why this is the case or what side effects might occur as a result but maybe that is a thread to follow 🤷
Crop: I added the border to the cropper. I don't know this feature well enough to test if it works correctly now.
You should be able to test the cropper by comparing it against trunk. Adding the border shouldn't change much other than the window through which the cropper operates. On that container, we might need to add the box-sizing rule to it as well.
These croppers are due for some love in Phase 3 as part of the Media Library improvements so maybe a holistic solution might come through that.
One last thought, it was requested that the image block's placeholder not show the border when selected. Perhaps we should be doing the same here for the Site Logo? This would avoid the upload icon getting clipped at the edges as well.
|
@aaronrobertshaw Thank you, I will continue on it this week or next. |
|
This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress. If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged. If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack. Thank you! ❤️ View changed files❔ packages/block-library/src/site-logo/index.php |
There was a problem hiding this comment.
I appreciate the persistence and hard work on this one @carolinan, I know it's a tricky one! 🙇
After giving this one another run, some aspects are working well and others not so much.
In particular, we might need to improve the handling around placeholders. I'm getting some weird behaviour when the block is in its placeholder state, including:
- Scrollbars within the placeholder when borders are applied
- Somewhat inconsistent rendering of the placeholder, seemed warped when different border combinations were applied
- Global styles apply to the placeholder at all times but block instance styles don't apply to the placeholder when the block is selected, which it has to be to edit the styles. It's a bit confusing.
Also, the global border radius styles don't apply. The .wp-block-site-logo img style will need to have the selector's specificity lowered.
When the block instance styles are applied, the image cropper maintains its shape and size nicely. That isn't the case when only using global border styles or the rounded block style.
That might sound like a lot but this has come a long way. Nice work!
Screen.Recording.2024-07-31.at.12.54.16.PM.mp4
On the placeholder, the CSS from the component is overriding the border radius from the global styles. |
| @@ -47,24 +44,27 @@ | |||
| // @todo this particular minimal style of placeholder could be componentized further. | |||
| .wp-block-site-logo.wp-block-site-logo { | |||
|
|
|||
| /** | |||
| &.is-default-size .components-placeholder { | |||
| height: 60px; | |||
| width: 60px; | |||
| } | |||
There was a problem hiding this comment.
If I don't remove the height and width, the upload button is misaligned when there is a border.
In this screenshot, a border width of 10px is added in global styles:
There was a problem hiding this comment.
I am not sure what this CSS is for, was 60px chosen because the default size of the logo is 120?
There was a problem hiding this comment.
Ok, I found the ticket #48218
I am not sure how to resolve it without letting it fall back to height: 100% and width:100% though! @richtabor
There was a problem hiding this comment.
I am not sure what this CSS is for, was 60px chosen because the default size of the logo is 120?
Yes, per https://github.com/WordPress/gutenberg/pull/48218—120x120 is too big for a logo placeholder, but 120px is the max initial width for when a logo is uploaded.
There was a problem hiding this comment.
This is a tricky one 🤔
While in the placeholder state the layout shift when adding a border sticks out. When not in the placeholder state the block has border-box set for the box-sizing. Adding border-box for the placeholder state exacerbates the problem with scrollbars etc.
I'm not sure there is a bulletproof solution but maybe there is something we can do to smooth out the experience a bit for the less extreme use cases.
If we avoid applying the custom border on the placeholder directly and instead apply it to a new pseudo element. We can position the bordered pseudo element over the inner contents. It will start to crop the underlying button as the border increases but that seems better than scrollbars.
Hacking around in dev tools this is what I got:
Screen.Recording.2024-08-08.at.8.05.37.PM.mp4
|
|
||
| /** | ||
| // Inherit radius. | ||
| > div, // A 60px width div shown only in the editor on mobile. | ||
| .components-resizable-box__container { | ||
| border-radius: inherit; | ||
| } |
There was a problem hiding this comment.
The border-radius:inherit on the >div here is overriding the border radius from the global styles.
I did not understand the code comment about the div only being shown in the editor on mobile, or how to test it.
| :root :where(.wp-block-site-logo .components-placeholder) { | ||
| border-radius: inherit; | ||
| } | ||
|
|
There was a problem hiding this comment.
Even with the border-radius:inherit
- Commented out on line 58
- Moved from line 67 to 114 with reduced specificity
the placeholder component's own CSS still overrides the border radius from the global styles.
| a, | ||
| img { | ||
| border-radius: inherit; | ||
| box-sizing: border-box; |
There was a problem hiding this comment.
I found that the border radius from the "rounded" style variation is applied without this CSS, but maybe I missed something in my testing.
|
I'm seeing this here; the default logo block experience should look like an image block, but be contained by a default width:
Would it be fine to just omit the border on the placeholder? The radius is fine, but the border is a bit funky. I get its applied elsewhere; just looking for a way to unblock this. |
|
I don't understand. What is "this* ? I don't know what setting is applied on the block in the screenshot or how it differs from the expected. Funky is not a technical term I can solve 😂😂😂 |
|
I should probably open a separate issue for this, but I want to ping @ciampo in this PR first because of the context. There seems to be conflicting intentions about wether the placeholder component should be styled by blocks or not. So I am looking for a definite answer about wether the placeholder component should be styled by global styles or not. |
|
For the part where the border on the placeholder is covering the misaligned upload button, what if I remove the box-sizing? site-logo-border-footprint.mp4 |
|
Thanks for persisting on this one @carolinan, huge effort 💪
I'd say at the time, this was intentional and desired. I also recall there was an effort to overhaul and improve placeholders more broadly. It's possible this is where things diverged. There might be a thread to follow in #43180 and before that #42847 (comment).
I think in general it is better to have more predictable sizing to prevent janky layout shifts and the like. Quickly testing with TT4, if you set a border on the site logo block type, the placeholder will exceed its expected width and start to overlap the site title in the header part. 
This seems to be the crux of the matter and I don't know the answer, sorry 🙏 At the risk of taking the easy way out, I'd like to defer to our design gurus on that. I suspect that either way, individual block instance styles and global styles should be rendered the same on placeholders. |
I am not able to reproduce this. Did you do anything extra besides applying and building the PR? |
|
With the option available in the toolbar, what if I remove the button that is in the placeholder? Then the border can not overlap the button, and we also don't need to think about the size of the clickable area. |
What?
Add border block support to the site logo.
Partial for #43247, #37757
Why?
To allow more customization of the block, and for consistency with other blocks.
How?
The border is applied to the image element, not the wrapper.
Testing Instructions
Please test both the placeholder and the site logo with image, with and without link, duotone, rounded style variation, and padding.
When testing with dutotone, you should find that the duotone changes the border color. This is consistent with other image blocks that supports both.
When testing padding, you should find that the padding is on the wrapper, which means it is not between the image and the border but between the border and other page content. This is also consistent with other blocks.
If you are using a placeholder, you will see that the border has a low opacity, but it should be present.
I found that this is inconsistent across blocks. The image block placeholder has no visible border, the featured image has a fully visible border, not a border with low opacity.
Confirm that the border displays correctly when an image is selected.
Screenshots or screencast