fix(animations): implement getPosition in browser animation builder by literalpie · Pull Request #28264 · angular/angular

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

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.

mhevery · 2020-12-02T21:15:50Z

@literalpie Could we get a test for this so that we can merge this?

literalpie · 2020-12-02T21:18:07Z

@mhevery did you see the tests I added? Sorry, I forgot to ping you when I made that change

JoostK · 2020-12-02T21:30:08Z

packages/animations/src/players/animation_group_player.ts

There's no need to sort the full array just to get the largest total time. Also sort mutates the original array and that may not be what you want here.

Thanks for catching that. I updated the code to use reduce instead.

Thanks, that was quick 👍

Now that I'm looking further into it, it's surprising that the position of the longest player needs to be divided by its total time; indicating that getPosition from the delegated player is using a different convention than the group player. As you mention in the description the documentation appears to be incorrect, as it does indeed seem that the value should be fractional between 0 and 1 and not the number of milliseconds. So I suspect there is an issue in a different player that is causing this inconsistency (maybe it's just the mock player in tests??).

If the devision by longestPlayer.totalTime is to remain (which I'm doubting it should) then you'd also need to account for it being 0, similarly to what is done in setPosition.

I think we may be stuck with the fractional value, since that's what setPosition uses. I'm assuming we want setting and getting to behave the same, and that we don't want to introduce a breaking change.

I'll update this tonight to handle 0 assuming fractional for now, and just let me know if you want me to change it.

Yeah agreed on the fractional, I wasn't suggesting to change that. It's just that I'd expect longestPlayer.getPosition() to also be fractional already, such that the division by its total time seems to be inconsistent.

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

JoostK requested changes Dec 2, 2020

View reviewed changes

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 force-pushed the browser-animation-get-position branch from e6a4f31 to 790d8bf Compare December 4, 2020 01:12

JoostK approved these changes Dec 4, 2020

View reviewed changes

mhevery approved these changes Dec 4, 2020

View reviewed changes

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

Conversation

literalpie commented Jan 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

Uh oh!

literalpie Jan 20, 2019

Choose a reason for hiding this comment

Uh oh!

rock-bolton-hyland left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhevery commented Nov 20, 2020

Uh oh!

literalpie commented Nov 22, 2020

Uh oh!

mhevery commented Nov 23, 2020

Uh oh!

mhevery commented Nov 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhevery commented Dec 2, 2020

Uh oh!

literalpie commented Dec 2, 2020

Uh oh!

JoostK Dec 2, 2020

Choose a reason for hiding this comment

Uh oh!

literalpie Dec 2, 2020

Choose a reason for hiding this comment

Uh oh!

JoostK Dec 2, 2020

Choose a reason for hiding this comment

Uh oh!

literalpie Dec 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JoostK Dec 2, 2020

Choose a reason for hiding this comment

Uh oh!

JoostK commented Dec 2, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JoostK left a comment

Choose a reason for hiding this comment

Uh oh!

mhevery left a comment

Choose a reason for hiding this comment

Uh oh!

mhevery commented Dec 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ngbot bot commented Dec 5, 2020

Uh oh!

mhevery commented Dec 5, 2020

Uh oh!

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

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

literalpie commented Jan 20, 2019 •

edited

Loading

rock-bolton-hyland left a comment •

edited

Loading

mhevery commented Nov 23, 2020 •

edited

Loading

literalpie Dec 2, 2020 •

edited

Loading

mhevery commented Dec 4, 2020 •

edited

Loading