Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fixes tooltip insert problems #6378

Merged
merged 8 commits into from
Dec 25, 2012
Merged

fixes tooltip insert problems #6378

merged 8 commits into from
Dec 25, 2012

Conversation

Yohn
Copy link
Contributor

@Yohn Yohn commented Dec 23, 2012

This adds a container option to the tooltip that can be called via html5 - data-container="body" or javascript - $().tooltip({container: 'body'})

if no container is given then it will get the default insertAfter() strategy

This fixes a number of bugs that were introduced when we changed the appendTo() to insertAfter()

If the tooltips are within a modal, and are inheriting its parents styles, then use $().tooltip({container: '#modalID'})

thanks to @WilliamStam and @blakeembrey for helping with this solution

some issues that noticed this bug
#5687 #6254 #6314 #6283 #6275 #6250 #6240 #6236 along with a lot more..

this is another resubmit of this.. previous pull request - #6321 with some tweaks to work with the 2.3 branch.

@@ -126,7 +126,8 @@
$tip
.detach()
.css({ top: 0, left: 0, display: 'block' })
.insertAfter(this.$element)

this.options.container && $tip.appendTo(this.options.container).length || $tip.insertAfter(this.$element)
Copy link
Member

Choose a reason for hiding this comment

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

just make this a ternary:

this.options.container ? $tip.appendTo(this.options.container) : $tip.insertAfter(this.$element)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added the .length there to make sure it got inserted in case the element wasnt on the page, and if not then it would still get the default insertAfter() otherwise it wouldnt show up if the container wasnt there

I'll make your change as you suggested though

Copy link
Member

Choose a reason for hiding this comment

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

yeah i understood what it was doing, but it's too "magical"… better to fail if the intended container is missing

updated it to @fat's suggestion
<td>string</td>
<td>''</td>
<td>
<p>Usefull if your tooltip is within a btn-group or modal</p>
Copy link
Member

Choose a reason for hiding this comment

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

i actually disagree with this. Tooltip will break if inserted in the body triggered on a modal because of z-index issues. Just remove this line.

@Yohn
Copy link
Contributor Author

Yohn commented Dec 25, 2012

@fat appreciate your support for this, and the comments. glad to know this has a possibility to be added

fat added a commit that referenced this pull request Dec 25, 2012
fixes tooltip insert problems
@fat fat merged commit b9c7f29 into twbs:2.3.0-wip Dec 25, 2012
@fat
Copy link
Member

fat commented Dec 25, 2012

yeah, looks great. thanks man!

@Yohn
Copy link
Contributor Author

Yohn commented Dec 25, 2012

no problem glad to have it in finally :)

@superflausch
Copy link

Hope it is okay to ask this directly here. How can I append the tooltip/popover directly into the triggering element, with this new 'container' possibility, without using an unique ID? In my specific case I have a list (UL) with a couple of LI>A children, which trigger the popovers. And because I wan't to be able to hover also over the popovers themselves I need them nested within my trigger element.

Cheers.

@Yohn
Copy link
Contributor Author

Yohn commented Jan 14, 2013

@superflausch I was trying to come up with a solution with this request, but couldnt get one working corrently without the use of ids..
but blake made another pull request shortly after this one got merged with an option to have the container a function to be able to do this easily -> #6412 his update to the container makes this possibe

@superflausch
Copy link

Missed that one. Thanks for the hint, will add this. Cheers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants