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.

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

Fix stretched logo and header issues in Twenty Seventeen #1419

Merged
merged 3 commits into from Sep 17, 2018

Conversation

Projects
None yet
3 participants
@westonruter
Copy link
Member

westonruter commented Sep 10, 2018

Problem

Per #1305 (comment), on Twenty Seventeen when uploading a square logo I see the following in non-AMP:

image

<img
	width="253" 
	height="250" 
	src="https://src.wordpress-develop.test/wp-content/uploads/2018/05/cropped-amp-brand-blue10x-1.png" 
	class="custom-logo" 
	alt="WordPress Develop" 
	itemprop="logo" 
	srcset="https://src.wordpress-develop.test/wp-content/uploads/2018/05/cropped-amp-brand-blue10x-1.png 253w, https://src.wordpress-develop.test/wp-content/uploads/2018/05/cropped-amp-brand-blue10x-1-100x100.png 100w" 
	sizes="100vw" 
/>

In AMP, however, this comes out as:

image

<amp-img 
	width="253" 
	height="250" 
	src="https://src.wordpress-develop.test/wp-content/uploads/2018/05/cropped-amp-brand-blue10x-1.png" 
	class="custom-logo amp-wp-enforced-sizes" 
	alt="WordPress Develop" 
	itemprop="logo" 
	srcset="https://src.wordpress-develop.test/wp-content/uploads/2018/05/cropped-amp-brand-blue10x-1.png 253w, https://src.wordpress-develop.test/wp-content/uploads/2018/05/cropped-amp-brand-blue10x-1-100x100.png 100w" 
	sizes="100vw" 
	layout="intrinsic"
></amp-img>

Solution

The max-height of the .custom-logo-link img is defined as being 80px, unless there is header media in which case it is 200px. Issues related to vertically-squashed images can be avoided if we just make sure that the image has this height to begin with.

The sizes attribute also has to be removed from the Custom Logo image.

<amp-img
	noloading=""
	width="80.96"
	height="80"
	src="https://src.wordpress-develop.test/wp-content/uploads/2018/05/cropped-amp-brand-blue10x-1.png"
	class="custom-logo amp-wp-enforced-sizes"
	alt="WordPress Develop"
	itemprop="logo"
	srcset="https://src.wordpress-develop.test/wp-content/uploads/2018/05/cropped-amp-brand-blue10x-1.png 253w, https://src.wordpress-develop.test/wp-content/uploads/2018/05/cropped-amp-brand-blue10x-1-100x100.png 100w"
	layout="intrinsic"
></amp-img>

Other Changes

  • Add noloading to the custom logo since the loading indicator is really best suited for larger images. (h/t @sebastianbenz)
  • Fix height of header on Twenty Seventeen when there is no header image/video set.

See also #1321.

westonruter added some commits Sep 10, 2018

@westonruter westonruter added this to the v1.0 milestone Sep 10, 2018

@westonruter westonruter requested a review from hellofromtonya Sep 10, 2018

@westonruter

This comment has been minimized.

Copy link
Member Author

westonruter commented Sep 10, 2018

There are several scenarios for testing this:

  • With/without header image set.
  • With/without title & tagline displayed
  • With portrait/square logo vs wide horizontal logo.
  • On homepage vs another page.
@kienstra

This comment has been minimized.

Copy link
Collaborator

kienstra commented Sep 17, 2018

Approved

Hi @westonruter,
This looks good. And the Custom Logo displays as expected in the testing scenarios you mentioned.

  1. Homepage with header image:

homepage-with-header

  1. Without header image:

without-header-image

  1. Without title and tagline:

without-title-and-tagline

  1. With wide horizontal logo:

wide-horizontal-custom-logo

  1. On post (not homepage):

a-post

return $attr;
}, 11, 3 );
$html = preg_replace( '/(?<=width=")\d+(?=")/', $width, $html );
$html = preg_replace( '/(?<=height=")\d+(?=")/', $height, $html );

This comment has been minimized.

Copy link
@kienstra

kienstra Sep 17, 2018

Collaborator

It's nice how this replaces the width and height of the Custom Logo.

@westonruter westonruter merged commit 8d54648 into develop Sep 17, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@westonruter westonruter deleted the fix/twentyseventeen-logo-header branch Sep 17, 2018

@westonruter westonruter changed the title Fix stretched logo and header issues in Twenty Sevetneen Fix stretched logo and header issues in Twenty Seventeen Sep 24, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.