-
Notifications
You must be signed in to change notification settings - Fork 242
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
Add option to generate image srcsets #772
Conversation
5f611a6
to
7683496
Compare
constructor(config) { | ||
this.log = config.log; | ||
this.enabled = !!config.optimizeImages; | ||
this.imageOptimizer = config.imageOptimizer || DEFAULT_IMAGE_OPTIMIZER; |
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've also considered to always require users to provide a function and get rid of the optimizeImages flag. Not sure if the default is very useful.
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.
why is that?
My preference would be to either keep it or have a few examples documented somewheres to ease the onboarding
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.
My plan was to go with samples as well. Current thinking is to provide implementations for:
- cloudiary
- thumbor
- static site (via eleventy-img wrapper)
- custom sample
If we do this, then it probably makes sense to remove the default.
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 would downstream tools (WP, next, etc) do? provide their own functions?
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.
Correct, this is most likely something that users need to provide as frameworks usually don't optimize images by default.
const MAX_IMG_SIZE = 820; | ||
const MIN_WIDTH_TO_ADD_SRCSET_IN_RESPONSIVE_LAYOUT = 300; | ||
const NUM_SRCSET_DPR = [1.0, 2.0, 3.0]; | ||
const SRCSET_WIDTH = [ |
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.
whole lot of magic numbers, does it make sense to add a comment as to why these numbers are being used?
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 comments
/** | ||
* Sets the base width, i.e., renderered dimension measured in CSS pixels. | ||
* Returns true if srcset is needed, that is, we'll resize the image to at | ||
* least 2 legitimate widths. |
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 a legitimate width?
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 to the list of supported srcset widths
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 meant like the phrase is confusing - in what way are other widths illegitimate? why are these widths legitimate?
} | ||
|
||
/** | ||
* Returns true if there is more legitimate width. |
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.
im not sure if this means "returns true is there are additional widths that are also legitimate", or "returns true is there is a different width that is preferable to the current width"
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.
rephrased
constructor(config) { | ||
this.log = config.log; | ||
this.enabled = !!config.optimizeImages; | ||
this.imageOptimizer = config.imageOptimizer || DEFAULT_IMAGE_OPTIMIZER; |
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.
why is that?
My preference would be to either keep it or have a few examples documented somewheres to ease the onboarding
@patrickkettner PTAL |
48d9b99
to
2a5161d
Compare
2a5161d
to
64146e1
Compare
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
64146e1
to
c868d86
Compare
A Googler has manually verified that the CLAs look good. (Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.) ℹ️ Googlers: Go here for more info. |
const {skipNodeAndChildren} = require('../HtmlDomHelper'); | ||
const {isValidImageSrcURL} = require('../URLUtils'); | ||
|
||
// Don't generate srcsets for images larger than MAX_IMG_SIZE |
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.
any reason this is the max that is worth documenting? based on stats? device? processing power?
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.
Good point. Leftover from cache implementation. Changed it to the max supported value.
// All legimate srcset widths. | ||
const SRCSET_WIDTH = [39, 56, 82, 100, 150, 300, 500, 750, 1000, 1500, 2000, 2500]; | ||
|
||
// The maximum number of values. We'll take the initial image width and generate more width values by |
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.
does "values" in this context mean "maximum number of [srcset
sources]"?
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.
yes, rephrased
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
couple of comment related things, but nothing flagrant.
this.log = config.log; | ||
this.imageOptimizer = config.imageOptimizer; | ||
this.srcsetWidth = new SrcsetWidth(); | ||
// TODO turn these into options |
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.
always cool to link to a tracking bug
c868d86
to
395a2ea
Compare
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
A Googler has manually verified that the CLAs look good. (Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.) ℹ️ Googlers: Go here for more info. |
395a2ea
to
1c8ae22
Compare
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
A Googler has manually verified that the CLAs look good. (Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.) ℹ️ Googlers: Go here for more info. |
Thanks a lot for the great review @patrickkettner ! |
This adds a new OptimizeImages transformer generating srcset attributes for amp-img.
This transformer requires the following option:
imageOptimizer
: a function for customizing the srcset generation. The function should return a URL pointing to a version of thesrc
image with the givenwidth
. If no image is available, it should return a falsy value. For example: (src, width) =>${src}?width=${width}
.