Skip to content

PX: detect hero images#1171

Merged
sebastianbenz merged 1 commit intomainfrom
data-hero
Mar 22, 2021
Merged

PX: detect hero images#1171
sebastianbenz merged 1 commit intomainfrom
data-hero

Conversation

@sebastianbenz
Copy link
Copy Markdown
Collaborator

..and suggest using data-hero attribute

@sebastianbenz
Copy link
Copy Markdown
Collaborator Author

Hm. Looks like tests are non-deterministic as they depend on browser output.

Copy link
Copy Markdown
Collaborator

@lluerich lluerich left a comment

Choose a reason for hiding this comment

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

Two little nits and as you already mentioned the the one test relies on browser output and thus fails.

*
* @param {Object} pageData
*/
const earlyIframes = (pageData, result) => {
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.

I guess this should be called heroImages or something more descriptive.

result.warn({
title: 'Improve LCP by optimizing hero images',
message:
'Let AMP Caches and Optimizers optimize your hero images, but adding the `data-hero` attribute to images in your first viewport.',
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.

*"by adding"

@sebastianbenz sebastianbenz force-pushed the data-hero branch 2 times, most recently from 893d143 to 83352ae Compare March 22, 2021 14:50
@sebastianbenz
Copy link
Copy Markdown
Collaborator Author

Thanks for the review! Fixed the nits.

..and suggest using data-hero attribute
Copy link
Copy Markdown
Collaborator

@lluerich lluerich left a comment

Choose a reason for hiding this comment

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

LGTM

@sebastianbenz sebastianbenz merged commit 6a4f2a6 into main Mar 22, 2021
@sebastianbenz sebastianbenz deleted the data-hero branch March 22, 2021 14:56
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.

2 participants