Change admin logo height to match breadcrumbs height #1822
Conversation
- adjusting to better vertical and horizontal align - remove unused css selectors
| img { | ||
| vertical-align: middle; | ||
| max-width: 100%; | ||
| max-height: 100%; |
There was a problem hiding this comment.
We should not remove the max-width and max-height rules, otherwise large images will pop out of the div.
| height: 58px; // height of .page-title | ||
| box-sizing: content-box; | ||
| background-color: $color-1; | ||
| padding: 14px 0 8px; |
There was a problem hiding this comment.
Instead of using (also asynchronous) padding we should use line-height to vertically align the image in the box and remove all the padding. This has the advantage that images could consume the whole available space if they like.
We can fix the padding of the default logo by removing the extra whitespace from the image.
There was a problem hiding this comment.
The solution I came up with
.admin-nav-header {
padding: 0;
border-bottom: 1px solid $color-border;
text-align: center;
// Using line height for proper vertical centering.
// As line height does not take the border width into account we need to subtract it.
line-height: $main-header-height - 1px;
}There was a problem hiding this comment.
We should also use the (to be introduced) $main-header-height variable to set the height on the .main-header
$main-header-height: 75px !default;.main-header {
...
height: $main-header-height;
...
}That fixes the height for empty main headers, in the login screen for instance
Before
After
| @@ -1,5 +1,5 @@ | |||
| <header class="admin-nav-header"> | |||
| <%= link_to spree.admin_path, class: 'brand-link' do %> | |||
There was a problem hiding this comment.
Shops may have used this class for deface overrides. We should not remove it now.
- to unify the with the header size - and "automatically" apply vertical center to the image -> fixes a mis-alignment on headers without text as suggested by @tvdeyen
- Using @include img-fluid(); doesn't actually work, because it defaults to `display: inline-block`, which is not what we want. - Thereby, source the definition of responsive image from Foundation: https://github.com/zurb/foundation-sites/blob/develop/scss/_global.scss#L154-L163
- available for replace by Deface overrides - as well pointed out by @tvdeyen
|
Super, actually no logo resize was necessary after the line-height change .. which is good, as I didn't want to mess the frontend somehow. I've allowed myself one more try with an automatic image resize in d2d0b89 . Reason is that I think that nowadays all images should be responsive, like in foundation. I'm not sure to what extend that would apply for such a project, but I've browsed through some product images, and everything looks normal, so it's pending your decision on this "global" one.
There is nothing much different than before, but posting a screen nevertheless:
|
The image itself seemed to contain white space underneath, so have modified the padding values accordingly to vertically center it.edit: uses line-height now, see conversation.
Removed a bit of css as well.
Before
After
Still looks a bit boxed, but oh well : )