add experimental amp-img -> img transformation #859
Conversation
| @@ -110,8 +110,7 @@ function maybeAddSizerInto(node, layout, width, height) { | |||
| sizer = createIntrinsicSizer(width, height); | |||
| } | |||
| if (sizer) { | |||
| const referenceNode = node.firstChild; | |||
| insertBefore(node, sizer, referenceNode); | |||
| appendChild(node, sizer); | |||
There was a problem hiding this comment.
Looks like the sizer needs to be added after the img.
There was a problem hiding this comment.
Sizer always goest first for responsive. Is there a bug you're seeing when sizer is first?
There was a problem hiding this comment.
Hm. Not sure what I tested there. All good with sizer first.
| @@ -110,8 +110,7 @@ function maybeAddSizerInto(node, layout, width, height) { | |||
| sizer = createIntrinsicSizer(width, height); | |||
| } | |||
| if (sizer) { | |||
| const referenceNode = node.firstChild; | |||
| insertBefore(node, sizer, referenceNode); | |||
| appendChild(node, sizer); | |||
There was a problem hiding this comment.
Sizer always goest first for responsive. Is there a bug you're seeing when sizer is first?
| return; | ||
| } | ||
| const imgNode = createElement('img', { | ||
| class: 'class=i-amphtml-fill-content i-amphtml-replaced-content', |
There was a problem hiding this comment.
| class: 'class=i-amphtml-fill-content i-amphtml-replaced-content', | |
| class: 'i-amphtml-fill-content i-amphtml-replaced-content', |
| generateImg(node) { | ||
| if (!this.experimentImg) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
I think we were only going to do this transform for amp-imgs that have a data-hero attr set. Since we can't guarantee loading=lazy behavior for most browsers yet, we don't want to load a bunch of far-from-viewport images.
There was a problem hiding this comment.
Introduce a max upper bound of 2 hero images and switched to loading=eager for these.
This commit introduces the data-hero attribute as a means to explicitly markup hero amp-img or amp-iframe elemenents. Other changes: * don't bail out if there are any kind of image preloads, instead check the img src to avoid duplicates * use loading=eager instead of loading=lazy
|
Adressed the comments, also:
PTAL |
| const imgNode = createElement('img', { | ||
| class: 'i-amphtml-fill-content i-amphtml-replaced-content', | ||
| decoding: 'async', | ||
| loading: 'eager', |
There was a problem hiding this comment.
I think we can drop this attr.
|
@jridgewell Thanks for the review! |
This commit introduces the data-hero attribute as a means to explicitly markup hero amp-img or amp-iframe elemenents. Other changes: * don't bail out if there are any kind of image preloads, instead check the img src to avoid duplicates * don't use loading=lazy * print warning if there are too many hero elements on a page
Introduce data-hero attribute which will disable auto hero detection.