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

build_post_featured_image() runs unnecessarily #732

Closed
paulschreiber opened this issue Jul 18, 2017 · 5 comments
Closed

build_post_featured_image() runs unnecessarily #732

paulschreiber opened this issue Jul 18, 2017 · 5 comments
Milestone

Comments

@paulschreiber
Copy link
Contributor

build_post_featured_image() runs unnecessarily, whether or not the featured image is shown in the template.

We have postmeta that determines whether to hide/show the image.

Result: if the featured image is a gif, amp-anim gets added to amp_component_scripts, which generates a warning in the AMP validator.

@paulschreiber
Copy link
Contributor Author

It would be helpful to be able to filter this i.e. if ( apply_filters( 'amp_featured_image_is_visible', true ) ) { ... } .

@westonruter
Copy link
Member

How are you turning off a featured image from being displayed? Are you doing something like:

add_filter( 'amp_post_template_data', function( $data ) {
    $data['featured_image'] = false;
    return $data;
} );

If so, then it would seem to me that the scripts returned by AMP_Img_Sanitizer should get queued up after the filter applies, not before.

@paulschreiber
Copy link
Contributor Author

How are you turning off a featured image from being displayed? Are you doing something like:

We have a metabox with a checkbox titled "Show image on article page." When the box is not checked, we save some postmeta to indicate suppression.

In the template itself, we retrieve the value of the postmeta, put it in $show_featured_image, and call something like this:

if ( $show_featured_image ) {
   get_template_part( 'template-parts/featured-image-embed' );
}

In the amp_post_template_data filter, we have code like this:

// Workaround for https://github.com/Automattic/amp-wp/issues/732
//
// Suppress amp-anim if:
// (a) no GIFs in body
// (b) featured image is a GIF
// (c) suppress featured image checkbox is checked
//
if ( ! preg_match( '/<amp-anim.+src=".+\.gif/', $data['post_amp_content'] ) ) { // query params may be present after gif; no close quote
	$featured_image_url = get_the_post_thumbnail_url( $post->ID );
	$url_parts = wp_parse_url( $featured_image_url );
	$file_extension = strtolower( pathinfo( $url_parts['path'], PATHINFO_EXTENSION ) );
	if ( 'gif' === $file_extension ) {
		$show_featured_image  = get_post_meta( $post->ID, ...

		if ( ! $show_featured_image ) {
			unset( $data['amp_component_scripts']['amp-anim'] );
		}
	}
}

@westonruter
Copy link
Member

This should be made obsolete by #2202 in that we can move to constructing the Reader mode templates in a more traditional way, and then only the AMP components that are actually used in the page will then be among the scripts included on the page.

@westonruter
Copy link
Member

westonruter commented Nov 26, 2019

Fixed by #3781. Now only the required component scripts are included on the rendered page, as needed.

@westonruter westonruter added this to the v1.5 milestone Nov 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants