Site icon: Fix site icon styling to display non-square site icons within a square button #37570
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
andrewserong
added
[Type] Bug
|
James and Joen, I've added you both as reviewers if you get a chance, since you've given feedback to or contributed to the site icon recently 🙂 |
|
Size Change: -511 B (0%) Total Size: 1.13 MB
ℹ️ View Unchanged
|
|
This tested well for me with wide, tall, and square images in both post and site editor contexts. Technically this looks like a good solution to me, but will hold off giving it a ✅ until it has had design feedback. |
|
My goodness, I hadn't even noticed that the hover style for a customized logo had changed! Here are GIFs to help the conversation along. In trunk, a landscape image remains landscape, just scaled down. It has a focus style, and it zooms way in on hover: This branch is the same, only the landscape image is soft-cropped to be square, regardless of its original aspect ratio:
Just looking at the before and after, this is better, the image should be square. 👍 👍 — I personally think the hover style might be a bit jumpy, but it's fine, and it's separate. In principle the code looks good, however it seems like you should be able to accomplish the square crop by combining the same width and height with |
jasmussen
approved these changes
Dec 22, 2021
There was a problem hiding this comment.
I noted in my testing that you could have probably accomplished the same square icon with object-fit: cover;. I would encourage a quick code sanity check to see if that's actually better, as that would afford the alt text to remain. Whether that's actually better for a screen-reader, to read a button with an image inside, or to just read a button, I can't say for sure.
But just as a before/after enhancement, this one is good to go.
| border-radius: $radius-block-ui; | ||
| background-size: cover; | ||
| margin-top: -1px; |
There was a problem hiding this comment.
Excellent eye. Interestingly, when you're just toggling off that minus 1 margin in the inspector, it makes the site icon look vertically uncentered when it's on:
However when you zoom in and look at the details, you can see that it has to have that negative margin in order to horizontally align with the other buttons in the toolbar. With negative margin:
Without negative margin:
This is, per your instinct, related to the dark square being 61px tall. That math comes $header-height + $border-width, so that the black area covers the gray border below the top-bar.
So which do we keep? I don't have a strong opinion, but I appreciate your attention to detail, so probably do keep it. But I'd add a small comment above, something like // Compensate for the top-bar border., and then change 1px to $border-width instead.
There was a problem hiding this comment.
Might it be neater to make the black square 60px tall, then add a shadow to hide the grey border? Then we don't need the awkward negative margin.
There was a problem hiding this comment.
Thanks for the detailed approach here! I had a go at switching to 60px tall and using a shadow to hide the grey border. Unfortunately the styling in the site editor then wound up being a bit unreliable due to the z-index stacking order. With 60px and box-shadow, from time to time the shadow would pop out / a white line would be visible:
I've gone with adding the inline comment and using $border-width instead of the hard-coded 1px, and we can always look at switching to the 60px approach in a follow-up if we'd like to tweak further.
| border-radius: $radius-block-ui; | ||
| background-size: cover; | ||
| margin-top: -1px; |
There was a problem hiding this comment.
Same as above, could use a comment and a variable.
There was a problem hiding this comment.
Switched to using the variable, and added a comment.
|
Thanks for the review @jasmussen!
Yes, I keep forgetting that we can use I liked the idea of using the |
andrewserong
deleted the
fix/site-icon-styling-to-support-non-square-site-icons
branch
noisysocks
removed
the
Backport to WP 6.7 Beta/RC
noisysocks
pushed a commit
that referenced
this pull request
Jan 4, 2022
…hin a square button (#37570) * Fix site icon styling to display non-square site icons within a square button * Switch back to using an img element, use object-fit instead
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #37564
Description
Following on from the site icon feature introduced in #35892 this PR fixes the styling for the site icon in the W button so that it displays non-square site icons within a square button. The fix does the following:
divinstead of animgtag, and set the image as a background image with background size ofcover.Note: I needed to add the
-1pxon the top margin to get the image to be centred correctly within the border as below:I think this is because the container is
61pxhigh instead of60pxto deal with the grey border, so getting the position exact along with the scale animation is slightly tricky. Let me know if anyone thinks this needs tweaking further!How has this been tested?
To quickly add a non-square site icon:
Use as site iconin the sidebar inspector controls.Also, I checked that the styling when no icon is set appears to be unaffected (to test this go to:
/wp-admin/customize.php?autofocus[section]=title_taglineand clear the existing site icon, then reload the post and site editors)Screenshots
Types of changes
Bug fix (non-breaking change which fixes an issue)
Checklist:
*.native.jsfiles for terms that need renaming or removal).