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

fix(ivy): ensure element removal triggers host removal animations #28162

Closed
wants to merge 3 commits into from

Conversation

matsko
Copy link
Contributor

@matsko matsko commented Jan 15, 2019

Prior to this fix Ivy would not execute any animation triggers
that exist as host bindings on an element if it is removed by
the parent template.

@matsko matsko requested review from a team as code owners January 15, 2019 18:42
@mary-poppins
Copy link

You can preview 5542c5f at https://pr28162-5542c5f.ngbuilds.io/.

@mary-poppins
Copy link

You can preview d678291 at https://pr28162-d678291.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 61da545 at https://pr28162-61da545.ngbuilds.io/.

@mary-poppins
Copy link

You can preview a500f28 at https://pr28162-a500f28.ngbuilds.io/.

@matsko
Copy link
Contributor Author

matsko commented Jan 15, 2019

@matsko matsko added the target: major This PR is targeted for the next major release label Jan 16, 2019
@mary-poppins
Copy link

You can preview f8f4abf at https://pr28162-f8f4abf.ngbuilds.io/.

// it will help the renderer to decide whether anything special before
// removal (like calling the leave animation (if defined) on the element
// before it is removed from the page).
const isHostElement = (tNode.flags & TNodeFlags.isComponent) > 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use the isComponent util here:

Suggested change
const isHostElement = (tNode.flags & TNodeFlags.isComponent) > 0;
nativeRemoveChild(renderer, parent !, node, isComponent(tNode));

@Component({
selector: 'ani-cmp',
template: `
fixmeIvy('unknown').it(
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you look into why your change doesn't fix this test? If there is a different root cause, we should create a Jira ticket and link it here.

@kara kara added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews comp: ivy labels Jan 16, 2019
@ngbot ngbot bot added this to the needsTriage milestone Jan 16, 2019
@mary-poppins
Copy link

You can preview e97daca at https://pr28162-e97daca.ngbuilds.io/.

@matsko
Copy link
Contributor Author

matsko commented Jan 16, 2019

@mary-poppins
Copy link

You can preview 0ed8966 at https://pr28162-0ed8966.ngbuilds.io/.

Copy link
Contributor

@kara kara left a comment

Choose a reason for hiding this comment

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

LGTM

@matsko matsko removed the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Jan 17, 2019
@mary-poppins
Copy link

You can preview 420e869 at https://pr28162-420e869.ngbuilds.io/.

@matsko matsko added the action: merge The PR is ready for merge by the caretaker label Jan 17, 2019
@alxhub alxhub closed this in 5a582a8 Jan 17, 2019
@matsko matsko deleted the issue_FW-943 branch January 17, 2019 19:40
kyliau pushed a commit to kyliau/angular that referenced this pull request Jan 17, 2019
…gular#28162)

Prior to this fix Ivy would not execute any animation triggers
that exist as host bindings on an element if it is removed by
the parent template.

PR Close angular#28162
ngfelixl pushed a commit to ngfelixl/angular that referenced this pull request Jan 28, 2019
…gular#28162)

Prior to this fix Ivy would not execute any animation triggers
that exist as host bindings on an element if it is removed by
the parent template.

PR Close angular#28162
@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 Sep 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes target: major This PR is targeted for the next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants