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

refactor($animate): use single variables for handling transition/animati... #3908

Closed
wants to merge 2 commits into from

Conversation

mgol
Copy link
Member

@mgol mgol commented Sep 6, 2013

...on events & properties

To avoid code duplication, use single variables for keeping properties/events
names to use. Also, fix some errors that have happened after the rewrite from moment ago.

@mary-poppins
Copy link

Thanks for the PR!

  • Contributor signed CLA now or in the past
    • If you just signed, leave a comment here with your real name
  • PR's commit messages follow the commit message format

If you need to make changes to your pull request, you can update the commit with git commit --amend.
Then, update the pull request with git push -f.

Thanks again for your help!

@mgol
Copy link
Member Author

mgol commented Sep 6, 2013

The bot @mary-poppins lies, I've signed the CLA long ago and got even a couple of pull requests merged...

@mgol
Copy link
Member Author

mgol commented Sep 6, 2013

I've also changed order of rules applied in tests - it's important that unprefixed rules are at the end, before vendor prefixed ones in case there are any implementation differences between these two and a browser implements both (and yes, it has happened in the past, e.g. with linear-gradient).

@mgol
Copy link
Member Author

mgol commented Sep 6, 2013

I separated the $animate simplification and correcting tests to separate commits for clarity.

@ghost ghost assigned matsko Sep 6, 2013
@mgol
Copy link
Member Author

mgol commented Sep 7, 2013

This should be ready for merging. :) cc @matsko @mhevery.

To avoid code duplication, use single variables for keeping
properties/events names to use. Also, fix some errors that have
happened after the rewrite from moment ago.
@mgol
Copy link
Member Author

mgol commented Sep 27, 2013

Just wanted to point out this Travis failure is not my fault. ;) A browser got disconnected

@matsko
Copy link
Contributor

matsko commented Sep 27, 2013

@mzgol wonderful work!

@mgol
Copy link
Member Author

mgol commented Sep 27, 2013

Thanks, @matsko :)

@matsko
Copy link
Contributor

matsko commented Sep 30, 2013

The changes look great. I'm just waiting on one final commit to be placed in and I'll rebase your code again and push the update: #4171

@matsko
Copy link
Contributor

matsko commented Sep 30, 2013

Here's the final rebased branch: https://github.com/matsko/angular.js/commits/pr_3908

I added an extra comment and fixed the merge commits so it works on master.

Just waiting for the CI server to finish testing it. Let me know if there is any issue before I merge it to master. Any last changes or things you see that should be fixed?

@matsko
Copy link
Contributor

matsko commented Sep 30, 2013

OK @mzgol waiting on your final say, the PR is up to date and bug free :) https://github.com/matsko/angular.js/commits/pr_3908

@mgol
Copy link
Member Author

mgol commented Oct 1, 2013

@matsko I've just looked into it and it looks ok. So if tests are passing, 👍 from me. :)

@matsko
Copy link
Contributor

matsko commented Oct 1, 2013

MERGED

@matsko
Copy link
Contributor

matsko commented Oct 1, 2013

Landed as f316c31 and ac2d06b

@matsko
Copy link
Contributor

matsko commented Oct 1, 2013

Thanks @mzgol for putting this together.

@matsko matsko closed this Oct 1, 2013
@mgol
Copy link
Member Author

mgol commented Oct 1, 2013

Thanks for merging! :)

@mgol mgol deleted the jquery branch October 1, 2013 16:48
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants