-
Notifications
You must be signed in to change notification settings - Fork 243
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
More flexible hero images and blurry placeholders #1133
Conversation
2d3a325
to
5d3fb3c
Compare
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.
lgtm, just some nits
class OptimizeHeroImage { | ||
constructor(config) { | ||
this.log = config.log; | ||
this.enabled = config.optimizeHeroImages !== false || config.preloadHeroImage !== false; |
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.
should a warning be emitted about using preloadHeroImage
?
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
packages/optimizer/README.md
Outdated
|
||
#### `preloadHeroImage` | ||
|
||
Deprecated, use `optimizeHeroImages` instead. |
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.
can you []()
link to the section, please
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
packages/optimizer/README.md
Outdated
|
||
If no `data-hero` attribute is present, AMP optimizer auto-detects hero images for `amp-img`, `amp-iframe`, `amp-video`, or `amp-video-iframe` and injects a `link rel=preload`. Image preload links will only be generated if there is none already existing. For `amp-img` elements, it will also server-side render the `img` element inside the `amp-img` element. This greatly improves image rendering performance and reduces the [largest contentful paint](https://web.dev/lcp/) (LCP) metric from [Core Web Vitals](https://web.dev/vitals/). | ||
Hero images are optimized by server-side rendering the `img` element inside the `amp-img` element. This greatly improves image rendering performance and reduces the [largest contentful paint](https://web.dev/lcp/) (LCP) metric from [Core Web Vitals](https://web.dev/vitals/). |
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.
IANAL, but I think "This [can/may] greatly improve"/"normally reduces", etc is more accurate, no? or are we 100% certain of this
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'm 100% certain, but might miss some corner cases. Rephrased.
} | ||
|
||
// check whether all required dependencies are installed | ||
this.missingDependencies_ = false; |
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.
what is this
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.
added comment
appendChild(node, imgNode); | ||
} | ||
} | ||
/* This transfomer has been deprecated, use OptimizeHeroImages.js instead */ |
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.
who is this message for? Should it be logged, rather than a comment?
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.
Added logging for the options. We don't have access to the log here.
This PR makes image optimization more flexible by making it possible to configure hero image count and blurry image placeholder generation on a per page basis. This PR also deprecates the preloadHeroImage option in favor of a new optimizeHeroImages option as preloading has been disabled in most cases (see #1132).
f7c3bb8
to
b37e1f6
Compare
Thanks for the review! |
This PR makes image optimization more flexible by making it possible to
configure hero image count and blurry image placeholder generation on a
per page basis.
This PR also deprecates the preloadHeroImage option in favor of a new
optimizeHeroImages option as preloading has been disabled in most cases (see
#1132).