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

How to make small images stretch to full width #1137

Closed
janvitos opened this issue May 9, 2018 · 15 comments
Closed

How to make small images stretch to full width #1137

janvitos opened this issue May 9, 2018 · 15 comments

Comments

@janvitos
Copy link

janvitos commented May 9, 2018

Hi,

So I've been playing with the amp plugin and its CSS to make small images stretch to full width (100%), but I can't seem to figure out how.

Sometimes I have images that are smaller than the viewport, and for some reason, they don't take the full width although they are added automatically to the content with layout="intrinsic" by the plugin. The only way I was able to make the images stretch properly was to set "display: block" on the "amp-img" tag and "width: 100%" on the "i-amphtml-intrinsic-sizer" tag. If not setting "width: 100%", the image takes the full width but ratio is not maintained and the image looks stretched.

Though, after changing the CSS for the "i-amphtml-intrinsic-sizer" tag, Google started reporting issues, stating I could not modify that tag. So now I'm all out of ideas.

Any help will greatly be appreciated.

Thank you.

@janvitos janvitos changed the title How to have small images stretch to full width How to make small images stretch to full width May 9, 2018
@janvitos
Copy link
Author

janvitos commented May 10, 2018

So I noticed that replacing the default "intrinsic" layout with "responsive" fixes the issue. Though, I had to do this is the plugin's code for now to make it work. Is there any filter I can use to change the layout from "intrinsic" to "responsive" on images in functions.php ?

Thank you.

@janvitos
Copy link
Author

janvitos commented May 10, 2018

In the meantime, I've edited class-amp-img-sanitizer.php to add a filter to be able to choose between the "intrinsic" layout and "responsive":

Change the following lines in class-amp-img-sanitizer.php:

if ( empty( $new_attributes['layout'] ) && ! empty( $new_attributes['height'] ) && ! empty( $new_attributes['width'] ) ) {
	$new_attributes['layout'] = 'intrinsic';
}

To this:

if ( empty( $new_attributes['layout'] ) && ! empty( $new_attributes['height'] ) && ! empty( $new_attributes['width'] ) ) {
	$new_attributes['layout'] = apply_filters( 'amp_image_layout', 'intrinsic' );
}

Add this to your functions.php and replace "responsive" with whatever layout you want:

function set_amp_image_layout( $layout ) {
	$layout = 'responsive';
	return $layout;
}
add_filter( 'amp_image_layout', 'set_amp_image_layout', 10, 1 );

P.S. This will NOT stick through updates.

I'm still wondering if the small images not stretching to the full width of the viewport is the expected behaviour of the "intrinsic" layout. Could somebody enlighten me on this please ? Thanks.

@westonruter
Copy link
Member

@kienstra Thoughts on this?

@janvitos The intrinsic layout was added in #937

@kienstra
Copy link
Contributor

kienstra commented May 11, 2018

Thanks, Looking At This Now

Hi @janvitos,
Thanks, you raise a good point about whether images with layout="intrinsic" should expand to fill the container. I'm looking at that now.

In the meantime, it's also possible to set a layout directly on the image. Though I'd imagine you're looking for a more programmatic solution.

When the plugin finds this in the post content (or anywhere in Native AMP or Paired Mode):

<img data-amp-layout="responsive" ...>

It will convert it on AMP endpoints to:

<amp-img layout="responsive" ...>

Update: I just corrected the attribute from data-layout to data-amp-layout

@kienstra
Copy link
Contributor

kienstra commented May 11, 2018

Expected Behavior Of layout="intrinsic"

Hi @janvitos,
You raise a good point about the expected behavior of layout="intrinsic". It's a little different than I thought.

It looks like <amp-img layout="intrinsic"> isn't expected to expand beyond its natural size:

The intrinsic layout will have an intrinsic size and will inflate a floated div until it reaches either the natural image size or a CSS constraint like max-width"

(emphasis mine)

This is a little different from how I thought layout="intrinsic" worked. But I think it's still a good default.

If the default layout were responsive, images would stretch beyond their natural size to fill the container:

https://legacy-ampconfdemo.pantheonsite.io/2018/05/11/test-small-image-layout/amp/
layout-intrinsic-amp

But let me see if there's a programmatic way to get the effect you're looking for.

@janvitos
Copy link
Author

janvitos commented May 11, 2018

Hi @kienstra, thanks for the reply and all the info.

After reading your reply, I understand that layout="intrinsic" does not stretch to fill the viewport by default. This is a behaviour I currently use a lot on my site, especially for animated gifs that often have a much smaller resolution than other images because of file size limitations.

So since layout="intrinsic" seems to be the default way the AMP plugin is going, what I did is add a filter in the code to be able to change layout="intrinsic" to layout="responsive" in my theme's functions.php file (see my third post) according to my needs. Though, I know this won't stick with updates.

I was thinking maybe it would be a good idea to have this filter added to the plugin so people can choose between "intrinsic" and "responsive"? Or maybe there's already a way to do it without changing the plugin's code?

Thank you.

@kienstra
Copy link
Contributor

Question About Use Case

Hi @janvitos,
Thanks for suggesting the filter.

Are there a lot of images that you'd like to make responsive?

There's a new feature in the upcoming 1.0 release, where you'll be able to select the layout for each image in Gutenberg. Of course, this wouldn't help if you want to change a lot of images.

@kienstra
Copy link
Contributor

Hi @janvitos,
No pressure, but if you're comfortable sharing it and it's public, would you like to share the URL of your site?

We could consider that as a use case where responsive would be a better default.

@janvitos
Copy link
Author

I honestly think layout="responsive" is still a better fit for all my images since they aren't always properly sized and identically sized. I'd be interested to see if others might also prefer this behaviour for their sites. I assume most blogs would want it. But maybe I'm wrong ;)

Most images fit properly in the viewport with layout="intrinsic", but it does leave the smaller ones looking pretty bad when displayed alongside bigger images. I can show you a screenshot to show you what I mean. Give me a few moments.

@janvitos
Copy link
Author

So here's the first screenshot with layout="intrinsic":

https://imgur.com/fUmxECF

Here's the second screenshot with layout="responsive":

https://imgur.com/NgRsM1f

If you'd like to inspect the code, just let me know and I'll provide you the URL.

@kienstra
Copy link
Contributor

kienstra commented May 11, 2018

Thanks!

Hi @janvitos,
Thank for your screenshots. I can see how layout="responsive" works better for those images.

If you're comfortable with it, it'd be great if you could share a URL of the site, so we can see the display on different viewports. And how many images there are. But no pressure 😄

@westonruter
Copy link
Member

I was thinking maybe it would be a good idea to have this filter added to the plugin so people can choose between "intrinsic" and "responsive"? Or maybe there's already a way to do it without changing the plugin's code?

Instead of a filter, in the current develop branch there is currently support for a data-amp-layout attribute for you to specify on the element itself the layout to use. This I think is better than a filter:

https://github.com/Automattic/amp-wp/blob/d2b2c2dccc7a6d4ddd0a2e27bb5715c36dc33f45/includes/sanitizers/class-amp-img-sanitizer.php#L132-L133

@janvitos
Copy link
Author

janvitos commented May 11, 2018

Hi @kienstra, it's my pleasure.

Here's an AMP URL to that same post shown in the screenshots: https://www.ipnoze.com/shibas-chiens-droles/amp/

The current layout is "responsive". Should you need to do some tests with the "intrinsic" layout on that URL, I could easily change that for you.

Thanks.

@janvitos
Copy link
Author

So I got your suggestion working @kienstra and used the following filter to add the data-amp-layout attribute on all images:

add_filter('the_content','new_content');
function new_content($content) {
    $content = str_replace('<img','<img data-amp-layout="responsive"', $content);
    return $content;
}

This is exactly what I wanted. Should I mark this ticket closed? If you still want to investigate on the matter I'll be glad to assist. But for my part, everything is now working properly.

Thank you so much for your time and expertise.

@kienstra
Copy link
Contributor

kienstra commented May 13, 2018

Thanks!

Hi @janvitos,
Thanks, it's great to see that you found a solution. Sure, feel free to mark this as closed.

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

No branches or pull requests

3 participants