Skip to content
This repository has been archived by the owner on Oct 10, 2018. It is now read-only.

Rework featured images to correspond with the correct image sizes. #48

Closed
kopepasah opened this issue Jan 27, 2018 · 11 comments
Closed

Comments

@kopepasah
Copy link
Contributor

Within the designs we are provide with very specific image sizes and currently there is no way to represent those image sizes as the amp-wp plugin filters the images in AMP_Img_Sanitizer class. While using the WordPress Core API the_post_thumbnail() does work, it does not output the correct image size, nor does it allow for specific image sizes to be added to the srcset of amp-img (or img before parsing).

@kopepasah
Copy link
Contributor Author

@westonruter @ThierryA are there currently any plans within the amp-wp plugin which will allow for correct setting of images sizes on featured images?

@westonruter
Copy link
Contributor

@kopepasah Could you indicate where the output is currently incorrect? Below are two example outputs. What is not appearing properly? We can absolutely modify the output of get_the_post_thumbnail() as needed for AMP, but I'm not clear on what needs to change:

<?php the_post_thumbnail( 'ampconf-375x225' ); ?>

👇

<amp-img width="375" height="225" src="https://src.wordpress-develop.test/wp-content/uploads/2018/01/American_bison_k5680-1-375x225.jpg" class="attachment-ampconf-375x225 size-ampconf-375x225 wp-post-image amp-wp-enforced-sizes" alt="" srcset="https://src.wordpress-develop.test/wp-content/uploads/2018/01/American_bison_k5680-1-375x225.jpg 375w, https://src.wordpress-develop.test/wp-content/uploads/2018/01/American_bison_k5680-1-768x461.jpg 768w, https://src.wordpress-develop.test/wp-content/uploads/2018/01/American_bison_k5680-1-320x192.jpg 320w" sizes="(min-width: 375px) 375px, 100vw"></amp-img>

and

<?php the_post_thumbnail( 'ampconf-1040x400' ); ?>

👇

<amp-img width="1040" height="400" src="https://src.wordpress-develop.test/wp-content/uploads/2018/01/American_bison_k5680-1-1040x400.jpg" class="attachment-ampconf-1040x400 size-ampconf-1040x400 wp-post-image amp-wp-enforced-sizes" alt="" srcset="https://src.wordpress-develop.test/wp-content/uploads/2018/01/American_bison_k5680-1-1040x400.jpg 1040w, https://src.wordpress-develop.test/wp-content/uploads/2018/01/American_bison_k5680-1-768x295.jpg 768w" sizes="(min-width: 694px) 694px, 100vw"></amp-img>

@kopepasah
Copy link
Contributor Author

kopepasah commented Jan 30, 2018

@westonruter while it is setting the srcset attribute, the values are not the same as within the theme, and I cannot see any way to change them without filtering the entire output of the post thumbnail. In addition, there are a couple other attributes which need to be added/altered (e.g. media and layout, sizes). You can see the expected values in the HTML templates.

I was not sure exactly what the amp-wp plugin was changing already and it was unclear if modifying the output of get_the_post_thumbnail() would not be overwritten by the plugin.

Lastly, if we are modifying the images, should this occur within the plugin or theme? I feel that if we come up with a standard for naming images we can rework the post_thumbnail() output to match our needs exactly, rather than requiring future theme developers to filter the output every time.

Example Name:

add_image_size( 'ampconf-1040x400__responsive__min-768__768w-1040w__[etc]` );

Alternatively we could pass attributes to the_post_thumbnail(), but wanted to run any other options first.

@westonruter
Copy link
Contributor

@kienstra the call to the_post_thumbnail( 'ampconf-1040x400' ) would output the following HTML normally:

<img width="2700" height="1761" src="https://src.wordpress-develop.test/wp-content/uploads/2018/01/American_bison_k5680-1-1.jpg" class="attachment- size- wp-post-image" alt="" ampconf-1040x400="" srcset="https://src.wordpress-develop.test/wp-content/uploads/2018/01/American_bison_k5680-1-1.jpg 2700w, https://src.wordpress-develop.test/wp-content/uploads/2018/01/American_bison_k5680-1-1-300x196.jpg 300w, https://src.wordpress-develop.test/wp-content/uploads/2018/01/American_bison_k5680-1-1-768x501.jpg 768w, https://src.wordpress-develop.test/wp-content/uploads/2018/01/American_bison_k5680-1-1-1024x668.jpg 1024w" sizes="(max-width: 2700px) 100vw, 2700px" />

In AMP this gets sanitized as:

<amp-img width="1040" height="400" src="https://src.wordpress-develop.test/wp-content/uploads/2018/01/American_bison_k5680-1-1-1040x400.jpg" class="attachment-ampconf-1040x400 size-ampconf-1040x400 wp-post-image amp-wp-enforced-sizes" alt="" srcset="https://src.wordpress-develop.test/wp-content/uploads/2018/01/American_bison_k5680-1-1-1040x400.jpg 1040w, https://src.wordpress-develop.test/wp-content/uploads/2018/01/American_bison_k5680-1-1-768x295.jpg 768w" sizes="(min-width: 694px) 694px, 100vw"></amp-img>

This is wrong, yes? Apparently the sanitizer is not doing the right thing, not honoring the width and height specified on the img itself, and not keeping the srcset?

@westonruter
Copy link
Contributor

Here's the short-term workaround: d26f628

@westonruter
Copy link
Contributor

@kienstra you could open an issue in the plugin to fix this issue?

@kienstra
Copy link
Contributor

Will Open An Issue

Sure, @westonruter. I'll open an issue to address this. Thanks for your workaround commit for reference.

@delawski
Copy link
Contributor

We may be able to simplify the sizes property of amp-img tags by applying max-width to the amp-img in the CSS. This way no matter what size is currently being used by AMP, the image will downsize to fit into the containing element.

Still, having control over layout, media and sizes properties would be beneficial.

@westonruter
Copy link
Contributor

@delawski you can control the layout and media by passing them in as args to the function, like:

<?php ampconf_the_post_thumbnail( 'ampconf-1040x400', array( 'layout' => 'responsive' ) ); ?>

The sizes one, however, is generated by WP via wp_calculate_image_sizes() and srcset via wp_calculate_image_srcset(). WP will override anything you pass into the function.

@kienstra
Copy link
Contributor

Request To Respond To Plugin Issue

Hi @kopepasah,
Could you please see this question on a plugin issue related to this issue? Thanks 😄

@kopepasah
Copy link
Contributor Author

kopepasah commented Feb 5, 2018

@westonruter while ampconf_the_post_thumbnail() helper function does allow for some customization, I think there are two main issues that still need to be solved by the plugin:

  1. Helper function should be in the plugin, not the theme. The helper function is only temporary.
  2. wp_calculate_image_sizes() simply does not provide the granularity that is needed for controlling exact layouts that AMP allows. Simply put, wp_calculate_image_sizes() was not built for this and, as such, we should come up with a better method for allowing usage of images easily within themes.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants