fix(animations): Trigger leave animation when ViewContainerRef is injected #48705

JeanMeche · 2023-01-11T21:32:38Z

Injecting ViewContainerRef into a component makes it effectively a container. The leave animation was triggered on container

fixes #48667

PR Type

What kind of change does this PR introduce?

Bugfix

Does this PR introduce a breaking change?

No

JeanMeche · 2023-01-11T21:48:44Z

I will remove the now unused isHostElement argument if the tests are successful.

jessicajaniuk · 2023-01-24T16:27:41Z

This will very likely need a TGP to verify it doesn't cause any breaking behaviors.

jessicajaniuk · 2023-02-07T03:15:51Z

TGP

jessicajaniuk · 2023-02-07T17:23:02Z

The TGP looks green. So I'll take a look at this code today.

jessicajaniuk

The code itself looks ok, but it's unclear to me what case this is fixing. Can you add a specific test to cover this so we have clarity on what's broken, what's being fixed, and why this change is needed?

JeanMeche · 2023-02-08T18:30:22Z

I wanted to add a test to animation_integration_spec.ts I didn't find a way to run it. Any idea ? 😊

jessicajaniuk · 2023-02-08T18:58:09Z

@JeanMeche those tests are part of packages/core/test. So you can isolate and run them with yarn bazel test packages/core/test. The trick is that line 22 of animations_integration_spec.ts has if (isNode) return;, which skips them when running locally. So I typically just comment that out and focus the test I want.

JeanMeche · 2023-02-08T19:39:42Z

Thanks for the heads-up Jessica, you pointed me into the right direction, I needed to run the test case in karma with ``yarn test packages/core/test:test_web`

The integration test I added was broken before the fix but passes fine with the fix.

jessicajaniuk

Thanks for adding the test.

reviewed-for: fw-core, fw-animations, public-api

pkozlowski-opensource · 2023-05-16T10:07:40Z

I will remove the now unused isHostElement argument if the tests are successful.

@JeanMeche so it seems like this PR has 2 changes:

the actual fix in packages/animations/browser/src/render/transition_animation_engine.ts
public API change of the Renderer2

While we can land a fix any time, changing API surface would mean going through the standard depreciation policy and the actual removal can happen only in a major version.

It seems to me like we can't just remove an argument of the Renderer2 since the existing code could be still calling renderer2 with the removed argument, right?

If the above makes sense we should split this PR into a fix and a cleanup. WDYT?

…ected Injecting `ViewContainerRef` into a component makes it effectively a container. The leave animation wasn't triggered on containers before this fix. fixes angular#48667

JeanMeche · 2023-05-16T10:44:16Z

Thank you Pawel, you're right. I was still a beginner when I wrote that fix 😊

We'll do the deprecation in another PR.

pkozlowski-opensource · 2023-05-16T10:56:09Z

Thnx @JeanMeche !

AndrewKushnir · 2023-05-16T16:31:17Z

@jessicajaniuk since the last TGP was a while ago, could you please help run a new one (just to make sure there are no regressed targets in g3)? Thank you.

jessicajaniuk · 2023-05-24T13:43:31Z

New TGP

ngbot · 2023-05-25T14:31:42Z

I see that you just added the action: merge label, but the following checks are still failing:
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.

jessicajaniuk · 2023-05-25T14:32:04Z

TGP is green still

dylhunn · 2023-05-25T18:40:39Z

This PR was merged into the repository by commit bada919.

…ected (#48705) Injecting `ViewContainerRef` into a component makes it effectively a container. The leave animation wasn't triggered on containers before this fix. fixes #48667 PR Close #48705

This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [@angular/animations](https://github.com/angular/angular) | dependencies | patch | [`16.0.2` -> `16.0.4`](https://renovatebot.com/diffs/npm/@angular%2fanimations/16.0.2/16.0.4) | | [@angular/common](https://github.com/angular/angular) | dependencies | patch | [`16.0.2` -> `16.0.4`](https://renovatebot.com/diffs/npm/@angular%2fcommon/16.0.2/16.0.4) | | [@angular/compiler](https://github.com/angular/angular) | dependencies | patch | [`16.0.2` -> `16.0.4`](https://renovatebot.com/diffs/npm/@angular%2fcompiler/16.0.2/16.0.4) | | [@angular/compiler-cli](https://github.com/angular/angular/tree/main/packages/compiler-cli) ([source](https://github.com/angular/angular)) | devDependencies | patch | [`16.0.2` -> `16.0.4`](https://renovatebot.com/diffs/npm/@angular%2fcompiler-cli/16.0.2/16.0.4) | | [@angular/core](https://github.com/angular/angular) | dependencies | patch | [`16.0.2` -> `16.0.4`](https://renovatebot.com/diffs/npm/@angular%2fcore/16.0.2/16.0.4) | | [@angular/forms](https://github.com/angular/angular) | dependencies | patch | [`16.0.2` -> `16.0.4`](https://renovatebot.com/diffs/npm/@angular%2fforms/16.0.2/16.0.4) | | [@angular/platform-browser](https://github.com/angular/angular) | dependencies | patch | [`16.0.2` -> `16.0.4`](https://renovatebot.com/diffs/npm/@angular%2fplatform-browser/16.0.2/16.0.4) | | [@angular/platform-browser-dynamic](https://github.com/angular/angular) | dependencies | patch | [`16.0.2` -> `16.0.4`](https://renovatebot.com/diffs/npm/@angular%2fplatform-browser-dynamic/16.0.2/16.0.4) | --- ### Release Notes <details> <summary>angular/angular</summary> ### [`v16.0.4`](https://github.com/angular/angular/blob/HEAD/CHANGELOG.md#1604-2023-06-01) [Compare Source](angular/angular@16.0.3...16.0.4) ##### animations | Commit | Type | Description | | -- | -- | -- | | [df65c4fc8f](angular/angular@df65c4f) | fix | Trigger leave animation when ViewContainerRef is injected ([#48705](angular/angular#48705)) | ##### common | Commit | Type | Description | | -- | -- | -- | | [7e1bc513de](angular/angular@7e1bc51) | fix | untrack subscription and unsubscription in async pipe ([#50522](angular/angular#50522)) | ##### core | Commit | Type | Description | | -- | -- | -- | | [9970b29ace](angular/angular@9970b29) | fix | update `ApplicationRef.isStable` to account for rendering pending tasks ([#50425](angular/angular#50425)) |  ### [`v16.0.3`](https://github.com/angular/angular/blob/HEAD/CHANGELOG.md#1603-2023-05-24) [Compare Source](angular/angular@16.0.2...16.0.3) ##### core | Commit | Type | Description | | -- | -- | -- | | [c11041e372](angular/angular@c11041e) | fix | adds missing symbols for animation standalone bundling test ([#50434](angular/angular#50434)) | | [98e8fdf40e](angular/angular@98e8fdf) | fix | fix `Self` flag inside embedded views with custom injectors ([#50270](angular/angular#50270)) | | [199ff4fe7f](angular/angular@199ff4f) | fix | host directives incorrectly validating aliased bindings ([#50364](angular/angular#50364)) | ##### http | Commit | Type | Description | | -- | -- | -- | | [080bbd2137](angular/angular@080bbd2) | fix | create macrotask during request handling instead of load start ([#50406](angular/angular#50406)) |  </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about these updates again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).  Co-authored-by: cabr2-bot <cabr2.help@gmail.com> Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1915 Reviewed-by: Epsilon_02 <epsilon_02@noreply.codeberg.org> Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org> Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>

angular-automatic-lock-bot · 2023-06-25T00:10:44Z

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.}

…ected (angular#48705) Injecting `ViewContainerRef` into a component makes it effectively a container. The leave animation wasn't triggered on containers before this fix. fixes angular#48667 PR Close angular#48705

jessicajaniuk added the area: animations label Jan 19, 2023

ngbot bot added this to the Backlog milestone Jan 19, 2023

JeanMeche force-pushed the fix/animation-leave branch 4 times, most recently from a8f184a to 4db884d Compare January 23, 2023 16:07

JeanMeche marked this pull request as ready for review January 23, 2023 20:29

pullapprove bot requested a review from dylhunn January 23, 2023 20:29

dylhunn requested a review from jessicajaniuk January 24, 2023 10:14

jessicajaniuk added action: review The PR is still awaiting reviews from at least one requested reviewer action: global presubmit The PR is in need of a google3 global presubmit target: patch This PR is targeted for the next patch release labels Jan 24, 2023

jessicajaniuk added the flag: breaking change label Feb 7, 2023

jessicajaniuk suggested changes Feb 7, 2023

View reviewed changes

JeanMeche force-pushed the fix/animation-leave branch from 4db884d to cddaf7b Compare February 8, 2023 19:39

JeanMeche force-pushed the fix/animation-leave branch from cddaf7b to b052d8a Compare February 8, 2023 20:04

JeanMeche requested a review from jessicajaniuk May 1, 2023 14:09

jessicajaniuk approved these changes May 1, 2023

View reviewed changes

pullapprove bot requested review from AndrewKushnir and jessicajaniuk May 1, 2023 14:13

jessicajaniuk removed their request for review May 1, 2023 14:14

jessicajaniuk removed the action: global presubmit The PR is in need of a google3 global presubmit label May 1, 2023

pullapprove bot requested a review from atscott May 1, 2023 14:14

JeanMeche force-pushed the fix/animation-leave branch from b052d8a to c4e8fbd Compare May 16, 2023 10:42

fix(animations): Trigger leave animation when ViewContainerRef is inj…

515a9ca

…ected Injecting `ViewContainerRef` into a component makes it effectively a container. The leave animation wasn't triggered on containers before this fix. fixes angular#48667

JeanMeche force-pushed the fix/animation-leave branch from c4e8fbd to 515a9ca Compare May 16, 2023 10:42

pkozlowski-opensource removed request for dylhunn, AndrewKushnir and pkozlowski-opensource May 16, 2023 10:56

AndrewKushnir added action: global presubmit The PR is in need of a google3 global presubmit and removed action: merge The PR is ready for merge by the caretaker labels May 16, 2023

jessicajaniuk added action: merge The PR is ready for merge by the caretaker and removed action: global presubmit The PR is in need of a google3 global presubmit labels May 25, 2023

dylhunn closed this in bada919 May 25, 2023

angular-automatic-lock-bot bot locked and limited conversation to collaborators Jun 25, 2023

JeanMeche deleted the fix/animation-leave branch July 12, 2023 08:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(animations): Trigger leave animation when ViewContainerRef is injected #48705

fix(animations): Trigger leave animation when ViewContainerRef is injected #48705

JeanMeche commented Jan 11, 2023

JeanMeche commented Jan 11, 2023 •

edited

jessicajaniuk commented Jan 24, 2023

jessicajaniuk commented Feb 7, 2023

jessicajaniuk commented Feb 7, 2023

jessicajaniuk left a comment

JeanMeche commented Feb 8, 2023

jessicajaniuk commented Feb 8, 2023

JeanMeche commented Feb 8, 2023

jessicajaniuk left a comment •

edited

pkozlowski-opensource commented May 16, 2023

JeanMeche commented May 16, 2023

pkozlowski-opensource commented May 16, 2023

AndrewKushnir commented May 16, 2023

jessicajaniuk commented May 24, 2023

ngbot bot commented May 25, 2023

jessicajaniuk commented May 25, 2023

dylhunn commented May 25, 2023

angular-automatic-lock-bot bot commented Jun 25, 2023

fix(animations): Trigger leave animation when ViewContainerRef is injected #48705

fix(animations): Trigger leave animation when ViewContainerRef is injected #48705

Conversation

JeanMeche commented Jan 11, 2023

PR Type

Does this PR introduce a breaking change?

JeanMeche commented Jan 11, 2023 • edited

jessicajaniuk commented Jan 24, 2023

jessicajaniuk commented Feb 7, 2023

jessicajaniuk commented Feb 7, 2023

jessicajaniuk left a comment

Choose a reason for hiding this comment

JeanMeche commented Feb 8, 2023

jessicajaniuk commented Feb 8, 2023

JeanMeche commented Feb 8, 2023

jessicajaniuk left a comment • edited

Choose a reason for hiding this comment

pkozlowski-opensource commented May 16, 2023

JeanMeche commented May 16, 2023

pkozlowski-opensource commented May 16, 2023

AndrewKushnir commented May 16, 2023

jessicajaniuk commented May 24, 2023

ngbot bot commented May 25, 2023

jessicajaniuk commented May 25, 2023

dylhunn commented May 25, 2023

angular-automatic-lock-bot bot commented Jun 25, 2023

JeanMeche commented Jan 11, 2023 •

edited

jessicajaniuk left a comment •

edited