feat(popover): support template url for partial

[mjp0]

As requested in #202 by @joshdmiller here's my request for external partial support for popover which would make popover much more versatile and lower the need of duplicate code in the apps. Already working implementation can be found in one of the "competing" Bootstrap implementations at http://mgcrea.github.com/angular-strap/#/popover.

[joshdmiller]

I'm thinking just <a popover popover-template="{{templateUrl}}">Open Me</a> would work well.

The issue we'll need to solve for, however, is getting the scope right. The tooltip and popover use child directives with isolate scopes, so we may have to do some transclusion magic to get this to work as expected. Thoughts?

---

FWIW - AngularStrap isn't really a "competing implementation" as it's not an "implementation" at all - it just wraps the original jQuery. We can't really translate what they do back here; we really have to re-engineer the original Bootstrap plugins to be in line with the "Angular Way".

[mjp0]

I have to confess that I'm not familiar enough to give you any advice on scope. My directives so far has been so basic that I haven't had to deal with them, or transclusion. But I think that transclusion might be problematic if you need A LOT of popovers for different elements. I'm not worried that much about desktop but tablets worry me when it comes to doing a lot of binding etc.

[jhiemer]

Hi,
I took a look into the last commits. The tooltip has the option of loading external content now. In the demo files I could not find any reference to that for popover. Does this work at well?

[joshdmiller]

@jhiemer The tooltip now supports putting HTML into the tooltip text to be unsafely included into the template. The popover doesn't. However, this issue is for external templating or partial support for the popover. i.e.:

<a tooltip-html-unsafe="<strong>Hello!</strong">Hover!</a>

versus

<button popover-template="'partial.tpl.html'">Click me!</button>

It doesn't make any sense, I don't think, to do partials for tooltips, but we can do HTML content in the popover with a fairly simple code change.

[jhiemer]

@joshdmiller I think doing partials for tooltips is not necessary as well. Perhaps there might be some usecases, but the main usage scenarios will cover just plain text. For the popover instead I see many different usage scenarios. For example I got a date picker, which could be put into the popover as well as a color picker.

Which changes would be necessary?

[jhiemer]

@joshdmiller any update on this?

[joshdmiller]

@jhiemer Sorry - I remember responding to this days ago... insanity can be inconvenient.

To implement an independent template, we need to decide on a few things:

Syntax

I think something like this:

<button popover popover-template="'partial.tpl.html'">Click me!</button>

Template

The popover default is this:

<div class="popover {{placement}}" ng-class="{ in: isOpen(), fade: animation() }">
  <div class="arrow"></div>

  <div class="popover-inner">
      <h3 class="popover-title" ng-bind="title" ng-show="title"></h3>
      <div class="popover-content" ng-bind="content"></div>
  </div>
</div>

Should we just, in essence, transclude the contents into the popover-inner element? Or is that too generic? We could also do it in popover-content to keep the default popover design from Twitter.

Scope

In which scope is the template compiled? A new sibling of the directive makes sense to me.

[jhiemer]

@joshdmiller thanks for getting back at this. Essentially I think the provided code could be what I am looking for. I created two examples of small directives which I would like to put into a popover and which you can see here: http://plnkr.co/PQOWFO and here: http://plnkr.co/99HHNd.

Both cases would need to access the parent scope. This issue #315, is reaching for something similar I guess.

[jhiemer]

@joshdmiller do you have a working version of your changes where I might take a look at?

[ffesseler]

any news on this one ?

[jhiemer]

+1

[joshdmiller]

I just opened a PR for the custom triggers and it changes the $tooltip service somewhat significantly so I won't push this until that PR goes through. But for those curious, here's a rough implementation: http://plnkr.co/edit/RTpGfZG0cl069nkqmm14?p=preview

[jhiemer]

@joshdmiller took a look at it. When I create a real template file, the contents still renders as mypopover.tpl.html so it does not seem to get loaded. Could that be right?

[joshdmiller]

Damn! There seems to a bug in Plunker - I lost a ton of work! I opened a bug report - filearts/plunker#39.

Anyway, here it is re-created: http://plnkr.co/edit/JgeUP0anaU4BmsS5cJNh?p=preview. Can you give it another go?

[pkozlowski-opensource]

@joshdmiller this doesn't seem to work for me, I'm getting "Error: selectors not implemented"...

[joshdmiller]

I'm not surprised it doesn't work - there are files missing! Now I'm very frustrated. Plunker's not keeping my most recent changes. This has happened twice now on this same issue - and I re-created it from scratch!

I re-created the changes once again, but it still won't save the changes I'm making even though it tells me it does. I'll just submit a PR later and we can go from there...

[pkozlowski-opensource]

Yeh, this happened to me couple of times as well... Now I'm using plunker for rather simple experiments only...

[jhiemer]

Great, waiting for it @joshdmiller. After you submitted it I will review it asap.

[joshdmiller]

Has anyone had a change to look at #369?

[jhiemer]

Hi,
taking a detailed look at it today!

[jhiemer]

@joshdmiller ok took a first look. When using the inline templates it works as expected. When I try to include templates from 'partials/entity/test.html' I get a large digest(); error.

Is this not allowed to load external urls? I think loading external stuff would be great, as I would enable us to put the datepicker directive of @bekos into the popover.

[joshdmiller]

@jhiemer Can you post a Plunker? It works for me: http://plnkr.co/edit/Mmq5dPi6MIYeEMoCrYSB?p=preview

[jhiemer]

@joshdmiller indeed you are right. I found the bug. It was an error on my side. Need to do some scope testing now, but I feel comfortable with the solution by now.

[jhiemer]

Good, did some further test. Exactly what I was looking for...will try the datepicker now.

[madhums]

+1

[aasif]

+1

[sjungwirth]

+1

[yordis]

@danielcompton your suggestion is shut down the opinion of the people. This add value to the issue, you know why, because you can see the effort from them. People like me trust them and the fact some guys even create the PR + Unit Testing + Doc and they don't do anything about.

This was 2 years ago sir, this wasn't 1 month and look the how many +1 are here ..... Close the comments, "good way" of fix problems.

[danielcompton]

@yordis I think we're all well aware of the need for this feature, and I too am frustrated that it's not available yet. However GitHub issues aren't the place for complaining about a bug or missing feature - they're for fixing it. The negative comments aren't adding anything of value, and they're bringing down the contributors. If you want something to change, your options are:

1. Become a contributor to angular-ui, and land the patch in a way that fits with the contribution guidelines
2. Do nothing and wait for the feature to arrive sometime in the future
3. Use a different UI library with this feature
4. Use a fork of angular-ui with this feature

If you just want to complain, then I suggest you save it for your bartender 😉.

[hallister]

This issue is a tough one, largely because it's been buzzing around for so long. I've been debating the merits of closing this for I don't know how long, largely due to a couple of significant fixes for this issue as open PR's. And since I'm not actively contributing to the project right now, it's debatable whether or not I should.

That being said, this project lost momentum as many of the maintainers had to real-life obligations and other projects take over. As a result the current/new maintainers have a lot of work ahead of them with some 200 PR's and over 500 open issues.

As a result, I'm going to close this for now (with the caveat that a current maintainer is more than welcome to re-open for any reason). Not because I have any desire to close down discussion, but because this horse has been dead a long time and it's less a matter of needing a fix and more a matter of needing to fix other things in order for this to get merged in.

[DaveWM]

@hallister surely in the time it took you to write that comment you could've just merged in one of the multiple open PRs for this (#1848,#2479, etc.)

[hallister]

@DaveWM Both of those have merge conflicts that need to be sorted out and I'm not actively contributing to the project right now, so it's highly unlikely that I could have merged it quickly.

Furthermore, because there are so many outstanding PR's, merging into master right now is going to do nothing more than cause more problems. With 200 open PR's, a great deal of care has to be taken on what gets merged.

I realize everyone is frustrated with the pace here, but this is still an open source project developed entirely by volunteers. I realize this issue has been around for a long time, but it's going to take a little more patience before this is merged in.

[RobJacobs]

@DaveWM @hallister My thoughts are this should not be part of the library. We should be focusing on making directives that have a single responsibility that people can use as components to build more complex directives. We shouldn't build directives that transcludes 2 different content areas, handles dismissing, and handles popup placement all in one. Rather these should be discreet directives (dismiss, placement, etc...) that are independent and can be re-used. That being said, here is a plunk that demonstrates that approach for the popover. You will notice I am using the native TWBS markup and classes to build the popover and only have a single directive that handles the dismiss. No need to transclude content, or trigger a scope.$apply to reposition the element after content changes, or the numerous other rabbit holes we go down to support a 'god' directive. While it is convenient not to have to write all that html, that should be a convenience reserved for simple cases.

I am also working on an approach for more placement options by using classes (can be bound to ng-class for dynamic placement), which can be seen in this plunk. This does not use pixels to set top, left, right, or bottom so as content grows, the container will resize without losing the anchor point.

If you are set on template support in the existing popup directive, I have a pull request here with all conflicts resolved against the current master branch,

[seiyria]

@RobJacobs I strongly disagree that this shouldn't be part of the library, and clearly so do many others. If the goal of this library is to recreate TWBS functionality with Angular-style markup, anything like this should definitely support templates at the bare minimum.

[DaveWM]

Completely agree with @seiyria. @RobJacobs, you seem to be saying that the entire popover directive should be re-written, and regardless of whether that's a good idea or not, it's not really what's up for discussion here. Also, I hardly think adding something as simple as dynamic content would make popover a "god directive".

[pioneer903]

Interesting thing, but it looks like popover template worked before. I found working plunkr with angular 1.2.9. However 1.3.x doesn't work.
As an alternative, i found nsPopover. Going to give it a try

[jbruni]

Since my previous comment on 28 May 2014, I've been away. Surprised to see that this is still an issue, I have just made a new version, mergeable to current HEAD.

• See it in action
• Get the ready-to-use code here or here

There is a difference now. While I was not expecting merge for my previous versions, because I knew they dissonate from the project's "roadmap", so to say... this time the patch does not affect absolutely nothing on existing code and embraces the design decisions taken. (While I previously made it NOT destroy the tooltip when it was hidden, now I kept the tooltip-removal code untouched.)

This is why, unlike many of my previous attempts, at this time I am opening a new PR. Just as I knew chances of merging were poor in the past - this time I sincerely believe there is no reason for not being embraced. The code is absolutely simple, unobstrusive and harmless. Please, take a look and consider. Thank you.

• Here is the Pull Request, where you can see the minimal and simple code.

[chrisirhc]

#1848 is pending a name poll, otherwise, it's mostly good to merge.

[QuentinFchx]

Can't believe this just happened 😂
Congratz everybody and especially @chrisirhc

[nomospace]

Your guys kicked ass!

[GabrielDelepine]

I want to cry. Thanks everybody.

[karianna]

FYI - Actually resolved in 0.13.0 but due to GitHub DDOS I can't change the milestone

[metanav]

Will it be part of 0.12.2 release?
What is the expected release date (or month) of 0.12.2 or 0.13.0?

[karianna]

There won't be a 012.2, 0.13.0 is the next release, no concrete ETA yet.

[sebastianovide]

fantastic !. any ETA yet ?

[HeavenGoGo]

just wait the release version, the issue last for too much time

[eabovsky]

+1

[funkybaboon]

+1

[chrisirhc]

Locking this issue as the feature has been released. Please open a new issue if there are further comments.