Skip to content

feat(common): Allow ngSrc to be changed post-init #50683

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.

Sign up for GitHub

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

Jump to bottom
Closed
atcastle wants to merge 1 commit into from
Closed

feat(common): Allow ngSrc to be changed post-init #50683

atcastle wants to merge 1 commit into from

Conversation

@atcastle
Copy link
Contributor

@atcastle atcastle commented Jun 12, 2023

This PR modifies the NgOptimizedImage behavior when ngSrc is modified after component initialization. Previously this would throw an error, but we've received feedback (#47813) that this is impeding people's ability to use the directive for some use cases. This PR starts with just allowing ngSrc to be modified, but could be expanded to allow modifying other inputs as well.

I'm concerned that modifying certain inputs, such as height, width and fill would be very likely to cause layout shift, which NgOptimizedImage is specifically designed to prevent.

I reviewed the docs and error messages related to this change in functionality, and I don't believe any changes are required.

CC: @AndrewKushnir @kara

@pullapprove pullapprove bot requested a review from jessicajaniuk June 12, 2023 16:56
@angular-robot angular-robot bot added the detected: feature PR contains a feature commit label Jun 12, 2023
@jessicajaniuk jessicajaniuk requested review from AndrewKushnir and removed request for jessicajaniuk June 12, 2023 16:57
@pkozlowski-opensource pkozlowski-opensource added action: review The PR is still awaiting reviews from at least one requested reviewer area: common Issues related to APIs in the @angular/common package common: image directive labels Jun 12, 2023
@ngbot ngbot bot modified the milestone: Backlog Jun 12, 2023
@AndrewKushnir
Copy link
Contributor

AndrewKushnir commented Jun 12, 2023

Just a quick comment: the code around ngSrc input will change a bit in #50580, so we'd need to move the code from the getter to a different location (ngOnInit?).

kara
kara reviewed Jun 12, 2023
View reviewed changes
@atcastle atcastle force-pushed the allow-changing-ngsrc branch from 58a0ddb to 855761e Compare June 12, 2023 23:44
@atcastle atcastle force-pushed the allow-changing-ngsrc branch 3 times, most recently from a445197 to 9638b5a Compare June 15, 2023 22:51
AndrewKushnir
AndrewKushnir reviewed Jun 19, 2023
View reviewed changes
@atcastle atcastle force-pushed the allow-changing-ngsrc branch 6 times, most recently from c136729 to 93ef797 Compare June 22, 2023 19:54
AndrewKushnir
AndrewKushnir approved these changes Jun 23, 2023
View reviewed changes
Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@atcastle looks great, just a couple minor nits.

jessicajaniuk
jessicajaniuk approved these changes Jun 26, 2023
View reviewed changes
Copy link
Contributor

@jessicajaniuk jessicajaniuk left a 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

@pullapprove pullapprove bot requested a review from alxhub June 26, 2023 15:32
AndrewKushnir
AndrewKushnir approved these changes Jun 26, 2023
View reviewed changes
Copy link
Contributor

@AndrewKushnir AndrewKushnir left a 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

@AndrewKushnir AndrewKushnir added the target: minor This PR is targeted for the next minor release label Jun 26, 2023
@atcastle
feat(common): Allow ngSrc to be changed post-init
1ef004f 
Remove thrown error when ngSrc is modified after an NgOptimizedImage image is initialized
@atcastle atcastle force-pushed the allow-changing-ngsrc branch from 93ef797 to 1ef004f Compare June 26, 2023 21:12
@AndrewKushnir AndrewKushnir added action: presubmit The PR is in need of a google3 presubmit and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Jun 27, 2023
@AndrewKushnir
Copy link
Contributor

AndrewKushnir commented Jun 27, 2023

Presubmit.

@AndrewKushnir AndrewKushnir added action: merge The PR is ready for merge by the caretaker and removed action: presubmit The PR is in need of a google3 presubmit labels Jul 3, 2023
@ngbot
Copy link

ngbot bot commented Jul 3, 2023

I see that you just added the action: merge label, but the following checks are still failing:
    failure status "google-internal-tests" is failing

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken main, please try rebasing to main and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

@AndrewKushnir AndrewKushnir added the merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note label Jul 3, 2023
@AndrewKushnir
Copy link
Contributor

AndrewKushnir commented Jul 3, 2023 •
edited
Loading

Caretaker note: the presubmit is "green", this PR is ready for merge.

@dylhunn
Copy link
Contributor

dylhunn commented Jul 11, 2023

This PR was merged into the repository by commit 1837efb.

@dylhunn dylhunn closed this in 1837efb Jul 11, 2023
@AndrewKushnir AndrewKushnir mentioned this pull request Jul 11, 2023
Closed
sunilbaba pushed a commit to sunilbaba/angular that referenced this pull request Jul 26, 2023
@atcastle @sunilbaba
feat(common): Allow ngSrc to be changed post-init (angular#50683)
a345642 
Remove thrown error when ngSrc is modified after an NgOptimizedImage image is initialized

PR Close angular#50683
@angular-automatic-lock-bot
Copy link

angular-automatic-lock-bot bot commented Aug 11, 2023

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
ChellappanRajan pushed a commit to ChellappanRajan/angular that referenced this pull request Jan 23, 2024
@atcastle @ChellappanRajan
feat(common): Allow ngSrc to be changed post-init (angular#50683)
e3ca483 
Remove thrown error when ngSrc is modified after an NgOptimizedImage image is initialized

PR Close angular#50683
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@AndrewKushnir AndrewKushnir AndrewKushnir approved these changes

+2 more reviewers

@kara kara kara left review comments

@jessicajaniuk jessicajaniuk jessicajaniuk approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

action: merge The PR is ready for merge by the caretaker area: common Issues related to APIs in the @angular/common package common: image directive detected: feature PR contains a feature commit merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note target: minor This PR is targeted for the next minor release

Projects

None yet

Milestone

Backlog

Development

Successfully merging this pull request may close these issues.

6 participants

@atcastle @AndrewKushnir @dylhunn @kara @jessicajaniuk @pkozlowski-opensource