Skip to content
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

Allow for the ngSrc (and other inputs to change) when using NgOptimizedDirective #47813

Closed
yharaskrik opened this issue Oct 19, 2022 · 13 comments
Labels
area: common Issues related to APIs in the @angular/common package common: image directive feature: under consideration Feature request for which voting has completed and the request is now under consideration feature Issue that requests a new feature
Milestone

Comments

@yharaskrik
Copy link
Contributor

yharaskrik commented Oct 19, 2022

Which @angular/* package(s) are relevant/related to the feature request?

common

Description

Currently an error is thrown (in dev mode) when one of the inputs changes on an img when when using NgOptimizedImage (the directive)

The directive currently only supports the most basic case of rendered once and only once, but there are many use cases where an image may change (realtime applications for example). Our use case in particular is a website builder that allows the user to edit the page inline (and eventually in realtime). Heres some screenshots from Trellis.org to demonstrate this:

image

image

A user can click to change the media which opens up a photo library using Unsplash and then crops the image, after the server response the ngSrc is updated with the new image (currently no other inputs change as the element is a static size)

Currently did does not work as inputs are not allowed to change.

Our rendered live pages (the ones the users of our users end up seeing after the page is finished being built) use the directive already but it would be nice if our website builder itself could be using it as well.

Proposed solution

Allow for the Inputs to change and check and update either all of the attributes or only the ones that would be affected by that specific attribute(s).

In ngOnChanges instead of throwing an error (we may still want to throw an error for some, like priority) determine what attributes on the img tag changed and need to be updated (or just update them all by calling setHostAttributes() again.

Alternatives considered

Existing solutions to this are a little hacky and annoying to do but are as such:

  1. Create a directive that manually (with some type coercing) force the directive to rerender (THIS IS HACKY DO NOT DO THIS):
    https://angular-team.slack.com/archives/C08M4JKNH/p1666193540352369?thread_ts=1666128103.036069&cid=C08M4JKNH
import { NgOptimizedImage } from '@angular/common';
import type { OnChanges, SimpleChanges } from '@angular/core';
import { Directive, inject, Input } from '@angular/core';

@Directive({
    selector: 'img[imageChange]',
    standalone: true,
})
export class OptimizedImageChangedDirective implements OnChanges {
    private readonly ngOptimizedImage = inject(NgOptimizedImage);

    @Input() rawSrc!: string;

    ngOnChanges(changes: SimpleChanges): void {
        if (changes['rawSrc'] && !changes['rawSrc'].isFirstChange()) {
            (this.ngOptimizedImage as any).setHostAttributes();
        }
    }
}
<img [ngSrc]="media" imageChange ...other attrs>
  1. Wrapping in an <ng-template> and create an embedded view (as per @AndrewKushnir):
    https://angular-team.slack.com/archives/C08M4JKNH/p1666132985022759?thread_ts=1666128103.036069&cid=C08M4JKNH

You may try wrapping an <img> into <ng-template> in this case and create a view dynamically (via ViewContainerRef.createEmbeddedView) - this would re-create the view containing an <img> and you can control when this view should be re-created.

@AndrewKushnir AndrewKushnir added feature Issue that requests a new feature area: common Issues related to APIs in the @angular/common package common: image directive labels Oct 19, 2022
@ngbot ngbot bot modified the milestone: Backlog Oct 19, 2022
@kbrilla
Copy link

kbrilla commented Oct 20, 2022

The same thing would be required if used in cdk virtual scroll

@angular-robot angular-robot bot added the feature: votes required Feature request which is currently still in the voting phase label Oct 20, 2022
@angular-robot
Copy link
Contributor

angular-robot bot commented Oct 20, 2022

This feature request is now candidate for our backlog! In the next phase, the community has 60 days to upvote. If the request receives more than 20 upvotes, we'll move it to our consideration list.

You can find more details about the feature request process in our documentation.

@yharaskrik
Copy link
Contributor Author

The same thing would be required if used in cdk virtual scroll

Hey @kbrilla please upvote the issue if it's something you feel would be good for the framework.

@angular-robot
Copy link
Contributor

angular-robot bot commented Nov 29, 2022

Just a heads up that we kicked off a community voting process for your feature request. There are 20 days until the voting process ends.

Find more details about Angular's feature request process in our documentation.

@angular-robot angular-robot bot added feature: under consideration Feature request for which voting has completed and the request is now under consideration and removed feature: votes required Feature request which is currently still in the voting phase labels Dec 14, 2022
@vytautas-pranskunas-
Copy link

watching

@gizm0bill
Copy link

I don't think this directive is very useful without being dynamic, as requested here

@tomastrajan
Copy link
Contributor

One of the example use cases is to use it for a component which listens to the :id path params change and renders different image based on current item.

This will currently result in error and needs to be fixed by double navigation (eg .. and then back to destroy / re-create the component)

@exequiel09
Copy link

We also encountered this issue and had a workaround to rerender it using: https://www.npmjs.com/package/ngx-rerender

@nicholasconfer
Copy link

Also encountered this issue, particularly in trying to make ngSrcset dynamic. The lack of ngSrc/ngSrcSet being non-bindable makes it difficult to use in a more advance application. Looking forward to this as a future feature, as otherwise ngOptimizedImage looks great.

@marcowuethrich
Copy link

I would also love this feature. At the moment i have to change the most ngSrc back to normal src because of dynamically routing changes without re-creating the components.

@francesco-buscicchio
Copy link

Any update?

@AndrewKushnir
Copy link
Contributor

Quick update: the change that allows ngSrc modification (PR #50683) has landed and will be available in v16.2.0 (see https://angular.io/guide/releases#release-schedule for additional info about the release dates). The behavior of other inputs remain the same to avoid Content Layout Shifts (CLS), see #50683).

I'm closing this ticket for now, please create a new ticket if changing other inputs is blocking you from using NgOptmizedImage.

@angular-automatic-lock-bot
Copy link

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Aug 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: common Issues related to APIs in the @angular/common package common: image directive feature: under consideration Feature request for which voting has completed and the request is now under consideration feature Issue that requests a new feature
Projects
None yet
Development

No branches or pull requests

10 participants