fix tooltip.addClass() argument

[suncat2000]

If used jQuery UI, second argument for .addClass() is duration

.addClass( className [, duration ] [, easing ] [, complete ] )

.addClass()

duration (default: 400)
Type: Number or String
A string or number determining how long the animation will run.

tooltip.addClass(placement[0], options.placementClassPrefix + ttPosition.placement);

This code give move animation with default duration value {duration: 400}
May be this animation effect not needed?

[icfantv]

@suncat2000, i'm not sure what problem this PR is attempting to solve. We do not rely on jquery at all and certainly not jquery UI.

[suncat2000]

@icfantv tooltip have fade in animation optional, may be add option for fix this animation effect?

[icfantv]

@suncat2000, i don't follow. what about your jqueryUI comments?

[suncat2000]

@icfantv jQueryUI library override some jQuery methods Method Overrides. For example .addClass() method.
This built-in jQuery signature .addClass( className )
jQuery .addClass()
and this jQueryUI signature: .addClass( className [, duration ] [, easing ] [, complete ] )
jQueryUI .addClass()

if I use jQueryUI and angular-ui/bootstrap v1.0.x in project, this code:

tooltip.addClass(placement[0], options.placementClassPrefix + ttPosition.placement);

worked with animation, because in .addClass() passed second argument.
https://github.com/jquery/jquery-ui/blob/0bfbd21d4fefa98d165b7d50277bd23be84e919a/ui/effect.js#L871

[icfantv]

@suncat2000, we don't support running Angular UI Bootstrap with jQuery UI because you're not supposed to be using Bootstrap with jQuery UI. The amount of bloat that would add is just staggering.

The addClass method is provided by jqLite which is bundled with Angular. If you pull in the full version of jQuery in front of Angular, then you wind up using the full version of jQuery. If you pull in another 3rd party library which overrides an API method used by Angular (either natively or via a 3rd party) then this is something that's way beyond our control.

[pkozlowski-opensource]

Agree with @icfantv - we really don't want to go down this (very slippery) path...
This would be -1 from me for this PR

[suncat2000]

@icfantv, @pkozlowski-opensource I agree use jQueryUI in conjunction with Angular not the best solution, but I use this popular package angular-dragdrop and this package has dependency from jQueryUI.
I need migrate to another package (without dependency from jQueryUI) or use only 1.14.3 version of angular-ui/bootstrap because of one line of code?

[suncat2000]

@icfantv, @pkozlowski-opensource the effect that I get when using angular-ui/bootstrap v1.0.x with jQueryUI: http://take.ms/kIsaH

[suncat2000]

@icfantv, @pkozlowski-opensource if I understand correctly, Angular's jqLite provides buit-in jQuery .addClass() method with one argument String or Function:
https://docs.angularjs.org/api/ng/function/angular.element
http://api.jquery.com/addClass/
in current code .addClass() method called with 2 arguments:

tooltip.addClass(placement[0], options.placementClassPrefix + ttPosition.placement);

What for?

[wesleycho]

Looking at jqLite's code here, it seems that it also doesn't support the second argument.

This looks like there is something wrong with the addClass method here.

Worse, it looks like removing either the first or second parameter doesn't break the tests.

@RobJacobs looking at the history, looks like d265113 is what caused this - can you give an explanation of what was intended here?

[RobJacobs]

@wesleycho That was an error on my part, this PR fixes a legitimate bug. The intent was to add the primary placement class (top, left, right, bottom) that are supported by the TWBS css and the placement class generated by the position service (primaryposition-secondaryposition). The second class is added to give folks a chance to use their own css and use the positioning approach described in my first post here

[wesleycho]

Alright, that's what it looked like, but I wanted to be absolutely sure.

[wesleycho]

I am going to merge this, and open a PR with a unit test for this just so the test can be reviewed (the test is indeed fixed by this PR).

[suncat2000]

@wesleycho, @RobJacobs thanks! 👍