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

AMP Applies CSS to all images in content so they take up 100% width #1182

Closed
ameshkin opened this Issue Dec 17, 2015 · 12 comments

Comments

Projects
None yet
7 participants
@ameshkin
Copy link

ameshkin commented Dec 17, 2015

I created this issue as requested, which is linked to my previous problem

#174

There should be a way to tell AMP to ignore images. I'm new to this code, and am using the wordpress plugin. So I'm trying to not have to modify it since we will lose the ability to update.

So perhaps I am missing something. Perhaps there is already a way to fix this.

If your images do not have a width or a height, console will give you a warning and a red dot will surround the enlarged image.

After applying a width and a height to my png, the image is still enlarged.

After changing it to an SVG, the image is enlarged but put inside of a grey circle and cut off.

http://tp.ameshkin.devprogress.org/justice/2015/11/24/3725294/white-supremacists-black-lives-matter-minneapolis/?amp=1

@dvoytenko

This comment has been minimized.

Copy link
Collaborator

dvoytenko commented Dec 17, 2015

It takes bigger size because it's marked as layout=responsive as opposed to layout=fixed. By saying it's responsive you basically tell the image to take all available width and sometimes it's actually desirable. What you can do here is this:

  1. Use layout=fixed (it's the default). It will use your width/height as hard pixel values.
  2. Continue using layout=responsive as now, but either put the image in a smaller container or set CSS max-width on it.

There are other options as well, such as using sizes attribute, but these two would be the most typical. More on this here: https://github.com/ampproject/amphtml/blob/master/spec/amp-html-layout.md

@dvoytenko dvoytenko self-assigned this Dec 17, 2015

@fsencinas

This comment has been minimized.

Copy link

fsencinas commented Dec 17, 2015

Well, when you use a RWD layout, in order to keep the proper aspect ratio of an image or video, and avoiding the "jump" or flash effect, is always a need to set a width height, in order to precalculate the DAR and to scale accordingly.
Note: The Display Aspect Ratio formula could be also calculated from original dimensions of an image or video without infer sizes, but such dimensions are only available once the media is fully loaded.

@ameshkin

This comment has been minimized.

Copy link

ameshkin commented Dec 18, 2015

Thank you. I will set a width and height to that image.

One problem is that we're using the wordpress plugin, and so I can't just make code changes.

The wordpress app automatically creates the html. I will figure something out though.

But basically the wordpress guys need to come up with some way to give an image a certain attribute so that the plugin knows to make it a fixed image.

I will bring this up on the plugin page.

@dvoytenko dvoytenko assigned rudygalfi and unassigned dvoytenko Dec 18, 2015

@dvoytenko

This comment has been minimized.

Copy link
Collaborator

dvoytenko commented Dec 18, 2015

@rudygalfi This seems pretty important. Layout spec is a critical spec. Should we reach out to them?

@rudygalfi

This comment has been minimized.

Copy link
Contributor

rudygalfi commented Dec 18, 2015

@mjangda Is there support for specifying layout using the AMP plugin for Wordpress? If not, shall I go ahead and open a new issue in https://github.com/Automattic/amp-wp ?

@pietergreyling

This comment has been minimized.

Copy link
Contributor

pietergreyling commented Dec 19, 2015

I have reached out to @mjangda - awaiting his feedback. @rudygalfi, You can assign this issue to me if you prefer. I will sync with @mjangda. //pieter

@mjangda

This comment has been minimized.

Copy link
Contributor

mjangda commented Dec 19, 2015

@rudygalfi @dvoytenko yep, we need to fix the plugin to handle the updated layout spec. Should land this week.

@bazzadp

This comment has been minimized.

Copy link

bazzadp commented Dec 19, 2015

Ignoring the Wordpress issue for now, I still think there's an issue here for when you have images that are too large for some viewports but too small for others.

Without AMP you'd just set max-width of 100% and height of auto so original image size is used until the viewport is too small and then the image is shrunk as appropriate, in ratio, for smaller screens.

I can't find a way to replicate this in AMP documents since AMP explicitly sets the height and width. I do not want smaller images stretched for larger viewports (so can't use layout="reponsive"), but I also do not want them them to be larger than the viewport for smaller viewports either (so can't use layout="fixed").

@dvoytenko you mentioned in your original reply about limiting the parent container size but do you have an example on how to do this? I'm guessing I need to create a parent div element and use CSS to set the height and width in CSS, but this is complicated to do, as need to create a div on an image by image basis.

It would be much nicer if there was a "responsive-capped" layout type which is capped at original image dimensions (as specified in the height and width for that AMP element) but can shrink smaller than that when necessary. The DAR should still be able to be calculated without downloading the image since you will know the original height and width (as these are mandatory for amp-img) and you know the width so browser can scale height appropriately.

@dvoytenko

This comment has been minimized.

Copy link
Collaborator

dvoytenko commented Dec 20, 2015

@mjangda thanks a lot!

@dvoytenko

This comment has been minimized.

Copy link
Collaborator

dvoytenko commented Dec 20, 2015

@bazzadp, relying on the physical width (e.g. via max-width:100%) of the image for layout is a bit ambiguous because it depends on the device pixel ratio. Instead, the layout system in AMP gives you layout=responsive and with it you have three main options to restrict maximum size:

  1. max-width CSS. Whatever you want to use as a maximum pixel size you can set this way, e.g. max-width: 320px;.
  2. You can use sizes attribute (https://developer.mozilla.org/en-US/docs/Web/HTML/Element/img#Example_4_Using_the_srcset_and_sizes_attributes). E.g. sizes="(min-width: 320px) 320px, 100vw"
  3. You can put the image inside of the container that will restrict its size. E.g. the container will instead have CSS max-width: 320px;

I put all of these examples in the http://jsbin.com/dekavu/edit?html,output

Depending on your exact case, one of these might be more convenient than others. But they are all fine. And there are many other variations that one could come up with, such as using media queries and so on. And srcset attribute is also always helpful to ensure that you have the right image downloaded depending on its layout size.

Hope this helps.

@ameshkin

This comment has been minimized.

Copy link

ameshkin commented Dec 22, 2015

Thank you guys so much for figuring all this out.

I will try some of these options tomorrow.

@rudygalfi

This comment has been minimized.

Copy link
Contributor

rudygalfi commented Jan 14, 2016

Closing this and shifting over to ampproject/amp-wp#101 for tracking.

@rudygalfi rudygalfi closed this Jan 14, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment