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

Jump to bottom

Fix header image filtering and YouTube header video detection #1208

Merged
merged 2 commits into from
Jun 12, 2018
Merged

Fix header image filtering and YouTube header video detection #1208

merged 2 commits into from
Jun 12, 2018

Conversation

westonruter
Copy link
Member

Amends #1074. See #1078.

Before 🚫

<div id="wp-custom-header" class="wp-custom-header">
    <amp-img 
        src="https://src.wordpress-develop.test/wp-content/uploads/2018/06/cropped-hoodoo-ski-area-2016-12-29.jpg"
        width="2000"
        height="1199"
        alt="WordPress Develop"
        srcset="
            https://src.wordpress-develop.test/wp-content/uploads/2018/06/cropped-hoodoo-ski-area-2016-12-29.jpg 2000w, 
            https://src.wordpress-develop.test/wp-content/uploads/2018/06/cropped-hoodoo-ski-area-2016-12-29-300x180.jpg 300w, 
            https://src.wordpress-develop.test/wp-content/uploads/2018/06/cropped-hoodoo-ski-area-2016-12-29-768x460.jpg 768w, 
            https://src.wordpress-develop.test/wp-content/uploads/2018/06/cropped-hoodoo-ski-area-2016-12-29-1024x614.jpg 1024w
        "
        sizes="(max-width: 767px) 89vw, (max-width: 1000px) 54vw, (max-width: 1071px) 543px, 580px"
    ></amp-img>
</div>

Notice the low resolution image:

image

After ✅

<div id="wp-custom-header" class="wp-custom-header">
    <amp-img
        src="https://src.wordpress-develop.test/wp-content/uploads/2018/06/cropped-hoodoo-ski-area-2016-12-29.jpg"
        width="2000"
        height="1199"
        alt="WordPress Develop"
        srcset="
            https://src.wordpress-develop.test/wp-content/uploads/2018/06/cropped-hoodoo-ski-area-2016-12-29.jpg 2000w, 
            https://src.wordpress-develop.test/wp-content/uploads/2018/06/cropped-hoodoo-ski-area-2016-12-29-300x180.jpg 300w, 
            https://src.wordpress-develop.test/wp-content/uploads/2018/06/cropped-hoodoo-ski-area-2016-12-29-768x460.jpg 768w, 
            https://src.wordpress-develop.test/wp-content/uploads/2018/06/cropped-hoodoo-ski-area-2016-12-29-1024x614.jpg 1024w
        " 
        sizes="100vw" 
        class="amp-wp-enforced-sizes"
        layout="intrinsic"
    ></amp-img>
</div>

Notice high resolution image:

image

@westonruter westonruter added this to the v1.0 milestone Jun 10, 2018
@westonruter westonruter requested a review from kienstra June 10, 2018 07:00
@westonruter
Fix header image filtering and YouTube header video detection
4445c18 
* Make sure that any get_header_image_tag filters from theme get applied. Fixes quality issue in Twenty Seventeen.
* Fix detection of YouTube short URLs.
* Dequeue wp-custom-header instead of deregistering to prevent Query Monitor complaining.
kienstra
kienstra approved these changes Jun 12, 2018
View reviewed changes
Copy link
Contributor

@kienstra kienstra left a comment

Choose a reason for hiding this comment

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

Approved

Hi @westonruter,
This PR looks good. It's nice how it accommodates URLs like https://youtu.be/v5zg_Yv048s.

Nice "before" and "after" images and markup. I also saw how this PR enables outputting the layout attribute on the header image.

includes/class-amp-theme-support.php
@@ -369,7 +369,10 @@ public static function add_hooks() {
add_action( 'comment_form', array( __CLASS__, 'amend_comment_form' ), 100 );
remove_action( 'comment_form', 'wp_comment_form_unfiltered_html_nonce' );
add_filter( 'wp_kses_allowed_html', array( __CLASS__, 'whitelist_layout_in_wp_kses_allowed_html' ), 10 );
add_filter( 'get_header_image_tag', array( __CLASS__, 'conditionally_output_header' ), 10, 3 );
add_filter( 'get_header_image_tag', array( __CLASS__, 'amend_header_image_with_video_header' ), PHP_INT_MAX );
Copy link
Contributor

Choose a reason for hiding this comment

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

It's nice how this allows all other filters to run first.

tests/test-class-amp-theme-support.php Outdated
@@ -1338,15 +1338,15 @@ public function test_whitelist_layout_in_wp_kses_allowed_html() {
/**
* Test AMP_Theme_Support::conditionally_output_header().
*
* @see AMP_Theme_Support::conditionally_output_header()
* @see AMP_Theme_Support::amend_header_image_with_video_header()
*/
public function conditionally_output_header() {
Copy link
Contributor

@kienstra kienstra Jun 12, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

This is an error I made in a commit, but this method conditionally_output_header() should have been test_conditionally_output_header().

But with the method renamed, this method should probably be test_amend_header_image_with_video_header().

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for noting that. Fixed in 97620b9.

includes/class-amp-theme-support.php
$image_header = AMP_HTML_Utils::build_tag( 'amp-img', $atts );
$youtube_id = null;
if ( isset( $parsed_url['host'] ) && preg_match( '/(^|\.)(youtube\.com|youtu\.be)$/', $parsed_url['host'] ) ) {
if ( 'youtu.be' === $parsed_url['host'] && ! empty( $parsed_url['path'] ) ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It's nice how this accounts for URLs like https://youtu.be/v5zg_Yv048s

@westonruter westonruter merged commit a92f802 into develop Jun 12, 2018
@westonruter westonruter deleted the fix/header-media branch June 12, 2018 15:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kienstra kienstra kienstra approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v1.0
Development

Successfully merging this pull request may close these issues.
2 participants
@westonruter @kienstra