feat(common): add placeholder to NgOptimizedImage #53783

atcastle · 2024-01-03T20:21:29Z

This PR adds a new automatic placeholders feature to NgOptimizedImage. This feature takes a small placeholder image, generated with an image loader, or provided by the developer in the form of a data URL, and applies it as a background-image with a blur while the primary image is being downloaded. The placeholder styling is removed when loading is complete.

CC: @AndrewKushnir @kara

AndrewKushnir

@atcastle looks great, just left a few minor comments.

adev/src/content/guide/image-optimization.md

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

packages/common/test/directives/ng_optimized_image_spec.ts

packages/common/src/errors.ts

packages/common/test/directives/ng_optimized_image_spec.ts

kara

Overall, looks good, with some minor changes requested to docs/comments/tests.

One meta-note: this PR could have been split up into 3 PRs: one for basic setup, one for additional configs (e.g. placeholderConfig, resolution), and one for warnings/errors. This makes it a lot easier to review :)

adev/src/content/guide/image-optimization.md

aio/content/guide/image-directive.md

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

adev/src/content/guide/image-optimization.md

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

kara · 2024-01-04T20:10:51Z

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

+ * attribute.
+ */
+export const DATA_URL_WARN_LIMIT = 2000;
+export const DATA_URL_ERROR_LIMIT = 5000;


Where are these limit numbers coming from?

+1, it would be good to have any links to reference material or an explanation of how they were determined via experiment, trial-and-error, etc.

Agreed. My heuristic for choosing them was to convert some very small images to base64 and see what the smallest reasonable character count is. I'll share some example sizes for different sized source images.

+1 sharing a few example sizes would help give context

packages/common/test/directives/ng_optimized_image_spec.ts

adev/src/content/guide/image-optimization.md

aio/content/guide/image-directive.md

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

jelbourn · 2024-01-05T00:40:24Z

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

+   * Configuration object for placeholder settings. Options:
+   *   blur: Setting this to false disables the automatic CSS blur.
+   */
+  @Input() placeholderConfig?: {blur: boolean} = {blur: true};


What's the thinking behind having one placeholderConfig vs. separate inputs? I typically see the latter in Angular apps since specifying objects with multiple properties in the config can get clunky

@kara @AndrewKushnir and I had some discussions about this while designing the placeholder feature, and decided that it's quite likely that we'll have additional configuration parameters in the future, assuming we add additional functionality to placeholders. So just starting with a config object seemed wise at it will allow us to keep things backwards compatible.

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

jelbourn · 2024-01-05T00:45:43Z

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

+        } else if (typeof placeholderInput === 'string' && placeholderInput.startsWith('data:')) {
+          return `url(${placeholderInput})`;
+        }
+        return null;


Would it make sense to add a developer-mode-only check that throws an error if typeof placeholderInput === 'string' and it doesn't start with "data:"?

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

adev/src/content/guide/image-optimization.md

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

kara

Two tiny nits, otherwise LGTM

kara · 2024-01-16T23:59:40Z

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

@@ -337,6 +366,9 @@ export class NgOptimizedImage implements OnInit, OnChanges, OnDestroy {
      if (!this.ngSrcset) {
        assertNoComplexSizes(this);
      }
+      assertNoPlaceholderConfigWithoutPlaceholder(this);


@atcastle Friendly ping

kara · 2024-01-18T22:24:08Z

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

+ * attribute.
+ */
+export const DATA_URL_WARN_LIMIT = 2000;
+export const DATA_URL_ERROR_LIMIT = 5000;


+1 sharing a few example sizes would help give context

AndrewKushnir

Reviewed-for: public-api

AndrewKushnir

@atcastle Looking at the public API again, I think we should rename PlaceholderConfig -> ImagePlaceholderConfig to be more specific and avoid potential confusion with @placeholder feature of @defer blocks.

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

AndrewKushnir

Reviewed-for: public-api

kara

LGTM

Add a automatic placeholder implementation supporting loader-based and data URL placeholders

AndrewKushnir

Reviewed-for: public-api

AndrewKushnir · 2024-01-26T22:22:50Z

Presubmit.

AndrewKushnir · 2024-01-27T00:54:05Z

Caretaker note (cc @jessicajaniuk): this PR was reviewed, but 1 public-api approval is missing. Could you please review it as well when you get a chance? If everything looks good from the public-api perspective, this PR would be ready for merge.

(the presubmit is "green" as well, failing targets are unrelated/flaky)

jessicajaniuk

reviewed-for: public-api, angular-dev

github-actions · 2024-01-29T15:56:10Z

Deployed adev-preview for f10e916 to: https://ng-dev-previews-fw--pr-angular-angular-53783-adev-prev-tr959tfk.web.app

Note: As new commits are pushed to this pull request, this link is updated after the preview is rebuilt.

jessicajaniuk · 2024-01-29T16:00:42Z

This PR was merged into the repository by commit f5c520b.

Add a automatic placeholder implementation supporting loader-based and data URL placeholders PR Close angular#53783

bjornharvold · 2024-02-15T02:31:52Z

Great feature! Is this feature documented somewhere here?

https://angular.dev/guide/image-optimization/

AndrewKushnir · 2024-02-15T20:25:36Z

@bjornharvold you can find more info about this feature at: https://angular.dev/guide/image-optimization#using-placeholders (note: the content was not there until angular.dev deploy today).

bjornharvold · 2024-02-16T03:05:31Z

Cheers Andrew!

angular-automatic-lock-bot · 2024-03-18T00:09:14Z

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 review from AndrewKushnir and jelbourn January 3, 2024 20:21

angular-robot bot added the detected: feature label Jan 3, 2024

AndrewKushnir reviewed Jan 3, 2024

View reviewed changes

AndrewKushnir added action: review target: minor labels Jan 3, 2024

kara reviewed Jan 4, 2024

View reviewed changes

jelbourn reviewed Jan 5, 2024

View reviewed changes

atcastle force-pushed the img-placeholder branch 5 times, most recently from d467c00 to b38c161 Compare January 11, 2024 21:47

dylhunn added area: common common: image directive labels Jan 17, 2024

ngbot bot modified the milestone: Backlog Jan 17, 2024

atcastle force-pushed the img-placeholder branch from b38c161 to 2dff8a3 Compare January 17, 2024 17:47

AndrewKushnir reviewed Jan 17, 2024

View reviewed changes

atcastle force-pushed the img-placeholder branch 2 times, most recently from 7b7690e to b5fdfca Compare January 17, 2024 21:01

kara reviewed Jan 18, 2024

View reviewed changes

AndrewKushnir approved these changes Jan 19, 2024

View reviewed changes

AndrewKushnir reviewed Jan 19, 2024

View reviewed changes

packages/common/src/directives/ng_optimized_image/ng_optimized_image.ts Outdated Show resolved Hide resolved

atcastle force-pushed the img-placeholder branch from b5fdfca to a147770 Compare January 22, 2024 22:26

AndrewKushnir approved these changes Jan 23, 2024

View reviewed changes

atcastle force-pushed the img-placeholder branch from a147770 to 80bf9f5 Compare January 23, 2024 20:52

angular-robot bot requested a review from AndrewKushnir January 23, 2024 20:52

atcastle force-pushed the img-placeholder branch from 80bf9f5 to 9ec94a6 Compare January 23, 2024 20:54

kara approved these changes Jan 23, 2024

View reviewed changes

atcastle force-pushed the img-placeholder branch 2 times, most recently from 8b54803 to ee34a28 Compare January 24, 2024 20:03

feat(common): add placeholder to NgOptimizedImage

Loading
Loading status checks…

f10e916

Add a automatic placeholder implementation supporting loader-based and data URL placeholders

atcastle force-pushed the img-placeholder branch from ee34a28 to f10e916 Compare January 25, 2024 21:56

AndrewKushnir approved these changes Jan 26, 2024

View reviewed changes

AndrewKushnir added action: presubmit action: merge merge: caretaker note and removed action: review action: presubmit labels Jan 26, 2024

jessicajaniuk added adev: preview area: adev labels Jan 29, 2024

jessicajaniuk approved these changes Jan 29, 2024

View reviewed changes

jessicajaniuk closed this in f5c520b Jan 29, 2024

nikvarma pushed a commit to nikvarma/angular that referenced this pull request Jan 31, 2024

feat(common): add placeholder to NgOptimizedImage (angular#53783)

7d59267

Add a automatic placeholder implementation supporting loader-based and data URL placeholders PR Close angular#53783

angular-automatic-lock-bot bot locked and limited conversation to collaborators Mar 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(common): add placeholder to NgOptimizedImage #53783

feat(common): add placeholder to NgOptimizedImage #53783

atcastle commented Jan 3, 2024

AndrewKushnir left a comment

kara left a comment

kara Jan 4, 2024

jelbourn Jan 5, 2024

atcastle Jan 17, 2024

kara Jan 18, 2024

jelbourn Jan 5, 2024

atcastle Jan 9, 2024

jelbourn Jan 5, 2024

kara left a comment

kara Jan 16, 2024

kara Jan 18, 2024

AndrewKushnir left a comment

AndrewKushnir left a comment •

edited

Loading

AndrewKushnir left a comment

kara left a comment

AndrewKushnir left a comment

AndrewKushnir commented Jan 26, 2024

AndrewKushnir commented Jan 27, 2024

jessicajaniuk left a comment

github-actions bot commented Jan 29, 2024

jessicajaniuk commented Jan 29, 2024

bjornharvold commented Feb 15, 2024

AndrewKushnir commented Feb 15, 2024

bjornharvold commented Feb 16, 2024

angular-automatic-lock-bot bot commented Mar 18, 2024

feat(common): add placeholder to NgOptimizedImage #53783

feat(common): add placeholder to NgOptimizedImage #53783

Conversation

atcastle commented Jan 3, 2024

AndrewKushnir left a comment

Choose a reason for hiding this comment

kara left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kara left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewKushnir left a comment

Choose a reason for hiding this comment

AndrewKushnir left a comment • edited Loading

Choose a reason for hiding this comment

AndrewKushnir left a comment

Choose a reason for hiding this comment

kara left a comment

Choose a reason for hiding this comment

AndrewKushnir left a comment

Choose a reason for hiding this comment

AndrewKushnir commented Jan 26, 2024

AndrewKushnir commented Jan 27, 2024

jessicajaniuk left a comment

Choose a reason for hiding this comment

github-actions bot commented Jan 29, 2024

jessicajaniuk commented Jan 29, 2024

bjornharvold commented Feb 15, 2024

AndrewKushnir commented Feb 15, 2024

bjornharvold commented Feb 16, 2024

angular-automatic-lock-bot bot commented Mar 18, 2024

AndrewKushnir left a comment •

edited

Loading