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

Add popover directive #113

Closed
wants to merge 1 commit into from
Closed

Add popover directive #113

wants to merge 1 commit into from

Conversation

ballmw
Copy link
Contributor

@ballmw ballmw commented Jan 31, 2013

Hello, I've built the popover directive for my use. I've never done a pull request or anything so excuse me. The tests are written and pass, demo is set up. Maybe someone else can get some use out of it...

popover

  • Mike

@ghost ghost assigned joshdmiller Jan 31, 2013
@pkozlowski-opensource
Copy link
Member

@ballmw great stuff, especially if this is a first pull request! I don't know if you've followed our discussion in #99 but we were pondering different option for the directive's syntax. I can see that you've decided to go with a "tooltip flavor".

@joshdmiller Josh, would you mind reviewing it? I will pre-merge it later today to a branch to see if tests are passing on all the browsers.

If this looks good I would be for merging it. This would be opportunity to have people's reaction to this API.

@ballmw, once again, thnx so much for taking time to prepare this PR!

@joshdmiller
Copy link
Contributor

@ballmw Nice - thanks for submitting this!

From what I can see, this is the same as the tooltip, but with a new template and a default "click" trigger. I can even see the tt_ prefix on the scope vars still... :-)

Referring to #99, if we're looking to start with the tooltip flavor, then this is great - it's essentially what I would have done too.

One concern - like the tooltip, no markup is allowed in the popover. This is probably more of an issue with a popover because it's bigger. We could change the binding in the template to ngBindHtml, but I'm not sure how I feel about that. In the view, someone would have to:

<a popover-title="Title" popover-content="This is <b>bold</b>.">Click me</a>

which is a tad strange. But I guess that's the argument for custom templates. What you think?

Anyway, if we want to start with something basic, I'd be for merging this too - it gets the job done! The only thing I would suggest in that case (and this is my anal-retentiveness coming up for air) is that we clean up the variable names a skosh, cutting any leftover references to the tooltip in the directive and in the specs.

@ballmw - Again, nice work!

@pkozlowski-opensource
Copy link
Member

@joshdmiller Regarding HTML in attributes I had a chat with @petebacondarwin (in #67) and I agree that I feel uneasy about HTML in attributes. On top of this ngBindHtml requires linky module so this is not cool either. I think that, as a general rule, we should try to avoid HTML in attributes.

Going back to this PR: I would be for merging this one and working on 2 items:

  • removing code duplication with tooltip
  • adding support for popovers as a template

What do you guys think?

@ballmw
Copy link
Contributor Author

ballmw commented Jan 31, 2013

Yes I took the tooltip code and modified it for the popover. It didn't even occur to me that tt_ meant tooltip. I'm really about how to reuse code among directives and implementing custom templates.

I really appreciate all the work that's gone into this project...

@joshdmiller
Copy link
Contributor

Groovy!

After the merge, we can hop back over to #99 and figure out how we want to tactically address those next steps.

@pkozlowski-opensource
Copy link
Member

Landed as 4b5c067. We are going to continue work on the popover (code duplication refactor, popover from a template) in #99

Thnx!

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.

3 participants