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 arrow fixes #13718

Closed
wants to merge 2 commits into from
Closed

Conversation

peterjwest
Copy link

This fixes two bugs in the tooltips plugin:

  • The tooltip arrow orientation was chosen using the truthiness of the delta.left value returned by getCalculatedOffset, however there are cases when this value is 0, but the placement of the plugin is still top or bottom, which means the arrow should be positioned using the left property. This fix uses the existing placement variable (fixes Tooltip arrow not positioned correctly when offset is adjusted and placement is top or bottom #13696)
  • If the page layout changes after the tooltip has been shown, the tooltip orientation could change, in which case the alternate CSS property was never reset (couldn't find an existing issue for this)

@cvrebert cvrebert added the js label Jun 3, 2014
@cvrebert cvrebert added this to the v3.2.1 milestone Jun 3, 2014
@cvrebert
Copy link
Collaborator

cvrebert commented Jun 3, 2014

Perhaps something like isVertical & isHorizontal might be better than topBottom & rightLeft?

this.arrow().css(position, delta ? (50 * (1 - delta / dimension) + '%') : '')
Tooltip.prototype.replaceArrow = function (delta, dimension, rightLeft) {
this.arrow()
.css(rightLeft ? 'left' : 'top', 50 * (1 - delta / dimension) + '%')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why'd you remove the delta-related ternary?

Copy link
Author

Choose a reason for hiding this comment

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

Because if delta is zero it removes the css value, which resets it to 50%, but if delta is zero it will always be 50% anyway.

@peterjwest
Copy link
Author

how about isPlacedVertically and isPlacedHorizontally? It's a bit more wordy but vertical and horizontal can mean a lot of different things out of context.

@cheesypoof
Copy link

This also appears to fix #13384.
👍

@fat
Copy link
Member

fat commented Jun 7, 2014

makes sense to me. (also i'm a fan of is prefixed vars).

@peterjwest do you think you could a unit test to make sure we dont see any regressions around this fix? Would be really great to get one

@peterjwest
Copy link
Author

Would be happy to, I'm out of the country for a week though.

@fat
Copy link
Member

fat commented Jun 8, 2014

cool, no rush. thanks! :)

@hnrch02
Copy link
Collaborator

hnrch02 commented Jun 30, 2014

@peterjwest Any updates on this?

@peterjwest
Copy link
Author

Sorry, I'll get on this ASAP :)

@hnrch02
Copy link
Collaborator

hnrch02 commented Jun 30, 2014

@peterjwest Nice, thanks for the great work!

@cvrebert
Copy link
Collaborator

@peterjwest Friendly ping on this.

@anotherlizwong
Copy link

These changes fixed my issue with tooltip arrow placement on wrapped labels, thank you!

@sebnuss
Copy link

sebnuss commented Aug 29, 2014

This also appears to solve #13696
Thanks

@Arkni
Copy link

Arkni commented Sep 2, 2014

Hi
Did this patch solve also the Popover arrow issue ?

@hnrch02
Copy link
Collaborator

hnrch02 commented Sep 2, 2014

@Arkni Presumably yes, as popovers are basically just fancy tooltips. The functions relevant to arrow placement don't differ between the popover plugin and the tooltip plugin. See also this fiddle.

@peterjwest
Copy link
Author

Yes it should if it's the same issue, I created it because I had an issue with popovers :)

@Arkni
Copy link

Arkni commented Sep 2, 2014

@hnrch02 @peterjwest Thanks a lot, it works perfectly 👍

@hnrch02 hnrch02 closed this in 4c98507 Sep 13, 2014
@cvrebert cvrebert mentioned this pull request Sep 13, 2014
Saranya-Raaj pushed a commit to Saranya-Raaj/bootstrap that referenced this pull request Oct 9, 2014
@HybridSolutions
Copy link

Thanks for this! Solves the problem!

@twbs twbs locked and limited conversation to collaborators Oct 15, 2014
@cvrebert cvrebert removed their assignment Apr 26, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tooltip arrow not positioned correctly when offset is adjusted and placement is top or bottom
9 participants