feat(common): Add fill mode to NgOptimizedImage #47738

atcastle · 2022-10-11T20:44:17Z

This PR adds 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

CC: @AndrewKushnir @pkozlowski-opensource @kara

kara

LGTM

kara · 2022-10-11T21:02:42Z

aio/content/guide/image-directive.md

+
+In cases where you want to have an image fill a containing element, you can use the `fill` attribute. This is often useful when you want to achieve a "background image" behavior, or when you don't know the exact width and height of your image.
+
+When you add the `fill` attribute to your image, you do not need and should not include a `width` and `height`, as in this example:


(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

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

LGTM 👍

I've left some comments, but they can all be addressed in a followup PR.

AndrewKushnir · 2022-10-12T05:17:07Z

packages/common/src/directives/ng_optimized_image/ng_optimized_image.ts

@@ -330,6 +336,19 @@ export class NgOptimizedImage implements OnInit, OnChanges, OnDestroy {
  }
  private _disableOptimizedSrcset = false;

+  /**
+   * Sets the image to "fill mode," which eliminates the height/width requirement and adds


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

AndrewKushnir · 2022-10-12T05:21:09Z

packages/common/src/directives/ng_optimized_image/ng_optimized_image.ts

    // 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());
+    }


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 sizes as 100vw (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.

AndrewKushnir · 2022-10-12T05:22:32Z

packages/common/src/directives/ng_optimized_image/ng_optimized_image.ts

+            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.`);


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

Reviewed-for: public-api

AndrewKushnir · 2022-10-12T05:30:29Z

Merge-assistance (cc @jessicajaniuk):

The PR was reviewed and approved, the only missing item is one more pubic-api approval. Could you please take a quick look at the changes from the pubic-api perspective (the changes are pretty small, just an extra @Input)?
There is some minor feedback (mostly docs-related), which we'll take care of in a followup PR.

Overall status (after public-api approval) of this PR: ready for merge.

jessicajaniuk

reviewed-for: public-api

jessicajaniuk · 2022-10-12T15:56:58Z

This PR was merged into the repository by commit 9483343.

angular-automatic-lock-bot · 2022-11-12T16:06:23Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

pullapprove bot requested a review from AndrewKushnir October 11, 2022 20:44

angular-robot bot added the feature Issue that requests a new feature label Oct 11, 2022

atcastle mentioned this pull request Oct 11, 2022

[draft] Add 'fill mode' to NgOptimizedImage #47727

Closed

kara approved these changes Oct 11, 2022

View reviewed changes

atcastle force-pushed the fill-mode-2 branch from d1c74ec to 8988259 Compare October 11, 2022 21:12

jessicajaniuk added the area: common Issues related to APIs in the @angular/common package label Oct 11, 2022

ngbot bot added this to the Backlog milestone Oct 11, 2022

atcastle force-pushed the fill-mode-2 branch 2 times, most recently from cf716ca to aab6ef8 Compare October 11, 2022 22:00

atcastle force-pushed the fill-mode-2 branch from aab6ef8 to 4c3ec1c Compare October 11, 2022 22:52

jessicajaniuk added the target: major This PR is targeted for the next major release label Oct 11, 2022

AndrewKushnir approved these changes Oct 12, 2022

View reviewed changes

pullapprove bot requested review from alxhub, AndrewKushnir and pkozlowski-opensource October 12, 2022 05:24

AndrewKushnir approved these changes Oct 12, 2022

View reviewed changes

AndrewKushnir added action: merge The PR is ready for merge by the caretaker merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note labels Oct 12, 2022

jessicajaniuk approved these changes Oct 12, 2022

View reviewed changes

jessicajaniuk added merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note and removed merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note labels Oct 12, 2022

jessicajaniuk closed this in 9483343 Oct 12, 2022

angular-automatic-lock-bot bot locked and limited conversation to collaborators Nov 12, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(common): Add fill mode to NgOptimizedImage #47738

feat(common): Add fill mode to NgOptimizedImage #47738

atcastle commented Oct 11, 2022

kara left a comment

kara Oct 11, 2022

AndrewKushnir left a comment

AndrewKushnir Oct 12, 2022

AndrewKushnir Oct 12, 2022

AndrewKushnir Oct 12, 2022

AndrewKushnir left a comment

AndrewKushnir commented Oct 12, 2022

jessicajaniuk left a comment

jessicajaniuk commented Oct 12, 2022

angular-automatic-lock-bot bot commented Nov 12, 2022


		In cases where you want to have an image fill a containing element, you can use the `fill` attribute. This is often useful when you want to achieve a "background image" behavior, or when you don't know the exact width and height of your image.

		When you add the `fill` attribute to your image, you do not need and should not include a `width` and `height`, as in this example:

	* 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

	`element, the size attributes have no effect and should be removed.`);
	`element, the width/height attributes have no effect and should be removed.`);

feat(common): Add fill mode to NgOptimizedImage #47738

feat(common): Add fill mode to NgOptimizedImage #47738

Conversation

atcastle commented Oct 11, 2022

kara left a comment

Choose a reason for hiding this comment

kara Oct 11, 2022

Choose a reason for hiding this comment

AndrewKushnir left a comment

Choose a reason for hiding this comment

AndrewKushnir Oct 12, 2022

Choose a reason for hiding this comment

AndrewKushnir Oct 12, 2022

Choose a reason for hiding this comment

AndrewKushnir Oct 12, 2022

Choose a reason for hiding this comment

AndrewKushnir left a comment

Choose a reason for hiding this comment

AndrewKushnir commented Oct 12, 2022

jessicajaniuk left a comment

Choose a reason for hiding this comment

jessicajaniuk commented Oct 12, 2022

angular-automatic-lock-bot bot commented Nov 12, 2022