Skip to content

fix(animations): animateChild shouldn't trigger animation on disabled element#37724

Closed
Zuckjet wants to merge 1 commit intoangular:masterfrom
Zuckjet:animations/animate-child
Closed

fix(animations): animateChild shouldn't trigger animation on disabled element#37724
Zuckjet wants to merge 1 commit intoangular:masterfrom
Zuckjet:animations/animate-child

Conversation

@Zuckjet
Copy link
Contributor

@Zuckjet Zuckjet commented Jun 25, 2020 •
edited
Loading

…ations

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

Fixes 35558

@googlebot
Copy link

googlebot commented Jun 25, 2020

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@pullapprove pullapprove bot requested a review from matsko June 25, 2020 03:34
@Zuckjet Zuckjet force-pushed the animations/animate-child branch from 45942e3 to 68ecf2f Compare June 25, 2020 04:24
@googlebot
Copy link

googlebot commented Jun 25, 2020

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@Zuckjet Zuckjet force-pushed the animations/animate-child branch from 68ecf2f to e5e1f92 Compare June 25, 2020 04:38
@Zuckjet Zuckjet force-pushed the animations/animate-child branch from e5e1f92 to fcd2c9f Compare June 25, 2020 05:40
@Zuckjet Zuckjet force-pushed the animations/animate-child branch from fcd2c9f to dfd3af5 Compare June 25, 2020 11:07
@Zuckjet
Copy link
Contributor Author

Zuckjet commented Jun 25, 2020 •
edited
Loading

By the way , the test file has some typo errors before. And I fix it with this PR together.

@Zuckjet Zuckjet changed the title fix(animations): animateChild should not invoke disabled child anim… fix(animations): animateChild shouldn't trigger animation on disabled element Jun 25, 2020
@ngbot ngbot bot added this to the needsTriage milestone Jun 25, 2020
@Zuckjet Zuckjet force-pushed the animations/animate-child branch from dfd3af5 to 28aa7ee Compare June 26, 2020 04:33
@pullapprove pullapprove bot requested a review from mhevery June 26, 2020 04:34
@Zuckjet Zuckjet force-pushed the animations/animate-child branch from 28aa7ee to 01edf66 Compare June 26, 2020 05:14
@Zuckjet Zuckjet force-pushed the animations/animate-child branch from 01edf66 to 93fa7c0 Compare June 26, 2020 05:59
@Zuckjet Zuckjet force-pushed the animations/animate-child branch from 93fa7c0 to 6012442 Compare June 26, 2020 06:45
@Zuckjet Zuckjet force-pushed the animations/animate-child branch from 6012442 to 7dccfd7 Compare June 26, 2020 06:58
@Zuckjet Zuckjet force-pushed the animations/animate-child branch from 7dccfd7 to c92325f Compare June 26, 2020 07:11
@pullapprove pullapprove bot removed the request for review from matsko August 27, 2020 14:35
@Zuckjet
Copy link
Contributor Author

Zuckjet commented Jun 15, 2021 •
edited
Loading

The cr/circleci error seems is not relevant with this PR.
It has passed almost one year since first commit, and I am glad to help If there is something I can do to help this PR go on.

@jessicajaniuk
Copy link
Contributor

jessicajaniuk commented Jun 15, 2021

I've restarted the CI target and am running a google3 presubmit.

crisbeto
crisbeto reviewed Jun 15, 2021
View reviewed changes
packages/animations/browser/src/render/transition_animation_engine.ts Outdated
Copy link
Member

@crisbeto crisbeto Jun 15, 2021

Choose a reason for hiding this comment

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

Overall the changes look good, but this filter with a side effect looks weird since filter predicate functions are supposed to be pure. I'd recommend doing this instead:

const timelines = [];

instruction.timelines.forEach(tl => {
  tl.stretchStartingKeyframe = true;

  if (!this.disabledNodes.has(tl.element)) {
    timelines.push(tl);
  }
});

instruction.timelines = timelines;

Copy link
Contributor Author

@Zuckjet Zuckjet Jun 16, 2021

Choose a reason for hiding this comment

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

Thanks, have updated code

@Zuckjet Zuckjet force-pushed the animations/animate-child branch from 622778c to 4101aca Compare June 16, 2021 03:09
crisbeto
crisbeto approved these changes Jun 16, 2021
View reviewed changes
Copy link
Member

@crisbeto crisbeto left a comment

Choose a reason for hiding this comment

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

LGTM

@Zuckjet
Copy link
Contributor Author

Zuckjet commented Jul 1, 2021

friendly ping @IgorMinar

@jessicajaniuk jessicajaniuk removed the request for review from IgorMinar December 13, 2021 18:58
@jessicajaniuk
Copy link
Contributor

jessicajaniuk commented Dec 13, 2021

@Zuckjet Looks like there's a code conflict here. Can you rebase this? I think we should be able to land it. Once you rebase, I'll re-run a presubmit and we should be green.

@ngbot ngbot bot modified the milestones: needsTriage, Backlog Dec 13, 2021
@jessicajaniuk jessicajaniuk added the risky Identifies a PR that has a level of risk to merging label Dec 13, 2021
@Zuckjet Zuckjet force-pushed the animations/animate-child branch from 4101aca to a270dcf Compare December 14, 2021 02:01
@Zuckjet
fix(animations): should not invoke disabled child animations
d60c34f 
Currently child animation will be triggered by `animateChild()` despite it has been disabled.
These changes add some logic to prevent that unexpected behavior.

PR Close angular#37724
@Zuckjet Zuckjet force-pushed the animations/animate-child branch from a270dcf to d60c34f Compare December 14, 2021 02:20
@Zuckjet
Copy link
Contributor Author

Zuckjet commented Dec 14, 2021

@Zuckjet Looks like there's a code conflict here. Can you rebase this? I think we should be able to land it. Once you rebase, I'll re-run a presubmit and we should be green.

have fixed the conflict.

@jessicajaniuk jessicajaniuk added action: merge The PR is ready for merge by the caretaker and removed state: blocked labels Dec 14, 2021
@jessicajaniuk
Copy link
Contributor

jessicajaniuk commented Dec 14, 2021

Everything looks good. Thanks @Zuckjet!

@alxhub
Copy link
Member

alxhub commented Dec 14, 2021

This PR was merged into the repository by commit bab7ed3.

alxhub pushed a commit that referenced this pull request Dec 14, 2021
@Zuckjet @alxhub
fix(animations): should not invoke disabled child animations (#37724)
d8b6adb 
Currently child animation will be triggered by `animateChild()` despite it has been disabled.
These changes add some logic to prevent that unexpected behavior.

PR Close #37724

PR Close #37724
@alxhub alxhub closed this in bab7ed3 Dec 14, 2021
This was referenced Jan 1, 2022
Closed
Closed
Closed
@angular-automatic-lock-bot
Copy link

angular-automatic-lock-bot bot commented Jan 14, 2022

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 Jan 14, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@crisbeto crisbeto crisbeto approved these changes

+2 more reviewers

@mhevery mhevery mhevery approved these changes

@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: animations cla: yes risky Identifies a PR that has a level of risk to merging target: patch This PR is targeted for the next patch release

Projects

None yet

Milestone

Backlog

Development

Successfully merging this pull request may close these issues.

animateChild invoke disabled child animation in group

8 participants

@Zuckjet @googlebot @mhevery @AndrewKushnir @jessicajaniuk @alxhub @crisbeto @atscott