Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

PX: detect hero images #1171

Merged
merged 1 commit into from
Mar 22, 2021
Merged

PX: detect hero images #1171

merged 1 commit into from
Mar 22, 2021

Conversation

sebastianbenz
Copy link
Collaborator

..and suggest using data-hero attribute

@sebastianbenz
Copy link
Collaborator Author

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

Copy link
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
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
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
Collaborator Author

Thanks for the review! Fixed the nits.

..and suggest using data-hero attribute
Copy link
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
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants