fix(animations): implement getPosition in browser animation builder #28264

literalpie · 2019-01-20T16:04:36Z

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?

calling player.getPosition always returns 0
Issue Number: #23785 #18891

What is the new behavior?

calling player.getPosition will return the position of the animation that is complete, as a percent (decimal) value (this is to match the setPosition functionality although it does not match the docs).

If player.getPosition is called on a player with a group of animations, the returned value will be the position of the longest animation (including delays)

Does this PR introduce a breaking change?

Yes
No

Other information

As mentioned above, the functionality does not match the documentation (which says get/set position is measured in milliseconds). I opted to have the functionality match setPosition for consistency.

I considered changing the documentation, but unfortunately, it is a interface implemented by CssKeyframesPlayer, which does match the documentation. Let me know if I should either include the documentation change or the functionality.

I did not include tests because AnimationGroupPlayer does not have any tests.

literalpie · 2019-01-20T16:33:12Z

packages/platform-browser/animations/src/animation_builder.ts

@@ -89,7 +89,7 @@ export class RendererAnimationPlayer implements AnimationPlayer {

  setPosition(p: number): void { this._command('setPosition', p); }

-  getPosition(): number { return 0; }
+  getPosition(): number { return this._renderer.engine.players[+this.id].getPosition(); }


I'm not sure how reliable it is to use the id as the player index. Let me know if there's a better way.

rock-bolton-hyland

this is a quality a PR

mhevery · 2020-11-20T23:43:26Z

Could we have a tests and have it rebased on latest head?

literalpie · 2020-11-22T23:11:07Z

It looks like you have already rebased and there is not an existing test suite for this class. Would you like me to make a new one in a new animations/test/animation_group_player_spec.ts file?

mhevery · 2020-11-23T22:18:46Z

It looks like you have already rebased and there is not an existing test suite for this class. Would you like me to make a new one in a new animations/test/animation_group_player_spec.ts file?

Yes, please, we need a test.

mhevery · 2020-11-23T22:21:06Z

presubmit Looks good.

JoostK · 2020-12-02T22:20:41Z

@literalpie ICYMI I left an additional comment in the already resolved conversation: #28264 (comment)

packages/animations/test/animation_group_player_spec.ts

packages/animations/src/players/animation_group_player.ts

packages/platform-browser/animations/src/animation_builder.ts

JoostK

Thanks for making the changes. I don't see any issues with this in its current state so happy to approve.

mhevery

reviewed-for: global-approvers

mhevery · 2020-12-04T20:05:02Z

presubmit deflake

ngbot · 2020-12-05T00:50:45Z

I see that you just added the action: merge label, but the following checks are still failing:
conflicts with base branch "master"
2 pending code reviews

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 master, please try rebasing to master 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.

mhevery · 2020-12-05T00:58:43Z

PR accidently closed. :-( and now unable to re-open. Moved content to #39983

angular-automatic-lock-bot · 2021-01-05T16:57:15Z

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

literalpie requested review from a team as code owners January 20, 2019 16:04

googlebot added the cla: yes label Jan 20, 2019

literalpie force-pushed the browser-animation-get-position branch 2 times, most recently from 153ae06 to 3c3600e Compare January 20, 2019 16:16

literalpie closed this Jan 20, 2019

literalpie force-pushed the browser-animation-get-position branch from 3c3600e to 73dcd72 Compare January 20, 2019 16:20

literalpie reopened this Jan 20, 2019

literalpie force-pushed the browser-animation-get-position branch from 5748528 to 93b7b76 Compare January 20, 2019 16:28

literalpie commented Jan 20, 2019

View reviewed changes

literalpie force-pushed the browser-animation-get-position branch from 93b7b76 to 5bcee93 Compare January 20, 2019 16:34

jasonaden added the area: animations label Jan 23, 2019

ngbot bot added this to the needsTriage milestone Jan 23, 2019

JoJk0 approved these changes Apr 17, 2020

View reviewed changes

pullapprove bot requested review from alxhub and matsko April 17, 2020 14:23

rock-bolton-hyland reviewed Apr 17, 2020

View reviewed changes

rock-bolton-hyland approved these changes Apr 17, 2020

View reviewed changes

mhevery self-assigned this Nov 20, 2020

pullapprove bot requested review from jelbourn and removed request for matsko November 20, 2020 23:43

mhevery force-pushed the browser-animation-get-position branch 3 times, most recently from 026ea42 to 8afd132 Compare November 21, 2020 00:06

literalpie force-pushed the browser-animation-get-position branch 2 times, most recently from ef4ff3b to 7ab987d Compare November 24, 2020 02:03

literalpie force-pushed the browser-animation-get-position branch 3 times, most recently from d6e5813 to 2c00466 Compare December 2, 2020 22:17

JoostK reviewed Dec 2, 2020

View reviewed changes

literalpie force-pushed the browser-animation-get-position branch from e42348b to e6a4f31 Compare December 3, 2020 01:56

mhevery added this to Needs review in mhevery-review-queue Dec 4, 2020

mhevery moved this from Needs review to In Review in mhevery-review-queue Dec 4, 2020

mhevery force-pushed the browser-animation-get-position branch from e6a4f31 to 790d8bf Compare December 4, 2020 01:12

mhevery moved this from In Review to Presubmit in mhevery-review-queue Dec 4, 2020

mhevery-review-queue automation moved this from Presubmit to Waiting for author Dec 4, 2020

JoostK approved these changes Dec 4, 2020

View reviewed changes

mhevery approved these changes Dec 4, 2020

View reviewed changes

mhevery moved this from Waiting for author to Presubmit in mhevery-review-queue Dec 4, 2020

mhevery added the target: patch This PR is targeted for the next patch release label Dec 5, 2020

ngbot bot modified the milestones: needsTriage, Backlog Dec 5, 2020

mhevery added the action: merge The PR is ready for merge by the caretaker label Dec 5, 2020

mhevery closed this Dec 5, 2020

mhevery force-pushed the browser-animation-get-position branch from 790d8bf to 7954c8d Compare December 5, 2020 00:53

mhevery-review-queue automation moved this from Presubmit to Done Dec 5, 2020

mhevery mentioned this pull request Dec 5, 2020

fix(animations): implement getPosition in browser animation builder #39983

Closed

14 tasks

angular-automatic-lock-bot bot locked and limited conversation to collaborators Jan 5, 2021

pullapprove bot removed the area: animations label Jan 5, 2021

ngbot bot removed this from the Backlog milestone Jan 5, 2021

literalpie deleted the browser-animation-get-position branch January 5, 2021 17:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(animations): implement getPosition in browser animation builder #28264

fix(animations): implement getPosition in browser animation builder #28264

literalpie commented Jan 20, 2019 •

edited

literalpie Jan 20, 2019

rock-bolton-hyland left a comment •

edited

mhevery commented Nov 20, 2020

literalpie commented Nov 22, 2020

mhevery commented Nov 23, 2020

mhevery commented Nov 23, 2020 •

edited

JoostK commented Dec 2, 2020

JoostK left a comment

mhevery left a comment

mhevery commented Dec 4, 2020 •

edited

ngbot bot commented Dec 5, 2020

mhevery commented Dec 5, 2020

angular-automatic-lock-bot bot commented Jan 5, 2021

fix(animations): implement getPosition in browser animation builder #28264

fix(animations): implement getPosition in browser animation builder #28264

Conversation

literalpie commented Jan 20, 2019 • edited

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

literalpie Jan 20, 2019

Choose a reason for hiding this comment

rock-bolton-hyland left a comment • edited

Choose a reason for hiding this comment

mhevery commented Nov 20, 2020

literalpie commented Nov 22, 2020

mhevery commented Nov 23, 2020

mhevery commented Nov 23, 2020 • edited

JoostK commented Dec 2, 2020

JoostK left a comment

Choose a reason for hiding this comment

mhevery left a comment

Choose a reason for hiding this comment

mhevery commented Dec 4, 2020 • edited

ngbot bot commented Dec 5, 2020

mhevery commented Dec 5, 2020

angular-automatic-lock-bot bot commented Jan 5, 2021

literalpie commented Jan 20, 2019 •

edited

rock-bolton-hyland left a comment •

edited

mhevery commented Nov 23, 2020 •

edited

mhevery commented Dec 4, 2020 •

edited