. #1730

Closed
powder96 opened this Issue Feb 5, 2012 · 28 comments

Comments

Projects
None yet

powder96 commented Feb 5, 2012

No description provided.

KenanY commented Feb 5, 2012

👍

Contributor

olvlvl commented Feb 5, 2012

In my implementation I use a .fit-content class that sets the width of the .popover-inner element to auto.

http://brickrouge.org/#popover-anchor-options

Contributor

olvlvl commented Feb 5, 2012

@powder96 Thanks !

klaemo commented Feb 5, 2012

I would love to see more options for the popovers as well. Like being able to disable the title. :)

Owner

mdo commented Feb 13, 2012

I can see us enabling this, making popovers more flexible and durable. I'll talk with @fat.

drewda commented Feb 13, 2012

Thanks, @markdotto.

Another option: just let us specify an id and/or class for popovers (and for tooltips, 'cause I guess that underlies the popover implementation) and then we can provide our own styling attributes like width and height.

Owner

fat commented Feb 13, 2012

how is this not just a tooltip? Particularly in the example above, i lol'd.

drewda commented Feb 13, 2012

Yeah, the example above is definitely in tooltip territory.

A different reason for wanting to customize the popover styling is to do things like iOS-style popover menus: example

klaemo commented Feb 13, 2012

I think the behavoir should be similiar to that of the modals. You lay out the structure of the popover/popover-menu in the markup and trigger them via js. That way they are persistent and don't get removed from the page everytime they aren't visible.
I built something like it with the current implementation, but it would be nice to have it natively. A another trigger mode 'click' would probably be necessary.

@ghost

ghost commented Feb 22, 2012

@klaemo You can specify a custom popover template using the 'template:' option. But that's not quite as easy as just having the title area disappear if there is no title.

@ghost

ghost commented Feb 23, 2012

My take on this is to have a data attribute that would set a custom class in the first div of the popover (and maybe even do it in the tooltip js file).

so like <a class="btn" data-custom-class="myClass" data-original-content="look at this content">hover me

Then the popover would start with <div class="popover myClass"><div class="popover-inner">etc...

pehrlich commented Mar 6, 2012

I can definitely put in a vote for this. It would be great to be able to specify a custom classname to be used by a popover..! Popovers are not appended to their triggering elements but instead directly to the page body, thus currently the only way to do custom styling is universally.

Owner

mdo commented Apr 1, 2012

Not that we'll be going with this pull request, but just wanted to link these two together: #2332.

trante commented Apr 28, 2012

👍
But my vote is for "fitting popover content" into its window autmatically.

akoprow commented May 21, 2012

👍 both for window size automatically fitting the content & ability to style individual popovers.

Blizzke commented Jun 12, 2012

👍

flynfish commented Jul 3, 2012

👍

LekeFly commented Jul 16, 2012

👍 on this and removing all html inside.. like title etc:)

Owner

fat commented Jul 22, 2012

Tagging this issue as popular, please stop commenting on this issue with +1. thanks!

Contributor

lookfirst commented Aug 27, 2012

If this is popular, why is it still open for 7 months?

Owner

mdo commented Aug 27, 2012

Popularity doesn't inherently mean we'll prioritize to add something. It's simply a way for us to organize issues and tackle them as we can. Improved popovers would be nice, but bugs and larger themes (e.g., our broke as hell modals) are more important to us in the long term. It's just a matter of time, energy, and everything else that goes into this.

If you want it sooner, talk to @fat to come up with the right solution, then submit a pull request. Otherwise, we'll get to it when we can.

Contributor

lookfirst commented Aug 27, 2012

@markdotto, @fat just closed my absolute modal pull request, so there you go. =(

people have proposed solutions... which are two lines of code and would solve the problem just fine, yet those solutions get closed out with @fat comments like 'just do this in css', which is clearly impossible. https://github.com/twitter/bootstrap/pull/2332/files

sometimes I think you guys make this project a lot more difficult than it needs to be.

Owner

fat commented Aug 27, 2012

i closed your pull request because it had no unit tests. beyond that, looking at it now, the code was poor. it would not have been merged.

we try not to set css values explicitly in js, which is why the pull request you are referring to was closed.

this project is what it is because we make it difficult.

Contributor

lookfirst commented Aug 27, 2012

The existing modal code is poor (or as @markdotto said above... broke as hell). So, if my code, which fixes the issues in the existing code, is also poor, what does that add up to?

If you have suggestions on how to improve the modals, let me know in the comments and give me a chance to fix things to comply with your coding standards. Don't just close the pull request with no comment, that's just plain rude.

This project is great. Thank you so much for it. I rely heavily on it and therefore I want to contribute to it. I'm also glad you have good standards for it. I'm not trying to discourage that at all.

That said, since you do have willing people to help you out, try a bit harder to encourage that so that issues like this one just don't sit around for 7 months. Spend two minutes describing what a valid pull request would look like to fix the issue and let someone else try to code it up.

Contributor

andriijas commented Sep 10, 2012

Im willing to give this one a shoot with proper documentation, test and everything required for a proper pull request to be merged in if I can get any directions in how this issue should be solved to fit bootstrap the best.

Cheers.

Owner

mdo commented Oct 1, 2012

@andriijas If you're still game, let's change up the popovers a bit:

  • By default, they should be flexible to any content placed within them
  • We should have some smart max-width or max-height options on them as well though
  • Then, we could perhaps add fixed sizes to them as well (.popover-large, for example)

Thoughts?

Contributor

andriijas commented Oct 2, 2012

Very nice ideas @markdotto

I tossed together the small, medium, large classes and made a quick example:

http://jsbin.com/ecojob/1/edit (commit: andriijas/bootstrap@4b79e99)

Im not sure which would be the best way to add the sizes on the popovers but probably via data attributes? Didnt have time to fiddle with the js now, to make it more complex the popovers are inherited from tooltips.

There are tons of quirks Ive ran into, mostly related "responsive popovers".

@andriijas @markdotto The custom classes would be helpful, but I also think it would be helpful if we could pass a custom class or id that is added to the popover. This would allow styling individual popovers and would give them much greater flexibility. @drewda and others have requested the same thing in the comments above.

Would passing a custom class/id be considered if I or someone else were to implement it?

Edit:
I see we could pass a new template, but that option is

  • more fragile (if default template changes in the future, you would have to go back and update all of the custom ones)
  • seems a bit hacky for just needing a custom class/id
Contributor

andriijas commented Oct 24, 2012

@flynfish feel free to try to get a pull request in shape because I dont have a lot of time at the moment :/

Owner

mdo commented Dec 28, 2012

For now, I've addressed this in #6346 (v2.3) via 1c0e4fc.

@mdo mdo closed this Dec 28, 2012

@cvrebert cvrebert locked and limited conversation to collaborators Jan 19, 2015

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