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

fix(tooltip): Transcludes contents to allow dynamic selector text - #72

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

joshdmiller wants to merge 1 commit into
angular-ui:masterfrom
joshdmiller:tooltip-dynamic

Conversation

@joshdmiller

Copy link
Copy Markdown
Contributor

A dynamic selector did not work as the contents of the directive element
were not transcluded. This fix adds transclusion through a new directive
controller method. It also includes a new test for this case (through an
ngRepeat) as well as a modified demo to show this use case.

My apologies for the earlier PR I closed. The bug fix was correct, but the test was useless. This new PR contains the better test.

Closes #69

A dynamic selector did not work as the contents of the directive element
were not transcluded. This fix adds transclusion through a new directive
controller method. It also includes a new test for this case (through an
ngRepeat) as well as a modified demo to show this use case.

Closes angular-ui#69
@pkozlowski-opensource

Copy link
Copy Markdown
Member

Landed as ab44a86. Had a look at the fix and wouldn't think of this :-)
I'm starting to wonder if we are not making it more complex than needed... but at the same time don't have any idea of simplifying things... so just a gut feeling at the moment.

@petebacondarwin

Copy link
Copy Markdown
Member

Isn't it easier just to use

{
transclude: true,
...
}

in the directive definition and then

var template = 
    '<tooltip-popup ng-transclude></tooltip-popup>';

which is basically what this new directive controller does anyway?

@joshdmiller

Copy link
Copy Markdown
Contributor Author

@petebacondarwin Actually, it's not transcluded into the tooltip popup; it's transcluded into the element on which the tooltip directive is applied. The tooltip-popup is only in the DOM on mouseover. The issue I was trying to solve was transclusion on an element that had no template (i.e. the tooltip directive).

I agree with @pkozlowski-opensource that it feels overly-complicated. I also considered removing the isolate scope, but then I would have to include the $watch logic myself, so complexity increased either way, so I opted for code security complexity instead.

But I'm sure there's a better way to do this.

@lanterndev

Copy link
Copy Markdown
Contributor

Thanks for getting a fix for this in so fast! Would you mind pushing it to https://github.com/angular-ui/bootstrap/blob/gh-pages/ui-bootstrap-tpls-0.1.0-SNAPSHOT.js too?

(What's the difference between that version and the one without tpls, by the way? Couldn't find that documented, sorry if I missed it.)

@pkozlowski-opensource

Copy link
Copy Markdown
Member

@Skivvies just pushed demo & build updates to the gh-pages branch. You can grab updated files from http://angular-ui.github.com/bootstrap/ui-bootstrap-tpls-0.1.0-SNAPSHOT.min.js

As for the file naming we've got:

  • ui-bootstrap-[version].js
  • ui-bootstrap-tpls-[version].js
  • ui-bootstrap-[version].min.js
  • ui-bootstrap-tpls-[version].min.js

Files with the min suffix are minified versions to be used in production (self-explaining). Then we've got files with the -tpls. To explain the difference let's start by explaining that we want our directives to be highly customizable. To achieve this we are trying hard not to hard-code markup nor CSS inside JavaScript code. Instead most of the directives are coming with a dedicated template(s) so people can change them. We are providing default templates based on Twitter's markup and CSS but you should be able to prepare your own partials to change the look&feel of widgets from this repo.

Now it should be clear that files with the -tpls in their name have bootstrap-specific templates bundled with directives. For people who don't want to take all the directives and don't need to customize anything the solution is to grab a file named ui-bootstrap-tpls-[version].min.js. If, on the other hand default templates are not what you need you could take ui-bootstrap-[version].min.js and provide your own templates, taking the default ones (https://github.com/angular-ui/bootstrap/tree/master/template) as a starting point.

@lanterndev

Copy link
Copy Markdown
Contributor

Thanks @pkozlowski-opensource! Saw you added this info to the README in the gh-pages branch, nice.

I also saw you pushed the fix in ab61230, but my plunkr (which points to http://angular-ui.github.com/bootstrap/ui-bootstrap-tpls-0.1.0-SNAPSHOT.min.js) doesn't seem to have been fixed by it, hm.

@joshdmiller

Copy link
Copy Markdown
Contributor Author

@Skivvies The fix is in the gh-pages branch, but has not yet been pulled by Github Pages. This should fix itself pretty soon.

@pkozlowski-opensource

Copy link
Copy Markdown
Member

@joshdmiller Normaly pages are built just seconds after the push... Maybe a cash refresh problem?

@joshdmiller

Copy link
Copy Markdown
Contributor Author

@pkozlowski-opensource Indeed you're right. I wonder why Github is telling the browser the file is unchanged...

@petebacondarwin

Copy link
Copy Markdown
Member

@joshdmiller Thanks for that. I understand now.
This raises another question. Why is the tooltip using isolate scope in the first place. It is not needed since we are not using a template for it. We should be manually interpolating and parsing the attributes and leaving the scope alone. Then you do not need to worry about transclusion at all.

@joshdmiller

Copy link
Copy Markdown
Contributor Author

@petebacondarwin We are using a template, but just not for the primary directive. Right now, the popup does not have an isolate scope while the tooltip does; we could swap this, removing the isolate scope from the tooltip and therefore also the need to worry about transclusion, and add an isolate scope to the popup directive. It increases complexity, but it's probably more "correct". What do you think?

@lanterndev

Copy link
Copy Markdown
Contributor

I just tried the latest version with the example in http://plnkr.co/edit/1GiwHv locally and it's still broken for me. Would anyone be able to check if #75 is actually a duplicate of this issue, and if not, reopen?

@joshdmiller

Copy link
Copy Markdown
Contributor Author

@Skivvies There is a typo in your Plunker. On line 17, you use "path" as the expression when it should be "url". Here's a corrected version: http://plnkr.co/edit/PNpdIg?p=preview.

@lanterndev

Copy link
Copy Markdown
Contributor

Oops, sorry for sending you a bad version! Here's the version I meant to send:
http://plnkr.co/edit/1GiwHv
That version demonstrates there's still a problem when you give a tooltip to a link whose href is bound to a value that is initially falsey: even after the value changes to something truthy, the href attribute stays set to nothing.

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.

Tooltip does not work inside repeat with dynamic value

4 participants