Skip to content

Add experimental image preloading support#730

Merged
sebastianbenz merged 3 commits intomasterfrom
hero-preload
May 4, 2020
Merged

Add experimental image preloading support#730
sebastianbenz merged 3 commits intomasterfrom
hero-preload

Conversation

@sebastianbenz
Copy link
Copy Markdown
Collaborator

This PR adds a new transformer that uses a set of heuristics to detect an hero image on an AMP page and inject a preload directive to speed up rendering.

@sebastianbenz
Copy link
Copy Markdown
Collaborator Author

@patrickkettner forgot to mention before, the srcset parser & tests do not need to be reviewed as they're copied form amphtml.

Copy link
Copy Markdown
Collaborator

@patrickkettner patrickkettner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry it took so long

few questions, but LGTM

Comment thread packages/optimizer/lib/parseSrcSet.js
Comment thread packages/optimizer/lib/parseSrcSet.js
for (let i = 0; i < sources.length; i++) {
const source = sources[i];
let src = source.url;
if (opt_mapper) {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

im assuming we aren't exposing this to a public api? Otherwise do we need any kind of enforcement on it being a func, or is the normal opt_mapper is not a function error that would be generated sufficient?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, we are not

Comment thread packages/optimizer/lib/transformers/PreloadHeroImage.js
Comment thread packages/optimizer/lib/transformers/PreloadHeroImage.js
Comment thread packages/optimizer/lib/transformers/PreloadHeroImage.js

if (!width && !height) {
if (layout === 'fill') {
console.log('getting dim parent', this.nodeDimensionsFromParent(ampImg));
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be this.log?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed it

Comment thread packages/optimizer/lib/transformers/PreloadHeroImage.js Outdated
@sebastianbenz sebastianbenz merged commit 6d8834f into master May 4, 2020
@sebastianbenz sebastianbenz deleted the hero-preload branch May 4, 2020 18:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants