popover-template directive - addresses issue #220

[jbruni]

Addresses feature request "support template url for partial" #220

Improved over @joshdmiller PR #369

Updated from @jbruni PR #1391 for version 0.10.0

Resolves properly the ng-model binding issue seen at http://plnkr.co/edit/cbqOnktHhxSjeLIBE1w7?p=preview

[chrisirhc]

(Updated comment)

The destruction of the tooltip after it's hidden was made for performance reasons (#1455). When the tooltip is not visible, it should no longer attempt to update its contents. If users hover many tooltips each with many directives, the application may slow down due to it trying update DOM that's not visible. This is similar to the difference between ng-if vs ng-show.

Perhaps an option needs to be created for this as well.

Another possibility to consider is to only keep the one latest tooltip DOM. All other previously hovered tooltips are not kept. This way, hiding/showing the same tooltip will be fast.

[jbruni]

@chrisirhc ... as I've just wrote here: "Are you guys sure that you want to enforce total removal (full destruction of DOM and binded events/data) upon each hide/show of a tooltip/popover? In my use case, there is a popover with Date Picker and other components... this approach is performance killer, as it takes quite some time for it to render. Why not detach? The tooltip/popover is still fully destroyed when the triggering element scope is destroyed. (This is also my case: when the ng-view changes, the tooltip is effectively removed... that's why the scope $destroy listener is there, isn't it?)"

Well... I may be wrong, but, at least, I am well aware of the leak issue... relating with the popover issue, Josh first mentioned it here... I myself referred to it later here, here, here...

And, in fact, I had to deal with the leaking problem... if you look at my comment explaining the usage of detach, you will see that I linked to the pull request resolving the original eye-opener-issue from Joe Grund.

According to my understanding, it is at this comment that Joe explains the leaking problem and also its solution.

So, I am sharing all this history above only to make the point that I am not ignoring the leak issue. It is quite the opposite: I believe I am aware of it in depth.

At my early first approach writing the popover-template directive, before I acknowledged the above, I simply substituted remove by detach. Then, with proper understanding, I went further, keeping the necessary removal intact, but balancing the "remove everywhere" code, with a "remove where necessary" approach - sufficient to avoid the leak issue.

So, either I am missing some point to understand what justifies the enforcement of a removal inside "createTooltip" (a step back to the "remove everywhere" style), or, as it seems to me, this enforcement is fruit of a misunderstanding of the issue - thus trying to apply a wrong solution to it.

I've already accepted that I may be wrong, and I would really enjoy to be teached and learn any new knowledge I may be missing here.

If BTW I am not wrong, then it seems to be just like I've said: the "removal enforcement" in the "createTooltip" does not address the leak issue; the enforcement in the scope destruction does. As the fastest "workaround" for my popover-template upgrade to 0.10.0, I changed the "removeTooltip" for "return" in the "createTooltip" context, and re-enabled the detachment possibility. The "return" simply promotes the usage of the already rendered tooltip.

Maybe it is the case of letting the developer decide if it should "detach" or "remove" on close. If there is a "slow to update" thing hidden, better have it removed. But if there is a "slow to render" thing, better have it detached. My use case is this: there is a delay when the popover is opened... but due to the detachment there isn't anymore when it is opened the following times... with removal/re-rendering, there is a delay each and every time the popover is opened. It is a popover which contains not only a date picker but also other inputs. This is why I don't see the current approach as optimal. And I fail to see the other leak potentials since we already have it addressed... see #486 (comment)

Thanks for the attention, and forgive me if I'm missing some obvious or not-so-obvious points. Sincerely.

[chrisirhc]

Sorry, I think you wrote the comment before seeing my update to the above comment. Github doesn't seem to handle edits well.

[jbruni]

Indeed, @chrisirhc, I wrote the previous comment before seeing your updated comment.

Well, it seems we had similar ideas. This is nice! Ok.

I also understand the above discussion itself, about "detach possibility versus removal enforcement" is in fact totally independent of the current PR.

Regarding the PR, I don't expect it to be merged as it is. But people (starting with myself) are using this proposed directive, so I just moved it forward... ok? If someone else could take this effort another step further, and make the "compile scope" available for the directive in a more elegant and less hackish way, it would be great.

As for the detach/remove thing, I think we already agreed that there are use cases for both... "how to best implement a solution?" is the new question.

At the moment, I do not have availability to focus and dive deeper on these. I can only thank the whole team and community efforts. So... thanks!

[jbruni]

Anyone willing to move this forward, please refer to #1391 (comment) and #1391 (comment). Thank you.

[chrisirhc]

@jbruni Thank you for your contribution and helping everyone else get this feature.

[rainboxx]

@jbruni I added this to my development environment and I have two issues:

1. scope.compileScope() evaluates to undefined and I'm getting the following message:

Uncaught Error: [ng:areq] Argument 'scope' is required
http://errors.angularjs.org/1.2.10-build.2148+sha.fced1c0/ng/areq?p0=scope&p1=required (caused by "undefined") (caused by "undefined") (caused by "undefined") (caused by "undefined") angular.js:78
(anonymous function) angular.js:78
assertArg angular.js:1363
publicLinkFn angular.js:5520
(anonymous function) directives.js:93
wrappedCallback angular.js:10949
wrappedCallback angular.js:10949
(anonymous function) angular.js:11035
Scope.$eval angular.js:11955
Scope.$digest angular.js:11781
Scope.$apply angular.js:12061
done angular.js:7843
completeRequest angular.js:8026
xhr.onreadystatechange

2. The positioning of the popover is off (vertically not centered). I think this is due to the fact that the template is loaded asynchronous and the positioning code positions the popover according to the actual content at the time of positioning. Once I remove the code from within div.popover-content via the dev tools the (empty) popover is positioned correctly.

Did you also try to fiddle with some transclude stuff? In general I'd think this would be a nice approach but AFAIK would require adding each popover as an E directive into the template, so it would just show or hide the popovers (which could still be positioned on the fly).

EDIT:
Versions:

• AngularJS v1.2.10-build.2148+sha.fced1c0
• angular-ui-bootstrap 0.10.0 - 2014-01-13

[Thinkscape]

My latest findings and aches:

• The compilation scope is bogus. It puts the content's controller in limbo, unable to target either the parent popover that holds it, nor parent controller... The simplest use-case is this: I have a custom directive <foo bar="x"> that renders a <button popover-template="xyz.html">. The directive takes bar="x" argument and stores it in scope, which is supposed to be accessible within the popover - which is currently close to impossible to do.
• The whole tooltip component is a memory and cycle hog. The problem is, it initializes the popup content (including the template in case of popover-template) with all controllers and directives inside it, AT COMPILATION PHASE. I'm using popovers in a grid, which makes the directive instantiate tens of popovers which might never be actually shown... regardless, templates are pulled, structured are constructed, handlers, watchers, everything. This obviously kills performance. What I expected is that popups are built only when needed, i.e. after the user clicks the trigger.
• No way to control the popover visibility from up or downstream. I've solved this in my custom compilation by adding $scope.tt_open = tt_open etc. This could also be handled by an event broadcast (i.e. $scope.on('tooltip.close', tt_close))

[jbruni]

Guys - back to #1391 (for version 0.9) and #1046 (for version 0.7) it worked fine.

In fact, I do use the version 0.7 one (PR 1046) in the project I've delivered to a client.

The following update (PR 1391) for version 0.9 was done due to userland request. But this was an easy one to do.

The latest update (PR 1593) for version 0.10 - this current PR - was done in a hurry, only due to userland request, and I have NOT in fact used it - I only tested when using a Plunkr to quickly make things work . I am not using it anywhere. My delivered project is fine with version 0.7.

While the previous update (0.7 to 0.9) was smooth, the latest (0.9 to 0.10) was the opposite: nothing seemed to work as before. I managed to make it "work" in the Plunkr and went no further. A complete tooltip refactoring is on the way. I am already involved with another projects. I do not have either motivation nor time.

In short: I have made my contribution on this, but I do not have availability to continue. I thank you for all support and comprehension!

[nkpz]

Regarding the positioning initially being off, I was able to work around this by wrapping the call to positionTooltip in tooltip.js in a $timeout and calling $templateCache.put('template url', $http.get('template url')) .. not an attempt at an elegant solution, but it seems to be working alright so far for a 2 minute fix. https://github.com/nnpp/bootstrap/blob/master/src/tooltip/tooltip.js#L231

[chrisirhc]

For those looking around for this, I've put up a work-in-progress pull request with this feature: #1848 .

[jbruni]

This PR became obsolete, so I'm closing it.