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

tooltip('toggle') fixed, also added container option for where the tooltip gets placed #5768

Closed
wants to merge 7 commits into from

Conversation

Yohn
Copy link
Contributor

@Yohn Yohn commented Nov 2, 2012

This is an old pull request

Both of these bugs have been fixed in 2.3.0

please refer to the following requests--
#6321 to fix the insert strategy and use a container option
#6176 fixes the tooltip('toggle') feature which is currently broken.


refer to #5753

.tooltip('toggle') and .popover('toggle') was broken, so heres the fix

took me forever to get this request to send right, but I think I got it this time

thanks to @demike for helping correct the problem

Also, within the pull request is an alternate way of doing the insertAfter or appendTo options noted within #5830 to fix the problems within #5687

I found out after I changed my -wip branch that it updates this pull requests with my changes, thats why they're in the same pull

How to use the new container option..
you can use html5 elements -- data-container="body" or with javascript - $().tooltip({ container: 'body' })
and the container value, can be any element on the page, and for tooltips within modals I've been using container: '.modal' or '.modal-body'

@Yohn
Copy link
Contributor Author

Yohn commented Nov 2, 2012

damn travis.. I'll see if I can figure out what broke there

Update..
I'm finding that the following is failing with my pull request

      test("should show tooltip with delegate selector on click", function () {
        var div = $('<div><a href="#" rel="tooltip" title="Another tooltip"></a></div>')
        var tooltip = div.appendTo('#qunit-fixture')
                         .tooltip({ selector: 'a[rel=tooltip]',
                                    trigger: 'click' })
        div.find('a').trigger('click')
        ok($(".tooltip").is('.fade.in'), 'tooltip is faded in')
      })

and then with some testing on my own with a fiddle it seems that somehow the selector option broke when you do something like $('body').tooltip({selector: 'a[rel=tooltip2]', trigger: 'click'}) but $('a[rel=tooltip3]').tooltip({trigger: 'click'}) works on click

not sure how or why, or how to fix this one.. but everything else seems to be working

@demike helped fix this so it would pass the unit tests
Yohn added 2 commits November 7, 2012 21:44
alternate way to go about tooltip container options from pull request twbs#5830 fixing problems noted within twbs#5687
added the unit tests from @WilliamStam's pull request twbs#5830
Yohn added 2 commits November 8, 2012 03:28
put container variable where it belongs, and made it only call to the dom when container option is found
@patrickmcelwee
Copy link

thanks ... overriding the toggle fixed popovers for me (I extend the popover functionality with Bootstrapx Clickover: https://github.com/lecar-red/bootstrapx-clickover)

@joe1chen
Copy link

+1 for this pull request.

@jschr
Copy link

jschr commented Nov 13, 2012

+1, this is exactly what I did in my application to fix popovers since there tend to be a lot of modal/tab controls I couldn't have every popover appended to body.

Appending to a specified container also allowed me to do this in the Tooltip class:

this.$container.on('hidden', $.proxy(this.hide, this));

Which will hide any tooltips/popovers when a modal is closed.

@Yohn
Copy link
Contributor Author

Yohn commented Nov 13, 2012

@jschr thats one way to do it, for modals I've been doing data-container=".modal-body" to get it to appened within the modal to stay on top, and then when its closed it closes as well since its within the modal

@jschr
Copy link

jschr commented Nov 13, 2012

@Yohn Yes but only if you use the hover trigger wouldn't it? If you use popovers with a manual or click trigger they will stay open if you reopen the modal. I had the same issue with tabs and accordions as well.

@Yohn
Copy link
Contributor Author

Yohn commented Nov 13, 2012

It should work with the click or manual trigger, but I dont remember trying it yet..
I'm about to head out for a little while so I'll check it out when i get back and let ya know

@Yohn
Copy link
Contributor Author

Yohn commented Nov 14, 2012

I see that youre talking with the popovers staying up on reload of the modal..
I guess I should add it with this pull request since it adds more functionality to the container options, or should I wait till we hear from @markdotto or @fat if this will make it onto the 2.2.2 milestone?

@nozpheratu
Copy link

Good looking out, hopefully this makes it for the next build. 👍

@blakeembrey
Copy link
Contributor

Would be much easier and better to just do something like:

$tip.appendTo(this.options.container || this.$element.parent())

Of course, if insertAfter is preferred then use that - but why create a jQuery object when it isn't required. Especially since the appendTo method accepts a string, element, jQuery object and selector out of the box - so why complicate things.

@Yohn
Copy link
Contributor Author

Yohn commented Dec 6, 2012

@blakeembrey I wanted to keep the same kind of idea they had for the tooltips with insertAfter but you did bring up a valid point with creating an object for the container when its not needed.. my main computer is down right now, so I'm trying to reinstall everything so once thats completed I'll try to modify this request

@blakeembrey
Copy link
Contributor

@Yohn I understand that - but I think the insertAfter method is flawed. For one, it doesn't seem work at all inside button groups because as the element is inserted between buttons it creates a small (about 1px) gap between the buttons. I tried every CSS hack in the book to remove it to, but to no avail. Plus it can break a lot more things, such as code that is relying on adjacent selectors.

Also, could you squash the commits after you do the latest commit? Just so I can see all the changes (no sure if I missed something over the four or so commits). Thanks

@Yohn
Copy link
Contributor Author

Yohn commented Dec 8, 2012

my main computer is still down, I think I gotta put in a new heat pad, so until I do that I cant use git to try to send a request in.. I can edit the files on github and send em that way..

I'm not sure how to squash commits either, so that makes me want to open another request, and close this one out cause I can see that this one it would be complicated to figure out what was changed.. and theres 2 different bug fixes within this one, the insert strategy of the tooltips and tooltip('toggle')

@mdo mdo closed this Dec 9, 2012
Yohn added a commit to Yohn/bootstrap that referenced this pull request Dec 9, 2012
tooltip('toggle') is broken in current bootstrap download -- http://jsfiddle.net/SqFbx/
and with the changes it works -- http://jsfiddle.net/SqFbx/1/

thanks to @demike for helping fix this issue.

This is also part 1 of a pull request that was closed - twbs#5768
@Yohn Yohn mentioned this pull request Dec 9, 2012
@Yohn
Copy link
Contributor Author

Yohn commented Dec 10, 2012

@blakeembrey I'm playing with this request again to submit it, and came up with a potential problem and was looking for your advice possibly, maybe someone elses?

anyways, how I did it in this request was to see if the container was a valid container in the source with

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

so what thats doing is seeing if there is a container option, and then creating an unneeded object for that container to check its length / if its in the source.. If its in the source then the tooltip is appended to that container, and If its not then it gets inserted after the element..

I believe that it needs to check if the container is there because if its not then the tooltip will not appear, would there be a different way to go about checking if the container is in the source? or would I have to create that object for it to make sure its valid?

@blakeembrey
Copy link
Contributor

I'm not sure about checking if the container exists before inserting it. The element should be up to the developer to implement. If you really wanted this feature though, you could do

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

Still not 100% sure about using insertAfter and would love some official word on this as it breaks button groups and other inline-block hacks. Even with this patch, the easiest way around it is to add an ID or something to the parent (hardly useful).

@Yohn
Copy link
Contributor Author

Yohn commented Dec 10, 2012

yeah, I really dont like using insertAfter() as well.. would also love to get the official word from @fat or @mdo about using insertAfter() vs appendTo() I know it fixes the modal z-index issue, but it creates a bunch of other bugs on top of it.. and having the dev use a container seems reasonable to fix this problem

I like the idea of putting the .length on the appendTo() and might use it

I've been playing with a fiddle - http://jsfiddle.net/R23KB/1/ trying to come up with something, and realized another bug with tooltips within the modals.. they seem to get cut off it it goes over the edge of .modal-body, on all the sides.. if its at the top or bottom it gets cut off by the modal-header and modal-footer

@blakeembrey
Copy link
Contributor

@Yohn Yeah, that's my fault. Need to upgrade the tooltip to stop using overflow: auto;. Was a result of tooltips and popovers not following the element on scroll. Would love to fix this as well - but would mean the modal goes to either a hybrid absolute/fixed positioning or something similar.

@Yohn Yohn mentioned this pull request Dec 12, 2012
@Yohn
Copy link
Contributor Author

Yohn commented Dec 12, 2012

@blakeembrey I just figured out why it was going over the edges there.. it was because of setting the container to '.modal-body' if I would have set it to '.modal' instead it works fine with no overflow issue.. but then if the modal is longer and scrolled, the popover doesnt stay with the element that triggered it lol
heres an updated fiddle - http://jsfiddle.net/R23KB/3/

@Yohn Yohn mentioned this pull request Dec 12, 2012
marcellodesales pushed a commit to marcellodesales/svnedge-console that referenced this pull request Dec 16, 2013
* web-app/js/bootstrap-2.2.1.js
  tooltip toggle is not working. There is an issue filed:
  twbs/bootstrap#5753

  A pull request with this change is open:
  twbs/bootstrap#5768

  So hopefully, this will be fixed in next release, so we don't
  have to re-apply.


git-svn-id: https://ctf.open.collab.net/svn/repos/svnedge/trunk@3365 03e8f217-bfc6-4b7c-bcb7-0738c91e2c5f
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.

9 participants