refactor: some cleanup / refactoring

[vicb]

I'll annotate the diff where it could help understanding. Otherwise, minor modifs.

Great work on your side !

[vicb]

Explains why I like the "toggle" variant.

[codelogic]

I rebased on misko's change detection, want to rebase send another pull request?

[vicb]

If you agree with the changes, I'll do that tomorrow

[vicb]

@codelogic this PR has been rebased

[vicb]

@codelogic any chance you can merge this today or do this need further changes ?

[codelogic]

@vicb sorry it's take me so long to get to this. After talking with Misko I rewrote a bunch of the Animate interface and how it integrates into angular at a block level, added tests for some of the pieces.

[vicb]

@codelogic IF you think this PR would be valuable, please let me know. Then we'll agree on a good timing for rebasing so that I don't have to rebase every other day. Thanks.

[codelogic]

@vicb I haven't contributed much to open source projects on github before, so I apologize in advance for all the rebasing and mess I'm making with the pull requests and merging as I'm stumbling through some of this. I'd love to pull in your cleanup changes, but a lot has changed since the initial PR.

[vicb]

@codelogic, not a problem, really. Just ping me when your branch stabilizes so that I have time to rebase and you to merge before futher major changes.

One "secret" with git is to avoid rebasing "shared" (= long running) branches. Instead of rebasing your branch on master you can merge master into your branch (or change-detector here IIRC)

[codelogic]

@vicb The branch is stable, tests have been added (and all pass the last time I ran them) and I'm waiting for feedback.

[vicb]

I'll check the code tomorrow morning (my time), any special point to check more carefully ?

I'll also rebase this PR.

[codelogic]

Most notably the main Animate interface is now NgAnimate (naming), and it treats everything as a list of elements instead of single elements per misko's feedback. It makes the implementation more complicated in the case of the CssAnimate implementation but it should work more efficiently for manipulating large blocks of content.

Finally, my preference would be for the toggle implementation to be separate from add and remove (even if it just forwards to add and remove) and it should include tests for the implementation.

Thanks!

[vicb]

@codelogic not sure if I'll have time today... more probably early next week.

I don't really understand what you mean by "the toggle implementation to be separate from add and remove " ?

[vicb]

@codelogic I've found time to rebase this PR, please merge if you agree with the changes. I'll have time for a deeper review tomorrow.

Could you please detail your thoughts about "toggle" (ie your previous comment). Thanks.

[vicb]

@codelogic one quick tip: you can enable travis ci on your repo by creating an account on https://travis-ci.org and enabling your repo in the configuration. The tests would run each time a PR is updated.

[vicb]

@codelogic ping ?

[vicb]

Thanks.

What about "Could you please detail your thoughts about "toggle" (ie your previous comment). Thanks." ?

Cheers,
Vic

[codelogic]

I've given it a bit of thought and right now I'd like to leave addClass and removeClass separately since this is how angularJS's $animate service is written.

[vicb]

@codelogic don't really get what you mean

• add and remove would remain exactly the same,
• toggle would be added,
• it more than often allow simplifying calling code (=syntactic sugar)

If your concern is ng.dart having toggle while ng.js don't I guess that would not be the only (the most important) API difference ?

[codelogic]

@vicb Concerns:

• Increase in api surface only to shorten two directives, namely, ng-show and ng-hide.
• Add an api method that will have to be implemented twice (CssAnimate does implement, not extends on NgAnimate)
• toggle has two meanings, my first impression is that it would inspect the element, and if the class is present it would remove it, if it's not present, it would add it. I know this is not what you are intending (since you expect to provide a boolean for add/remove)
• Inconsistent with existing list and set interfaces. Set doesn't have a 'toggle' method even though it might be convenient to 'toggle' an element in and out of a set, add and remove are more explicit.

In general, I feel toggle doesn't make the api simpler for the added complexity and inconsistency it adds.

[vicb]

@codelogic valid concerns, thanks for the detailed explanation.