fix(tooltip): respect animation attribute

[chrisirhc]

Previously, it was only possible to set animation to false with an empty
string or through global options.

I don't think attributes that don't expect strings should be using attrs.$observe as the interpolation just creates extra work.

I suggest to use watchers on the scope, which isn't any performance impact as the attrs.$observe uses a watcher behind the scenes, anyway.

The same thing can be said of the popUpDelay but these changes might break existing setups. Hence, I left that as a separate refactor commit that can be dropped from this pull request and left for later.

[pkozlowski-opensource]

@chrisirhc yes, I also don't feel comfortable with interpolation being used for boolean / one-off settings. I would even go as far as removing watchers on those attributes altogether and evaluate those attributes only once. AFAIK the initial code was written like it was for consistency reasons but I don't think it is enough of justification to add complexity / perf overhead to the code.

[chrisirhc]

I agree, especially for these options that are very unlikely to change during usage of the directive. I could re-work this PR to do change options to evaluate once and we can keep it in consideration.

[pkozlowski-opensource]

Yep, I think we should evaluate it only once. It makes the source code simpler but even more importantly - makes reasoning about the directive behaviour simpler.

[pkozlowski-opensource]

@chrisirhc so do you want to rework this PR based on what we've discussed so far? Should this one be closed then?

[chrisirhc]

@pkozlowski-opensource , I've reworked this PR but only changed the logic for the animation attribute.

As I worked on this, I realized that some of these observers can be transitioned into using the isolate scope (scope: {attr: '@'}) after moving to AngularJS 1.2. This is because the isolate scope can be used alongside other directives after angular/angular.js@909cabd .

As such, I'm thinking we should remove those observers later instead of changing the logic now. What do you think?

Thinking based on use case scenarios, here's how likely I think the options will change after the creation of the tooltip:

Most likely to change:

• Title
• Body

Might change:

• Placement

Unlikely to change:

• PopupDelay
• Animation
• AppendToBody

If we were to modify the directive based on this, the last three options don't need observers.

Another thing to note, Title and Body should be templates strings (that use the {{}} syntax), but not the rest of the options. In my opinion, an option should take a template only if there's a use case for that option to contain concatenated strings. For example, it doesn't make sense for Placement to be two strings joined together, it's always one word/value as a whole. I think this can be a way to decide whether we would interpolate options (templates) instead of using $eval.

We can specify the type in the documentation.

[pkozlowski-opensource]

@chrisirhc OK, let's merge the minimal change for now to fix the issue reported by the users.

I mostly agree about what you are saying, the only difference being the placement attribute. I totally agree that interpolated attributes make most sense where we want to have multiple interpolations in one attribute, but I think there is one more factor to consider - what I've noticed is that people naturally lean toward using {{ }} for string type attributes. I think it is just to be able to type placement='left' instead of placement="'left'". This is why @bekos opened those 2 PRs: #1459 and #1460.

But I think this all is a more general issue in AngularJS as it is nor crystal-clear when to use @ and when =. I think we could try to write a small guide based on our experience and discuss this with the core team. Another related issue is how to properly how a given attribute is interpolated / evaluated.

[pkozlowski-opensource]

Landed as 54e614a.

[chrisirhc]

@pkozlowski-opensource , actually, that's why in the case for placement, I was thinking it shouldn't even allow interpolation, only allow strings. It might not make sense for that variable to be "live", as we're not supporting ways to change it after it has been set up.

I'm just not sure how important is the use case of allowing interpolation for this particular attribute. We probably need some statistics for that.

[pkozlowski-opensource]

@chrisirhc if you ask me it makes sense to not evaluate those attributes and thus simplify things considerably.

[chrisirhc]

@pkozlowski-opensource yep, agreed. I wonder if we can have a poll for such decisions heh.