feat(popover): created popover-template directive by joshdmiller · Pull Request #369 · angular-ui/bootstrap · GitHub
Skip to content
This repository was archived by the owner on May 29, 2019. It is now read-only.

feat(popover): created popover-template directive#369

Closed
joshdmiller wants to merge 1 commit into
angular-ui:masterfrom
joshdmiller:issue220
Closed

feat(popover): created popover-template directive#369
joshdmiller wants to merge 1 commit into
angular-ui:masterfrom
joshdmiller:issue220

Conversation

@joshdmiller

Copy link
Copy Markdown
Contributor

I haven't created any tests because I'm not a fan of the approach. I am submitting this PR to open a dialog to see if anyone can think of a better way to implement this.

Specifically, I created a new (internal, undocumented, but nonetheless exported) directive called ttLoadTemplateInSibling that loads a template from a remote source, compiles it relative to a new sibling scope, and replaces the HTML content of itself with said template. This directive is used in the new popoverTemplate directive's template to load the passed template where it needs to go.

I couldn't think of any other way to do this while still compiling the template in the right scope (a new sibling to avoid the isolate scope) and without introducing knowledge of the template to the directive.

@pkozlowski-opensource

Copy link
Copy Markdown
Member

@joshdmiller

Copy link
Copy Markdown
Contributor Author

@pkozlowski-opensource You're right about removing the scope. That was a blunder. :-)

I just don't like that I had to create another directive to make the templating clean. I can live with it though. If you're fine with it, I'll go ahead and write up some tests and add the scope destroy logic.

@lksv

lksv commented May 18, 2013

Copy link
Copy Markdown

@joshdmiller I try to use ng-model in popover but it seems to me that it does not work (after closing a popover and opening it again the binding is lost). http://plnkr.co/edit/cbqOnktHhxSjeLIBE1w7?p=preview

@jhiemer

jhiemer commented Jun 3, 2013

Copy link
Copy Markdown

I am having the same struggles related to the scope atm. Does anyone here have a working plnkr with scope being transferred from the popover to the parent?

@GabrielDelepine

Copy link
Copy Markdown

I include the @joshdmiller's modifications in ui-bootstrap-4.0.0 and it works well.

@jhiemer : I didn't try to update the parent scope. I don't need it !

Hope this merge will be include soon in the master.
Thanks

@jhiemer

jhiemer commented Jun 4, 2013

Copy link
Copy Markdown

@yappli are you doing any model manipulation in your popover?

@GabrielDelepine

Copy link
Copy Markdown

@jhiemer It seems to be OK for me to call a function in the parent scope but ONLY the first time the popover be display. From the popover be hided and deleted from the DOM, the link is broken.

In my parent scope function, I can modify variables, but if I put a $WATCH on this variables an exception "$digest already in progress" is throwed.

@tomsdev

tomsdev commented Jun 21, 2013

Copy link
Copy Markdown

@joshdmiller Do you need help to complete this PR?

@ghost

ghost commented Jul 21, 2013

Copy link
Copy Markdown

@joshdmiller I am having the same issue as @Iksv, on re-opening the datepicker the binding is lost and the page is refreshed, so ng-model binding of all other controls is also lost

@ihcnet

ihcnet commented Jul 21, 2013

Copy link
Copy Markdown

@algosystech @lksv check out the comment from @johnobe here #220 (comment). It should resolve the issue of the binding being lost when re-opening the datepicker.

@jbruni

jbruni commented Sep 20, 2013

Copy link
Copy Markdown
Contributor

Hi, @joshdmiller, @pkozlowski-opensource and all... I've arrived at issue #220 and PR #369 today only wanting a line-break or paragraph inside my popover - and nothing else. :-)

I've gradually went deep in the issue. In the end, I've read the whole history carefully, and also this PR code.

Please, take a look at the PR I've just opened - #1046 - I have commented some lines of the code.

To see whole PR patch in a glance I've created this gist - https://gist.github.com/jbruni/6633055

The new directives are very simple. The main challenge was to access the correct scope to compile the partial. I've added just a single line to tooltip.js to make the proper scope available to the popup directive through an attribute.

This way, there is no need of the additional directive + scope + sibling relationship - and I think there are no side-effects also.

The popup directive template is almost identical to the normal popover one. I've preferred to keep the title.

Finally, the somewhat unrelated issue with ng-model/binding became the real hard work... the most challenging... and my one-liner intervention in tooltip.js was not possible anymore... I've explained more about it in a PR code comment. Basically, we needed to "detach" instead of "remove" the tooltip from the DOM. But it was not that simple - due to the mentioned memory leak issue. I think everything has been properly addressed. Please, review.

Thanks.

@wcandillon

Copy link
Copy Markdown

+1
I was surprised to not find symmetry between the way modals and popovers work.

@pkozlowski-opensource

Copy link
Copy Markdown
Member

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants