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.

Sign up for GitHub

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 icon: Fix site icon styling to display non-square site icons within a square button #37570

Merged
merged 2 commits into from
Dec 23, 2021
Merged

Site icon: Fix site icon styling to display non-square site icons within a square button #37570

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
dc57d46
Fix site icon styling to display non-square site icons within a squar…
andrewserong Dec 22, 2021
04e0109
Switch back to using an img element, use object-fit instead
andrewserong Dec 23, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions packages/edit-post/src/components/header/fullscreen-mode-close/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,11 +67,12 @@ function FullscreenModeClose( { showTooltip, icon, href } ) {

if ( siteIconUrl ) {
buttonIcon = (
<motion.img
<motion.div
variants={ ! disableMotion && effect }
alt={ __( 'Site Icon' ) }
className="edit-post-fullscreen-mode-close_site-icon"
src={ siteIconUrl }
style={ {
backgroundImage: `url(${ siteIconUrl })`,
} }
/>
);
}
Expand Down
3 changes: 3 additions & 0 deletions packages/edit-post/src/components/header/fullscreen-mode-close/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,5 +55,8 @@

.edit-post-fullscreen-mode-close_site-icon {
width: $button-size;
height: $button-size;
border-radius: $radius-block-ui;
background-size: cover;
margin-top: -1px;
Copy link
Contributor

Choose a reason for hiding this comment

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

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:
margin shift

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:
with minus 1 px

Without negative margin:

without minus 1 px

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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:

Kapture 2021-12-23 at 11 32 51

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.

}
7 changes: 4 additions & 3 deletions packages/edit-site/src/components/navigation-sidebar/navigation-toggle/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,11 +66,12 @@ function NavigationToggle( { icon } ) {

if ( siteIconUrl ) {
buttonIcon = (
<motion.img
<motion.div
variants={ ! disableMotion && effect }
alt={ __( 'Site Icon' ) }
className="edit-site-navigation-toggle__site-icon"
src={ siteIconUrl }
style={ {
backgroundImage: `url(${ siteIconUrl })`,
} }
/>
);
} else if ( isRequestingSiteIcon ) {
Expand Down
3 changes: 3 additions & 0 deletions packages/edit-site/src/components/navigation-sidebar/navigation-toggle/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,5 +62,8 @@

.edit-site-navigation-toggle__site-icon {
width: $button-size;
height: $button-size;
border-radius: $radius-block-ui;
background-size: cover;
margin-top: -1px;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above, could use a comment and a variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Switched to using the variable, and added a comment.

}