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-story-bookend] Replaces amp images to regular images in amp-story-bookend #15904
Conversation
examples/amp-story/bookendv1.json
Outdated
@@ -15,19 +15,19 @@ | |||
"type": "small", | |||
"title": "This is an example article", | |||
"url": "http://example.com/article.html", | |||
"image": "http://placehold.it/256x128" | |||
"image": "http://placehold.it/100x100" |
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 liked the idea of having images that don't fit their container, it makes it easier to notice any cropping issue.
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.
Done.
|
||
addAttributesToElement(ampImg, dict({'src': articleData.image})); | ||
el.insertBefore(ampImg, el.firstChild); | ||
<div class="i-amphtml-story-bookend-article-image"> |
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.
Here and in the other templates: Can we get rid of this (unnecessary?) div?
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.
We need it make the images take the full width and height of its available space. Also to do the padding-bottom
css trick on the portrait and landscape cards to keep a width and expand/decrease the height.
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.
Perfect, thanks :)
@newmuis PTAL |
Fixes #15313