feat(common): Add fill mode to NgOptimizedImage#47738
Closed
atcastle wants to merge 1 commit intoangular:mainfrom
Closed
feat(common): Add fill mode to NgOptimizedImage#47738atcastle wants to merge 1 commit intoangular:mainfrom
atcastle wants to merge 1 commit intoangular:mainfrom
Conversation
kara
approved these changes
Oct 11, 2022
aio/content/guide/image-directive.md
Outdated
Contributor
There was a problem hiding this comment.
(can be follow up PR): can we add something here about why width and height aren't necessary? e.g. that we usually require them to prevent layout shift but here it's unnecessary
cf716ca to
aab6ef8
Compare
Add a new boolean attribute to NgOptimizedImage called `fill` which does the following: * Removes the requirement for height and width * Adds inline styling to cause the image to fill its containing element * Adds a default `sizes` value of `100vw` which will cause the image to have a responsive srcset automatically generated
AndrewKushnir
approved these changes
Oct 12, 2022
Contributor
AndrewKushnir
left a comment
There was a problem hiding this comment.
LGTM 👍
I've left some comments, but they can all be addressed in a followup PR.
| private _disableOptimizedSrcset = false; | ||
|
|
||
| /** | ||
| * Sets the image to "fill mode," which eliminates the height/width requirement and adds |
Contributor
There was a problem hiding this comment.
nit: typo, let's fix it in a followup PR.
Suggested change
| * Sets the image to "fill mode," which eliminates the height/width requirement and adds | |
| * Sets the image to "fill mode", which eliminates the height/width requirement and adds |
Comment on lines
407
to
+416
| // Must set width/height explicitly in case they are bound (in which case they will | ||
| // only be reflected and not found by the browser) | ||
| this.setHostAttribute('width', this.width!.toString()); | ||
| this.setHostAttribute('height', this.height!.toString()); | ||
| if (this.fill) { | ||
| if (!this.sizes) { | ||
| this.sizes = '100vw'; | ||
| } | ||
| } else { | ||
| this.setHostAttribute('width', this.width!.toString()); | ||
| this.setHostAttribute('height', this.height!.toString()); | ||
| } |
Contributor
There was a problem hiding this comment.
nit: I think this section would require some extra comments (and updates to existing ones):
- the width/height comment can be updated to better reflect the logic
- it's be great to add a comment on why we define
sizesas100vw(related question: what happens when the sizes are defined differently by a user?) - we could also add a quick comment above the code that sets width/height and explain why we don't do it for "fill" mode
We could do it in a followup PR as well.
| imgDirectiveDetails( | ||
| dir.ngSrc)} the attributes \`height\` and/or \`width\` are present ` + | ||
| `along with the \`fill\` attribute. Because \`fill\` mode causes an image to fill its containing ` + | ||
| `element, the size attributes have no effect and should be removed.`); |
Contributor
There was a problem hiding this comment.
nit: the size might be confused with the sizes input, so we can rephrase this a bit:
Suggested change
| `element, the size attributes have no effect and should be removed.`); | |
| `element, the width/height attributes have no effect and should be removed.`); |
AndrewKushnir
approved these changes
Oct 12, 2022
Contributor
AndrewKushnir
left a comment
There was a problem hiding this comment.
Reviewed-for: public-api
Contributor
|
Merge-assistance (cc @jessicajaniuk):
Overall status (after |
jessicajaniuk
approved these changes
Oct 12, 2022
Contributor
jessicajaniuk
left a comment
There was a problem hiding this comment.
reviewed-for: public-api
Contributor
|
This PR was merged into the repository by commit 9483343. |
|
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 file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR adds a new boolean attribute to NgOptimizedImage called
fillwhich does the following:sizesvalue of100vwwhich will cause the image to have a responsive srcset automatically generatedCC: @AndrewKushnir @pkozlowski-opensource @kara