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
feat(common): add automatic srcset feature to ngOptimizedImage #47547
Conversation
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.
Thanks for the PR! Great basis for discussion on srcset design - let's continue in the chat?
packages/common/src/directives/ng_optimized_image/ng_optimized_image.ts
Outdated
Show resolved
Hide resolved
packages/common/src/directives/ng_optimized_image/ng_optimized_image.ts
Outdated
Show resolved
Hide resolved
aio/content/guide/image-directive.md
Outdated
If the `ngSrcset` attribute is present, `NgOptimizedImage` generates and sets the [`srcset` attribute](https://developer.mozilla.org/en-US/docs/Web/API/HTMLImageElement/srcset) using the configured image loader. Do not include image file names in `ngSrcset` - the directive infers this information from `ngSrc`. The directive supports both width descriptors (e.g. `100w`) and density descriptors (e.g. `1x`) are supported. | ||
|
||
You can also use `ngSrcset` with the standard image [`sizes` attribute](https://developer.mozilla.org/en-US/docs/Web/API/HTMLImageElement/sizes). | ||
If the `ngSrcset` attribute is present, `NgOptimizedImage` generates and sets the using the configured image loader. Do not include image file names in `ngSrcset` - the directive infers this information from `ngSrc`. The directive supports both width descriptors (e.g. `100w`) and density descriptors (e.g. `1x`) are supported. |
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.
How do ngSrcset
and the generated srcset
intersect? Is there any reason to keep ngSrcset
around if you can define custom breakpoints anyway?
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.
Right now if you use ngSrcset
the directive will just use that value, and won't use the automatic srcset. I'm not sure if we need to keep providing the ngSrcset convenience API or not. It could be useful for some people, but it also does complicate the API
// the smallest percent size in the `sizes` prop, multiplying it by the smallest | ||
// size in the viewport prop, and removing anything smaller from the subviewport | ||
// breakpoints. | ||
const vwRegExp = /(^|\s)(1?\d?\d)vw/g; |
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.
How necessary do we feel this trimming is? It seems like more computation to do for every image, and I'm not sure it's worth saving a few bytes in the document.
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.
Also what happens if the "sizes" attribute contains a fancy query (e.g. "(max: 500px) 400w" type thing)? Do we want to support that? I guess we'd have to throw it contained a value that wasn't in our breakpoint list
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.
Yeah, I'd be ok with removing this logic. Especially in a CSR scenario, there's not that much benefit to trimming those extra URLs. Though, it could make some sort of a difference on a page with a lot of images doing SSR. A few of the smallest breakpoints (16, 32) tend get trimmed almost all the time.
As for the mixed queries--I don't think that's super common, and I also think they're a pretty good case for where someone would want to shut off the automated srcset entirely. If you're using sizes with that level of specificity, you probably also want to manually define your srcset.
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.
Let's remove this trimming logic for now? We can discuss and add it in a future PR if we think it's necessary
packages/common/src/directives/ng_optimized_image/ng_optimized_image.ts
Outdated
Show resolved
Hide resolved
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.
@atcastle thanks for creating this PR! 👍 Just left a few comments.
packages/common/src/directives/ng_optimized_image/ng_optimized_image.ts
Outdated
Show resolved
Hide resolved
packages/common/src/directives/ng_optimized_image/ng_optimized_image.ts
Outdated
Show resolved
Hide resolved
* Disables automatic srcset generation for this image. | ||
*/ | ||
@Input() | ||
set unoptimized(value: string|boolean|undefined) { |
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 we make this input name more specific? Currently it may sound like it disables all optimizations. May be mimic the config property and call it disableAutoSrcset
, WDYT?
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.
Hmm I like the idea of making it more specific, but I also want to make it clear that this pattern is not recommended. What about "unoptimizedSrcset"?
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.
To me, unoptimizedSrcset
sort of implies that you get a srcset, but that it's not optimized. But with this attribute you actually get nothing, srcset-wise.
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.
Oh, that's a good point. I can see it being read that way 🤔. I'm good with "unoptimized"
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 think I'm still a bit concerned that we may be delivering a wrong message via an input name. The unoptimized
sounds more like the entire optimization logic (including loaders, etc) is disabled. Can we chat about this during the next sync?
It'd be ok to proceed with the PR as is and update it later (before the feature freeze).
packages/common/src/directives/ng_optimized_image/ng_optimized_image.ts
Outdated
Show resolved
Hide resolved
packages/common/src/directives/ng_optimized_image/ng_optimized_image.ts
Outdated
Show resolved
Hide resolved
packages/common/src/directives/ng_optimized_image/ng_optimized_image.ts
Outdated
Show resolved
Hide resolved
packages/common/src/directives/ng_optimized_image/ng_optimized_image.ts
Outdated
Show resolved
Hide resolved
packages/common/src/directives/ng_optimized_image/ng_optimized_image.ts
Outdated
Show resolved
Hide resolved
// the smallest percent size in the `sizes` prop, multiplying it by the smallest | ||
// size in the viewport prop, and removing anything smaller from the subviewport | ||
// breakpoints. | ||
const vwRegExp = /(^|\s)(1?\d?\d)vw/g; |
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.
Let's remove this trimming logic for now? We can discuss and add it in a future PR if we think it's necessary
packages/common/src/directives/ng_optimized_image/ng_optimized_image.ts
Outdated
Show resolved
Hide resolved
packages/common/src/directives/ng_optimized_image/ng_optimized_image.ts
Outdated
Show resolved
Hide resolved
* @see `NgOptimizedImage` | ||
* @developerPreview | ||
*/ | ||
export const IMAGE_CONFIG = new InjectionToken<ImageConfig>( |
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.
Question: do we want people to provide different breakpoint values in the different parts of the application? Or is the intention that one set of breakpoints applies to the entire application?
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 believe the main use case for doing this is to match a configuration on your CDN. Since it's possible to have different CDNs for different sections of an application (one for product images, another for static images, for instance) I think it makes sense to be able to set this differently for different parts.
2b3660b
to
31155db
Compare
31155db
to
99ad054
Compare
a2cc152
to
a44deda
Compare
packages/common/src/directives/ng_optimized_image/ng_optimized_image.ts
Outdated
Show resolved
Hide resolved
packages/common/src/directives/ng_optimized_image/ng_optimized_image.ts
Outdated
Show resolved
Hide resolved
* Disables automatic srcset generation for this image. | ||
*/ | ||
@Input() | ||
set unoptimized(value: string|boolean|undefined) { |
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 think I'm still a bit concerned that we may be delivering a wrong message via an input name. The unoptimized
sounds more like the entire optimization logic (including loaders, etc) is disabled. Can we chat about this during the next sync?
It'd be ok to proceed with the PR as is and update it later (before the feature freeze).
packages/common/src/directives/ng_optimized_image/ng_optimized_image.ts
Outdated
Show resolved
Hide resolved
@@ -60,18 +60,18 @@ export class ImageDistortionPassingComponent { | |||
<!-- These images should throw --> | |||
<!-- Supplied aspect ratio differs from intrinsic aspect ratio by > .1 --> | |||
<!-- Aspect-ratio: 0.86666666666 --> | |||
<img ngSrc="/e2e/b.png" width="26" height="30"> | |||
<img ngSrc="/e2e/b.png" width="26" height="30" unoptimized> |
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.
Just curious why we need to add unoptimized
here?
a44deda
to
3876b41
Compare
Latest update to this PR changes |
packages/common/src/directives/ng_optimized_image/ng_optimized_image.ts
Outdated
Show resolved
Hide resolved
Add a feature to automatically generate the srcset attribute for images using the NgOptimizedImage directive. Uses the 'sizes' attribute to determine the appropriate srcset to generate.
3876b41
to
33dbbf5
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.
Reviewed-for: public-api
Merge-assistance (@jessicajaniuk): this PR is almost ready for merge, needs one more |
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.
reviewed-for: public-api
This PR was merged into the repository by commit 4fde292. |
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
This PR adds a feature for automatic srcset generation to the NgOptimizedImage directive. Srcsets are generated as density selectors (eg "1x", "2x") if there is no
sizes
prop provided, or with responsive srcsets with a range of pixel values if asizes
prop exists. These responsive breakpoints are user-configurable using a new provider called ImageConfig.The automatic srcset can be disabled with a new
unoptimized
attribute, or via the ImageConfig provider.Note: This PR only contains unit testing of this feature. End-to-end tests will be added shortly. Opening this now to begin discussion with @pkozlowski-opensource and @AndrewKushnir.
// CC: @kara
PR Type
What kind of change does this PR introduce?
Does this PR introduce a breaking change?