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

Issue using AMP #750

Closed
miguelcolomer opened this issue Oct 25, 2015 · 14 comments
Closed

Issue using AMP #750

miguelcolomer opened this issue Oct 25, 2015 · 14 comments

Comments

@miguelcolomer
Copy link

Hi,
i have a Blogger's page and i'm trying to use it with AMP. This is the problem that i have and the steps:

  1. I've included the <script async='true' src="https://cdn.ampproject.org/v0.js"></script> before the

  2. When i'm using the code, i have to use the width and the height. But those values are "calculated" and they are responsive.

  • So if i write this for my logo:

I get obviusly the error in my Chrome's developer console: "Expected width to be available and be an integer/length value: 180px; "

  1. If i use the width and height attributes i don't have that problem:

But my image needs to be responsive and i can't use it with those fixed values, what can i do?

@erwinmombay
Copy link
Member

hi @miguelcolomer so assigning a height and width doesn't mean its fixed. you can add for example layout=responsive and it will scale gracefully. One of the reason why we need the original width and height is so we can reserve the space for the image (so that we don't cause any reflows when the image actually arrives) as well as to scale it correctly. There are other layout options that might fit your requirements more so don't hesitate to ask (some that don't require a height and width).

see https://github.com/ampproject/amphtml/blob/master/spec/amp-html-layout.md for further layout attributes and properties. 😸

@erwinmombay
Copy link
Member

btw @miguelcolomer if you could repost your code samples (it doesn't look like its showing), we'd be able to help you out more if you still need it. thanks

@miguelcolomer
Copy link
Author

Ok,
sorry, i'm veru newbie with this. This is my code, i'll try to summarize it to be more effective:

i've added the kayout=responsive but it's not working with my header logo.

I'm using this css to my header inner:
#header-inner {
background-position: center;
margin-left: auto;
margin-right: auto;
}

And then i'm usign this code to use the amp-img tag:

This works ok but it's not responsive:

div id='header-inner'>
h1> a expr:href='data:blog.homepageUrl' style='display: block'>
amp-img expr:alt='data:title' expr:height='42' expr:id='data:widget.instanceId + "_headerimg"' expr:src='data:sourceUrl' expr:width='180' layout='responsive' style='display: block;padding-left:0px;padding-top:0px;'/> /a> /h1>
/div>

This is with layout=responsive option... but that doesn't work:

div id='header-inner'>
h1> a expr:href='data:blog.homepageUrl' style='display: block'>
amp-img expr:alt='data:title' expr:height='42' expr:id='data:widget.instanceId + "_headerimg"' expr:src='data:sourceUrl' expr:width='180' layout='responsive' /> /a> /h1>
/div>

Is this sufficient to help me or you would need all my code? (it's a lot of Blogger's XHTML code). i've deleted the "<" markup because if i don't do that i can post the source code.

Sorry i'm newbie and i don't know how to work well in this forum, i'm trying to use amp with my blogger code and try to explain it for people in my country.

Thank you very much for your work and patience!

@erwinmombay erwinmombay reopened this Oct 25, 2015
@erwinmombay
Copy link
Member

@miguelcolomer np, thank you for your patience as well. so i don't see your code still (github must be doing some sort of escaping)

could you go to http://codepen.io/anon/pen/Qjmvmv?editors=100, fork it and then add the problematic scenario you're running into? Thanks again!

@miguelcolomer
Copy link
Author

Ok, how can i save my code to explain it there?, i have to sign there and then give you a link? or can i wirite there directly my problematic code?

Thanks again!

@erwinmombay
Copy link
Member

@miguelcolomer so you need to fork the code (theres a fork button at the top), add your code (css and problematic markup/am-img scenario) and just press save again, grab the new link from the url and send me the link here so we can investigate. 😸 Thanks!

@miguelcolomer
Copy link
Author

Ok, i've done it!:

https://gist.github.com/anonymous/6f70e57b681b2c15479a

Please review lines 2909 to 2912. If i include "layout=responsive" it not works and i have to add manually the value for widht and height due to amp img specification. I need that image to be responsive. What can i do?

Thank you very much again!!!!

@erwinmombay
Copy link
Member

@miguelcolomer so having the width and height + layout=responsive should make it responsive. does it not resize properly? see http://jsbin.com/gixizerufe/1/edit?html,output (try resizing your browser screen). the gist you pasted is a little hard to parse through since its pretty big, would make it easier if the problem was in isolation.

also make sure to close the amp-img tag.

wrong:

<amp img src="" />

correct:

<amp-img src=""></amp-img>

and then remove the inline style as they are not allowed.

@dvoytenko
Copy link
Contributor

@miguelcolomer another easy way for us to advise you on this is if you can send us a link to a working (though assuming incorrectly working) page. That way we can dig through it using devtools.

@miguelcolomer
Copy link
Author

Ok, i've add the

the closing markup for <amp-img> with </amp-img>:

<div id='header-inner'>
      <h1><a expr:href='data:blog.homepageUrl' style='display: block'>
        <amp-img expr:alt='data:title' expr:height='42' expr:id='data:widget.instanceId + &quot;_headerimg&quot;' expr:src='data:sourceUrl' expr:width='180' style='display: block;padding-left:0px;padding-top:0px;'></amp-img> </a></h1>
      </div>

Now the image it's correct for resolutions for more than 637p but if i try to make the image for resolutions lower than 637p the logo it doesn't fit the container and i have to use @media query instead "layout=responsive".

When i add the layout=responsive code to the amp-img, the image doesn't work.

      <div id='header-inner'>
      <h1><a expr:href='data:blog.homepageUrl' style='display: block'>
        <amp-img expr:alt='data:title' expr:height='42' expr:id='data:widget.instanceId + &quot;_headerimg&quot;' expr:src='data:sourceUrl' expr:width='180' layout='responsive' style='display: block;padding-left:0px;padding-top:0px;'></amp-img> </a></h1>
      </div>

Take into account that i have to represent the sourcecode in HTML for you because it's Blogger and it's done with XHTML. Anyway, if you find the problem i can change it in HTML withou any problems.

You can see what i said in my page http://www.diariosdelanube.com , if you press F12 and add layout='responsive' in the HTML code you'll see how the logo disappear.

What can i do?.

@miguelcolomer
Copy link
Author

Hi again,
i've fixed using media queries instead of using layout='responsive' but i don't know why it's not working that attribute in Blogger... You can investigate it with the previos code that i have published in the comments and comparing with the actual code in my web.

Anything you need to discover the problem, please, let me know.

I'll continue applying amp with the Ads in my blog.

Let's continue with this issue opened until we found a fix for this.

Thank you!

@miguelcolomer
Copy link
Author

Let me isolate the problem making a summary:

Issues detected in Blogger when trying to use amp-img:

  1. The Blogger's source code it's like this without amp-img tag:

This "Blogger's code" (XHTML) display the logo for my header-inner's page:

<div id='header-inner'>
        <a expr:alt='data:title' expr:href='data:blog.homepageUrl' style='display: block'>
          <img expr:alt='data:title' expr:height='data:height' expr:id='data:widget.instanceId + &quot;_headerimg&quot;' expr:src='data:sourceUrl' expr:width='data:width' style='display: block;padding-left:0px;padding-top:0px;'/>
        </a>
      </div>

This is the HTML code for the previous Blogger's code:

<div id="header-inner">
<h1><a href="http://www.diariosdelanube.com/" style="display: block">
<img alt="Diarios de la nube" height="42" id="Header1_headerimg" src="http://1.bp.blogspot.com/-Df4TGR8Fa5s/VUftNcmaEzI/AAAAAAAAQJU/De2X74Pru2E/s1600/Logo_pruebas5.png" style="display: block;padding-left:0px;padding-top:0px;" width="180">
</a></h1>
</div>
  1. To make the amp code, i've included the .js before the tag of my Blogger's code:
<script async='true' src='https://cdn.ampproject.org/v0.js'/>
  1. After that change, i have recoded with AMP specifications my Blogger's code:
<div id='header-inner'>
      <h1><a expr:href='data:blog.homepageUrl' style='display: block'>
        <amp-img expr:alt='data:title' expr:height='42' expr:id='data:widget.instanceId + &quot;_headerimg&quot;' expr:src='data:sourceUrl' expr:width='180' style='display: block;padding-left:0px;padding-top:0px;'></amp-img> </a></h1>
      </div>

NOTE: if i don't change expr:height='data:height' and expr:width='data:width' with expr:height='42' and ' expr:width='180' the AMP .js show in the Chrome's Developer console an error because needs those values to show the image.

  1. Because i need this image to be responsive, i've add layout='responsive' but it isn't works:
 <div id='header-inner'>
      <h1><a expr:href='data:blog.homepageUrl' style='display: block'>
        <amp-img expr:alt='data:title' expr:height='42' expr:id='data:widget.instanceId + &quot;_headerimg&quot;' expr:src='data:sourceUrl' expr:width='180' style='display: block;padding-left:0px;padding-top:0px;' layout='responsive'></amp-img> </a></h1>
      </div>

I have to fix the problem using media queries instead of layout= responsive.

  • What i have to do to use layout=responsive correctly?.

Thank you very much!.

@dvoytenko
Copy link
Contributor

@miguelcolomer I see the issue. The problem is with this definition:

#header {
  float: left;
}

The layout=responsive (described in https://github.com/ampproject/amphtml/blob/master/spec/amp-html-layout.md) works by taking the width available to it and resizing its height proportionally to the aspect ration defined by width:height attributes. The problem with float:left is that it communicates 0 available width to its children by its definition. So, it does block itself on what its children might need. This however, blocks rendering until the images are downloaded and forces relayouts and other performance problems. And thus AMP does not allow it.

I'm not sure exactly why float:left is used in that place and if it's necessary. If you'd like to continue to use it, you can add min/max-width CSS and that would communicate to layout=responsive a valid width to work it. Alternatively, where float:left was used in the past, a lot of times flexbox now does a better job and with better performance. But that, of course, depends on your needs. One thing is for certain, though, I'd strongly recommend arriving at a clear sizing of logos and not relying on image being downloaded. Often times background-image and svg are used for logos - and they all require CSS-resolvable sizing and this is what we require in AMP as well. You can switch to background-image and see how to solve your problem that way. While it'd work perfectly well, the main use case for amp-img are content images.

@miguelcolomer
Copy link
Author

Great!, i'll do that and i'll update here my results.

Thank you very much!!!

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