Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

Update animator.js #2396

Closed
wants to merge 1 commit into from
Closed

Update animator.js #2396

wants to merge 1 commit into from

Conversation

romiem
Copy link

@romiem romiem commented Apr 13, 2013

Added support for transition-delay property.

Added support for transition-delay property.
@cburgdorf
Copy link
Contributor

Pretty cool! I'm happy to rebase my PR (#2381) on top of that if there will be an official statement. /cc @IgorMinar @mhevery

@cburgdorf
Copy link
Contributor

Ah actually, I think you should rebase on mine since there can also be multiple delays.

@romiem
Copy link
Author

romiem commented Apr 13, 2013

Hey - you'll have to forgive my ignorance :) How do I rebase my patch against your branch/pull request?

@cburgdorf
Copy link
Contributor

We probably first wait for an official statement from the core team.

But to answer your question, it goes like this:

  1. check out your branch with your fix
  2. then add my repository as a remote (that's basically what the angular team has to do for our pull requests as well unless they merge them via the github website)

git remote add cburgdorf https://github.com/cburgdorf/angular.js.git

  1. fetch my stuff
    `git fetch cburgdorf``
  2. then rebase against my branch
    git rebase cburgdorf/PR2373

But before you do that, let's wait for an official statement ;-)

@romiem
Copy link
Author

romiem commented Apr 15, 2013

That's great - thanks for the instructions... I'm still a relative newbie when it comes to Git :)

Will wait for an official statement before making any more changes 👍

@petebacondarwin
Copy link
Contributor

This looks good to me. As @cburgdorf suggests you should probabyl build it on top of his PR. It will also need unit tests and a better commit message. Take a look at the commit message guidelines: commit message format.
Also it would be good if you can provide two commits, first one with the unit test, second one with the change to the animator.

@cburgdorf
Copy link
Contributor

There is one trap I think. E.g. when we animate on the properties left and height and we use:

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

then the overall time spent is 3s and not 4s. So you need to take that into account when rebasing and adjusting your PR. Or am I completely mistaken?

@romiem
Copy link
Author

romiem commented Apr 17, 2013

Nice spot! You're right, the patch would have to take into account the combined duration+delay if multiple times are specified.

@cburgdorf
Copy link
Contributor

yep, you can see in my PR that I also already added a test that takes multiple durations into account.

https://github.com/cburgdorf/angular.js/blob/8a3fc7d0781042247b0d86626d7f485a9aa6e2c4/test/ng/animatorSpec.js#L311

I guess once you rebased, you should more or less copy that test and then provide a situation as described in my previous comment. If you have any trouble I'm happy to help :)

@petebacondarwin
Copy link
Contributor

Closing this as the work seems to have moved over to #2381

@romiem romiem deleted the patch-1 branch February 6, 2016 19:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants