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 #1046 for version 0.7.0

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

[kasparasg]

@jbruni, thanks for this 👍 . I had a similar problem with tooltip.remove() and tried to mimic the detach() functionality too in the PR: #1403

Hopefully it gets merged in.

[Thinkscape]

This is golden. Thank you! 👍

[GabrielDelepine]

+1

[blueteeth]

+1

[devmondo]

👍

[ghost]

Does this work for Bootstrap 3? I'm a little confused because nothing is working for me.

[mattdarveniza]

Have been wanting this feature for a long time now. Works exactly as expected, would love to see in the mainline. +1

[websirnik]

+1

[tamtakoe]

+1

[Yuripetusko]

would love to see this merged.

[TomDeVito]

+1

[nkpz]

+1

[kencaron]

+1

[jbruni]

I want to recreate the PR for newer 0.9.0 version - what should I do? "Rebase" this PR? Or create a new PR?

[Thinkscape]

@jbruni Honestly, I think you're wasting your time a little bit.

This PR is great, I'm using it daily, but why isn't it merged yet into master ?
Hello maintainers? There's 33 members on AngularUi, guys and gals, please step it up and give it a review. If there's something wrong with it in your eyes, let us know and we'll refactor accordingly. I'm happy to help you @jbruni if you want, though we need to get attention of people with merge permissions so we can get this inside.

[chrisirhc]

@Thinkscape , there is a running plan to refactor to the $tooltip service to support such use cases, as such, a review isn't applicable to this PR at the moment. I have personally looked into this PR. Parts of this PR are no longer applicable due to recent changes to the tooltip service.

My main issue with this PR is that it uses manual transclusion (instead of ngTransclude or ngInclude) and the custom detach method (which won't be applicable now due to changes to the $tooltip service).

Some other issues: In Angular 1.2, you have to consider $sce for resource urls. The use of the compile-scope attribute seems a little hacky, not sure if that's needed.

Anybody is welcome to review this PR and provide more design thought, it is not only up to the members of Angular UI.

---

My current input to the discussion with regards to design of $tooltip is that I feel that the $tooltip service could take on a similar design to the $routeProvider.when's route parameter.
With that design change, you can create tooltips or popovers with custom templates.

[Thinkscape]

@chrisirhc Ok, two things then:

• The way I understand it, you want to completely drop overflow in favor of tooltip, because right now overflow is just a template for tooltip and duplicates most functionality. I get it, though it's a little out of scope of this one. I do recognise the need for refactor of this PR though.
• I see a detach in how you look at timelines here. I understand the will to refactor tooltip to whatever specs you have in mind, however in the meantime there's possibly of getting a a very much desired feature extension to existing popover component. If this PR was refactored to be in line with current tooltip changes (and kept up to date later on), I do not see any harm in having it in for everyone to use. If at any point it gets deprecated (or merged into) newer tooltip-template implementation, so be it, but I feel like it's not coming any time soon - on the other hand, popover-template seem well baked already.

@jbruni Could you take a look at the critique above ?

In my opinion:

• $sce support seems quite easy to add.
• compile-scope is a little hackish. If we dropped it, we could use native transclude:true. Compile scope is easily replaceable with i.e. $scope.$parent in userland code.

[Thinkscape]

I want to recreate the PR for newer 0.9.0 version - what should I do? "Rebase" this PR? Or create a new PR?

Take a look at how many conflicts you'd have. It's often easier to just create a new branch and cherry-pick commits (or straight on copy-paste relevant code and the refactor it)

[jbruni]

Thanks, @Thinkscape and @chrisirhc - very nice to receive a feedback from the team about the code.

My current understanding is that the PR code, although very simple, does not follow the project roadmap.

In a previous effort, I included $sce, but people wanted to use it with Angular 1.0.8 (this was before we had 1.2 stable)... so I dropped it.

Regarding the scope issue, I simply moved forward with the first working solution I've found.

So, as of now, and due to popular request, I've only made the required modifications so people (starting with myself) can simply use HTML and bindings inside their popovers with latest version (0.10.0, at this moment).

I've just submitted the new PR, not exactly expecting it to merged, but used as a temporary workaround, while the official feature is not available.

NOTE: 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?)

[phc-itt]

+1

[clicman]

+1