fix:(animation): Animator does not correctly parse transition durations with multiple values #2381

cburgdorf · 2013-04-11T23:17:35Z

This fixes #2373

Unfortunately I don't really know how to write tests for that. Any idea would be welcome.

cburgdorf · 2013-04-12T08:05:37Z

I rebased against latest master and (hopefully) improved the commit message

petebacondarwin · 2013-04-16T12:25:42Z

petebacondarwin · 2013-04-16T12:26:09Z

@cburgdorf - can you take a look at the list here. Most importantly we need a CLA signature and unit tests.

cburgdorf · 2013-04-16T13:32:53Z

Hey @petebacondarwin, thanks for taking the time to look through it. The CLA is already signed and I also contributed before (only to docs though).

I now changed an existing test since I personally guess that's more appropriate here. If your rather want me to test it seperately I will change that. But the tests would look pretty much identical then.

It's rebased against latest master and squashed into one commit.

petebacondarwin · 2013-04-16T15:43:26Z

Looks good. I'll push it into the CI server for a sanity check.

petebacondarwin · 2013-04-16T15:56:45Z

Fails on Safari 6.0 Mac: http://ci.angularjs.org/job/angular.js-pete/75/console

cburgdorf · 2013-04-16T16:39:15Z

That's a bit weird because I just checked it with Safari 6 and it's picking the highest duration for me. I know absolutely nothing about the testing frameworks Angular uses because I mostly just use qunit. So pardon me, is there any way for me to just run this particular test in safari so that I can hook in with the inspector and attach breakpoints?

petebacondarwin · 2013-04-16T16:44:56Z

If you have node installed then try:

npm install -g grunt-cli

then cd into the project directory

npm install
grunt
grunt test:unit

That should trigger a run with Chrome but you can modify the
karma-*.conf.js files to run with different browsers.

On 16 April 2013 17:39, Christoph Burgdorf [email protected] wrote:

That's a bit weird because I just checked it with Safari 6 and it's
picking the highest duration for me. I know absolutely nothing about the
testing frameworks Angular uses because I mostly just use qunit. So pardon
me, is there any way for me to just run this particular test in safari so
that I can hook in with the inspector and attach breakpoints?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/2381#issuecomment-16455971
.

petebacondarwin · 2013-04-16T16:46:22Z

Actually run grunt autotest then change the particular test to from "it" to
"iit". In the browser, you should be able to hit the "debug" link and then
every time you refresh the page it should rerun that test.

On 16 April 2013 17:44, Peter Bacon Darwin [email protected] wrote:

If you have node installed then try:

npm install -g grunt-cli

then cd into the project directory

npm install
grunt
grunt test:unit

That should trigger a run with Chrome but you can modify the
karma-*.conf.js files to run with different browsers.

On 16 April 2013 17:39, Christoph Burgdorf [email protected]:

That's a bit weird because I just checked it with Safari 6 and it's
picking the highest duration for me. I know absolutely nothing about the
testing frameworks Angular uses because I mostly just use qunit. So pardon
me, is there any way for me to just run this particular test in safari so
that I can hook in with the inspector and attach breakpoints?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/2381#issuecomment-16455971
.

cburgdorf · 2013-04-16T19:17:53Z

That's a neat trick with this iit thingy :)

I fixed the bug with Safari. It was actually a bug within the test as I forgot to vendor prefix one of the css properties. It did work in Chrome as Chrome does not require the vendor prefix anymore.

cburgdorf · 2013-04-17T20:42:48Z

@petebacondarwin Can you check against the CI again (or can I do that, too?). It should be fine actually since the test is working for me on Safari locally. Is there anything left for me to do to get that merged?

matsko · 2013-04-23T18:03:21Z

Once you guys are close to complete, I can merge this with the other two PRs I have for CSS animations and transition delays.

cburgdorf · 2013-04-23T20:01:45Z

I'm would be happy to continue @romiem's work in that other PR (#2396) if that's ok for your @romiem? I would do it in a way that I keep your commit and just amend it so that the commit log shows you as an author. So that we would only have two logical commits but keeping you and me as an author. Guess no one likes to see his work being hidden under a different name ;-)

romiem · 2013-04-23T21:17:29Z

Hey there @cburgdorf - apologies for the late reply. That would be great if you could make that change. I had written the function to calculate the max duration+delay pair but hadn't got around to writing unit tests or the other formal process that goes into committing code into Angular. Here is the method that I had written - please feel free to modify as you wish :)
https://gist.github.com/romiem/5447476
Thanks again for your patience!

romiem · 2013-04-23T21:25:40Z

I should also mention that if you leave the delayString parameter empty, it returns the max duration, which means that we can combine our patches - but I shall await your thoughts on how best to progress this :)

cburgdorf · 2013-04-23T21:49:10Z

Hey @matsko I rebased @romiem 's PR on top of mine and and also added a test to take the following case into account:

property    |    duration    |   delay
height      |     1s         |   2s
left        |     2s         |   1s
opacity     |     1s         |   2s

In this situation the overall time spent is 3s and not 4s.

@romiem I liked your approach! I just adjusted minor stuff (e.g. I removed the map since that would result into looping three times instead of just looping once). I made those minor adjustments under your name so that we can stick to only two commits (one by me, one by you).

@matsko anything left for us to do or is it ready for you to take over?

matsko · 2013-04-24T20:54:29Z

I'm happy to take over, could you provide me with the latest commit so that I can put the final changes together?

cburgdorf · 2013-04-24T21:50:45Z

It's all pushed so it's only those two commits here https://github.com/angular/angular.js/pull/2381/commits The first one provides support for multiple durations (cburgdorf@8a3fc7d) and the second one provides support for multiple delays and also takes the special situation about multiple delays and durations into account (cburgdorf@d0288b1)

So basically, you can pick or rebase from my PR2373 branch.

matsko · 2013-05-01T22:19:30Z

Going to combine this into a larger commit. Plans are to change animation-enter-setup to just be called animation-enter and animation-enter-start to be called animation-enter-active so that CSS Animations work cleanly (since there is no need for two CSS classes). Your delay and multi-property duration code will be merged in together with the CSS Animation values too.

mhevery · 2013-05-01T22:40:34Z

src/ng/animator.js

@@ -267,6 +267,25 @@ var $AnimatorProvider = function() {
              // $window.setTimeout(beginAnimation, 0); this was causing the element not to animate
              // keep at 1 for animation dom rerender
              $window.setTimeout(beginAnimation, 1);
+
+              function savelySplitByComma(str){


savely -> safely???

move to top level

cburgdorf · 2013-05-02T05:59:00Z

@matsko should I override the branch with the corrections from @mhevery then? Or did you already rebase and it's easier for you to adjust yourself? Either is fine for me. When you say you combine it, will you keep our commits then?

cburgdorf · 2013-05-02T08:39:51Z

Ok, I made those changes now and forced push the branch. @matsko I think if you already rebased on top of this, then you probably don't care about this PR update and just apply them again in your already rebased branch. On the other hand, if you haven't already started rebasing on top of this PR, then we might just merge it to master and you rebase on master then?

@mhevery I put safelySplitByComma into the Angular.js file. That's where you wanted me to put it, right? I'm wasn't sure if you have a convention on where exactly to put the generic helper stuff so I put it directly after the reverseParams function. Is that the right way?

I also did the other stuff. I just left the getMaxDuration function where it is since as I noted on your inline comment, this function is safe to assume to only deal with seconds as getComputedStyle already normalizes the units for us.

In case multiple durations are specified like so: transition-duration: 0.5s, 3000ms; transition-property: left, height; The animator will pick the longest duration. Fixes angular#2373

The code also handles situations with multiple different delay and duration values. E.g. in the following example, the correct duration would be 3 seconds: property | duration | delay height | 1s | 2s left | 2s | 1s opacity | 1s | 2s

matsko · 2013-05-06T18:54:25Z

Hey guys. Sorry for being out of touch. There are bunch of fixes for animations that are taking place so I'm merging your commits into a bigger PR.

matsko · 2013-05-09T00:29:42Z

Landed in:

1475787

Thank you guys for the animator and test code.

matsko · 2013-05-17T03:25:01Z

@cburgdorf, please close the PR. This feature is complete and it will be 1.1.5.

cburgdorf · 2013-05-17T05:41:46Z

Done

This was referenced Apr 13, 2013

Update animator.js #2396

Closed

Don't call $window.getComputedStyle on non-element #2405

Closed

cburgdorf referenced this pull request Apr 21, 2013

fix(ngAnimate): prevent animation on initial page load

570463a

mhevery reviewed May 1, 2013
View reviewed changes

cburgdorf and others added 2 commits May 2, 2013 21:55

fix:(animator): Correctly handle multiple durations

669d31d

In case multiple durations are specified like so: transition-duration: 0.5s, 3000ms; transition-property: left, height; The animator will pick the longest duration. Fixes angular#2373

cburgdorf closed this May 17, 2013

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix:(animation): Animator does not correctly parse transition durations with multiple values #2381

fix:(animation): Animator does not correctly parse transition durations with multiple values #2381

cburgdorf commented Apr 11, 2013

cburgdorf commented Apr 12, 2013

petebacondarwin commented Apr 16, 2013

petebacondarwin commented Apr 16, 2013

cburgdorf commented Apr 16, 2013

petebacondarwin commented Apr 16, 2013

petebacondarwin commented Apr 16, 2013

cburgdorf commented Apr 16, 2013

petebacondarwin commented Apr 16, 2013

petebacondarwin commented Apr 16, 2013

cburgdorf commented Apr 16, 2013

cburgdorf commented Apr 17, 2013

matsko commented Apr 23, 2013

cburgdorf commented Apr 23, 2013

romiem commented Apr 23, 2013

romiem commented Apr 23, 2013

cburgdorf commented Apr 23, 2013

matsko commented Apr 24, 2013

cburgdorf commented Apr 24, 2013

matsko commented May 1, 2013

mhevery May 1, 2013

cburgdorf commented May 2, 2013

cburgdorf commented May 2, 2013

matsko commented May 6, 2013

matsko commented May 9, 2013

matsko commented May 17, 2013

cburgdorf commented May 17, 2013

fix:(animation): Animator does not correctly parse transition durations with multiple values #2381

fix:(animation): Animator does not correctly parse transition durations with multiple values #2381

Conversation

cburgdorf commented Apr 11, 2013

cburgdorf commented Apr 12, 2013

petebacondarwin commented Apr 16, 2013

petebacondarwin commented Apr 16, 2013

cburgdorf commented Apr 16, 2013

petebacondarwin commented Apr 16, 2013

petebacondarwin commented Apr 16, 2013

cburgdorf commented Apr 16, 2013

petebacondarwin commented Apr 16, 2013

petebacondarwin commented Apr 16, 2013

cburgdorf commented Apr 16, 2013

cburgdorf commented Apr 17, 2013

matsko commented Apr 23, 2013

cburgdorf commented Apr 23, 2013

romiem commented Apr 23, 2013

romiem commented Apr 23, 2013

cburgdorf commented Apr 23, 2013

matsko commented Apr 24, 2013

cburgdorf commented Apr 24, 2013

matsko commented May 1, 2013

mhevery May 1, 2013

Choose a reason for hiding this comment

cburgdorf commented May 2, 2013

cburgdorf commented May 2, 2013

matsko commented May 6, 2013

matsko commented May 9, 2013

matsko commented May 17, 2013

cburgdorf commented May 17, 2013