Skip to content
This repository has been archived by the owner on May 29, 2019. It is now read-only.

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

Closed
wants to merge 1 commit into from

Conversation

joshdmiller
Copy link
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
Member

@joshdmiller I just quickly went through the code and it doesn't look bad, really. What was the thing you didn't like in this approach?

@joshdmiller
Copy link
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
Copy link

lksv commented May 18, 2013

@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
Copy link

jhiemer commented Jun 3, 2013

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

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
Copy link

jhiemer commented Jun 4, 2013

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

@GabrielDelepine
Copy link

@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
Copy link

tomsdev commented Jun 21, 2013

@joshdmiller Do you need help to complete this PR?

@ghost
Copy link

ghost commented Jul 21, 2013

@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
Copy link

ihcnet commented Jul 21, 2013

@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
Copy link
Contributor

jbruni commented Sep 20, 2013

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

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

@pkozlowski-opensource
Copy link
Member

@joshdmiller it doesn't merge cleanly any more and there is an alternative PR so I'm going to close this one for now. I would love to come back to this one, though.

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