-
Notifications
You must be signed in to change notification settings - Fork 6.7k
refactor(tooltip): remove child scope #1600
refactor(tooltip): remove child scope #1600
Conversation
@chrisirhc I like what I'm seeing here. The other thing is that $tooltip is up to bigger changes:
+1 from me to merge this one before making those bigger changes. |
I'll look into those, this weekend. |
@chrisirhc OK. Do you want me to merge this one as-is and you will open another PR for the above? Or should we close this PR and you are going to open another one with all the fixes? |
We could start with this one. The observers is a good stuff to study with an easy fix (when there is a list of what to remove). The important thing is the templateUrls which is blocking other issues. That shouldn't be hard to implement also. |
@chrisirhc @Foxandxss I'm just looking into templateUrl stuff but would like to sort out all the opened PRs for tooltips / popovers as well as simplify the $tooltip service before doing so. @chrisirhc would be great if we could have a chat about this one, especially when it comes to scopes. My idea was slightly different to your approach, I wanted to have the Would love to chat more about it, ping me if you are available. |
So, based on the above I would be keen on splitting this PR into several ones:
WDYT? |
@pkozlowski-opensource Sorry, finally got back to this. Sounds good. I just realized that all the state can be kept in the controller if we add a TooltipController, so no child scope is needed. This is similar to what's being done to the typeahead directive (popping up something handled by a controller), except the TooltipController can be a very simple object that just contains the information needed to render a tooltip (e.g. placement, isOpen). Indeed it's a good idea to split up the changes, which ones should we work on first? And should we consider using a controller as I mentioned above? |
@chrisirhc my main objective for this directive is to easily support templateUrl(s) for content and "manual" triggers as well as get rid of scopes so people are not confused. But in order to do so we should simplify this $tooltip service. My proposal is to split this PR into:
WDYT? |
@pkozlowski-opensource Makes sense to break up the changes, but it'll take a bit of work. I'll look into it later next week. I like the idea of tooltipWindow and tooltip directives. I'm guessing the tooltip directive's contents will be evaluated once on each trigger and won't be updated while the tooltip is being displayed, while the tooltipWindow will have full two-way binding capabilities? Since two-way binding isn't possible (without leaking) if we don't want to introduce a new (child/isolate) scope. |
Really appreciate the work on this @chrisirhc; mind if I ask how things are progressing? |
@pkozlowski-opensource , I think this has been sitting around for too long. The changes proposed aren't too drastic. Also, due to a change (angular/angular.js@531a8de) in AngularJS 1.3.1, tooltips are broken due to our use of observers, so merging #1817 would fix it. |
My stab at removing the child scope. This is a proof of concept right now. Opening this for discussion and review.
I considered replacing the directives with ngInclude since they're just placeholders for templates but I ran into issues removing the DOM as the ngInclude replaces the DOM node. Hence, I thought using ngInclude probably increases code complexity to handle its quirks. Anyways, using the popup directives might come in handy if we want some custom link logic.
Closes #1269
Note: While working on this, I realized I introduced a watcher leak to the tooltip (similar to what was going on with the modals). Looking into that now. However, that leak is fixed here.