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

"popoverTemplate" and "popoverTemplatePopup" directives #1046

Closed
wants to merge 6 commits into from
Closed

"popoverTemplate" and "popoverTemplatePopup" directives #1046

wants to merge 6 commits into from

Conversation

jbruni
Copy link
Contributor

@jbruni jbruni commented Sep 20, 2013

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

Improved over @joshdmiller PR #369

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

@@ -102,6 +102,7 @@ angular.module( 'ui.bootstrap.tooltip', [ 'ui.bootstrap.position', 'ui.bootstrap
'placement="'+startSym+'tt_placement'+endSym+'" '+
'animation="tt_animation()" '+
'is-open="tt_isOpen"'+
'compile-scope="$parent"'+
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this single and simple line, we are able to provide the "compile scope" to the "popup" directive.

} else {
tooltip.remove();
angular.forEach( tooltip, function( e ) { e.parentNode.removeChild( e ) } );
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is similar to jQuery's "detach" - which is what we need here instead of "remove", so we don't break the ng-model bindings.

tooltip.remove();
} else {
angular.forEach( tooltip, function( e ) { e.parentNode.removeChild( e ); } );
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • There is a difference between "detach" and "remove".
  • jqLite does not have "detach", only "remove".
  • This forEach loop above emulates the detach behaviour.
  • Sometimes we need to call remove - it is when the scope is destroyed (see fix(tooltip): make sure tooltip scope is evicted from cache. #486).
  • But by calling "remove" and clearing all data and bindings from the compiled DOM - only to hide the tooltip, without effectively needing to do it - we had the ng-model binding issue.
  • Now, we selectively call "remove" or the "detach" equivalent. The "remove" function is effectively called only when needed (i.e., when the scope is actually being destroyed).

Removed jQuery dependency
Accepts external file templates
@jbruni
Copy link
Contributor Author

jbruni commented Oct 7, 2013

Developers at #220 have tested the directive - we've found:

  • it required jQuery
  • it required AngularJS 1.2
  • it required templates declared within script tags

All these deficiencies were treated, resulting in a much more mature implementation.

As it is, I think it may be good to merge now.

Of course, if major/deep changes in popover/tooltip are on the horizon, this PR is past its time. Otherwise, if a major refactor is for a future milestone, I don't see why not include this expected feature as it is asap.

@joshdmiller - can you review it? It is based in your PR #369

Thank you.

Removed jQuery dependency
Accepts external file template
@jbruni
Copy link
Contributor Author

jbruni commented Dec 5, 2013

Need to update this PR... better yet, just close this one and open another... it became obsolete with the launch of 0.7.0... everyday someone else appears at #220 wanting the feature to be official... I must ask:

  • Is there a complete refactoring of tooltip / popover happening in the short-term?

If yes then I understand this effort is merely a temporary workaround.

If not then why not merge it? (Considering a "new PR" for 0.7.0)

Hmmm... now I see that what I am really asking is if it worths the effort to create a new PR for 0.7.0 - it certainly will be cleaner than this one, since the solution is mature now (i.e., this PR has a history of commits, while the new one should contain a single commit).

Thanks.

@jbruni
Copy link
Contributor Author

jbruni commented Dec 11, 2013

Closing this PR because I've just opened a new updated one: #1391 - based on new Angular UI Bootstrap version 0.7.0

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.

1 participant