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.

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

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

atcastle
Copy link
Contributor

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

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?).

@atcastle atcastle force-pushed the allow-changing-ngsrc branch 3 times, most recently from a445197 to 9638b5a Compare June 15, 2023 22:51
@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.

packages/common/src/directives/ng_optimized_image/lcp_image_observer.ts Outdated Show resolved Hide resolved
packages/common/src/directives/ng_optimized_image/ng_optimized_image.ts Outdated Show resolved Hide resolved
packages/common/src/directives/ng_optimized_image/ng_optimized_image.ts Outdated Show resolved Hide resolved
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
@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

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

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

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

PR Close angular#50683
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

@kara kara kara left review comments

@AndrewKushnir AndrewKushnir AndrewKushnir approved these changes

@jessicajaniuk jessicajaniuk jessicajaniuk approved these changes

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.

None yet
6 participants
@atcastle @AndrewKushnir @dylhunn @kara @jessicajaniuk @pkozlowski-opensource