-
Notifications
You must be signed in to change notification settings - Fork 243
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
Fix i-amphtml-sizer responsive issue #1319
Fix i-amphtml-sizer responsive issue #1319
Conversation
<i-amphtml-sizer slot="i-amphtml-svc" style="display:block;padding-top:100%"></i-amphtml-sizer> | ||
<i-amphtml-sizer slot="i-amphtml-svc" style="display:block"></i-amphtml-sizer> | ||
</amp-img> | ||
<amp-img id="customId" height="100" width="100" layout="responsive" src="img2.png" class="i-amphtml-layout-responsive i-amphtml-layout-size-defined" i-amphtml-layout="responsive"> | ||
<i-amphtml-sizer slot="i-amphtml-svc" style="display:block;padding-top:100%"></i-amphtml-sizer> | ||
<i-amphtml-sizer slot="i-amphtml-svc" style="display:block"></i-amphtml-sizer> | ||
</amp-img> | ||
<amp-img height="100" width="100" layout="responsive" src="img3.png" class="i-amphtml-layout-responsive i-amphtml-layout-size-defined" i-amphtml-layout="responsive" id="i-amp-1"> | ||
<i-amphtml-sizer slot="i-amphtml-svc" style="display:block;padding-top:100%"></i-amphtml-sizer> | ||
<i-amphtml-sizer slot="i-amphtml-svc" style="display:block"></i-amphtml-sizer> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The elements in input.html
do not have heights
attributes. Is this change properly being limited to when heights
is present?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In amphtml source I've found when the padding-top is being set. I've update the code and now padding-top will set when heights
attribute is missing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see there in static-layout.js
where the presence of the heights
attribute determines whether padding-top
is added. Maybe when the heights
attribute is processed by the runtime, the padding-top
later is removed/overridden? This appears so:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the confusion. I wanted to say the same. Yes, you're right. applyStaticLayout creates the sizer element first, then adds the padding-top based on the height and width attributes. Later it gets overridden if the heights attribute is present.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - thanks for fixing! One nit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks a lot! |
Fixes #1314
This PR fixes the responsive issue of the
i-amphtml-sizer
generated by the SSR when we have aheights
attribute.