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

Fix different tooltips offset when hovering #22276

Merged
merged 3 commits into from
Mar 28, 2017
Merged

Conversation

Johann-S
Copy link
Member

Fix different tooltips offset when hovering by resetting tip classes

/CC : @mdo and @bardiharborow

Close : #21146

@Johann-S
Copy link
Member Author

Johann-S commented Mar 26, 2017

The test failed on trying to stop the tunnel with SauceLabs
https://travis-ci.org/twbs/bootstrap/jobs/215178467#L431-L451

Squashed my commits and no more failed on Travis

@Johann-S Johann-S force-pushed the v4-tooltip-offset branch from f3de98a to 702d0cf Compare March 27, 2017 09:51
@@ -438,6 +439,15 @@ const Tooltip = (($) => {
return AttachmentMap[placement.toUpperCase()]
}

_cleanTipClass() {
Copy link
Contributor

Choose a reason for hiding this comment

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

The Popover class extends Tooltip class, so it's probably better to implement this method in Tooltip to handle only the tooltip case, and to override it in Popover to handle only the case of popover.
This is how it's implemented for other methods (for example isWithContent);

Copy link
Member Author

Choose a reason for hiding this comment

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

Finally I found a generic solution but thank you @vanduynslagerp 👍

Copy link
Contributor

@pvdlg pvdlg Mar 27, 2017

Choose a reason for hiding this comment

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

Using the CLASS_PREFIX is clever !
You can make this code shorter and more efficient thought:

// On top of the file (constant)
const TETHER_PREFIX_REGEX = new RegExp(`(^|\s)${CLASS_PREFIX}(\S*)`, 'g');

const $tip = $(this.getTipElement())
$tip.removeClass($tip.attr('class').match(TETHER_PREFIX_REGEX).join(' '));

That makes the code more readable and would avoid the multiple calls to Jquery to remove the classes one by one. It also avoid the loop and the assigning and reassigning of temporary variables multiple time.

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right nice regex thanks you @vanduynslagerp

_cleanTipClass() {
const $tip = $(this.getTipElement())
if (this.config.toggle === 'tooltip') {
$tip.attr('class', 'tooltip fade')
Copy link
Contributor

Choose a reason for hiding this comment

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

The tooltip template is customizable with the parameter template.
The class tooltip is not mandatory so the user might specify a different class. He can also specify additional classes.
In those cases this code with wipe the user's custom classes defined in his template.
Instead of wiping all the classes from the tooltip you should remove only the relevant ones.

Copy link
Member Author

Choose a reason for hiding this comment

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

good feedbacks @vanduynslagerp thank you 👍

@Johann-S Johann-S force-pushed the v4-tooltip-offset branch from 702d0cf to 8d7c070 Compare March 27, 2017 20:43
@Johann-S Johann-S force-pushed the v4-tooltip-offset branch from 8d7c070 to bf3a8a2 Compare March 27, 2017 20:48
@Johann-S Johann-S added this to the v4.0.0-beta milestone Mar 28, 2017
@Johann-S
Copy link
Member Author

I changed a bit your regex @vanduynslagerp but thank you it works very well 👍

@Johann-S Johann-S force-pushed the v4-tooltip-offset branch 2 times, most recently from 121626b to cddc851 Compare March 28, 2017 09:32
@Johann-S Johann-S force-pushed the v4-tooltip-offset branch from cddc851 to 0fb0ca8 Compare March 28, 2017 10:09
@Johann-S
Copy link
Member Author

This PR (#17094) tried to do the same thing but addTargetClasses only remove Tether class from the element which trigger the tooltip not the tooltip.

@pvdlg
Copy link
Contributor

pvdlg commented Mar 28, 2017

This PR (#17094) tried to do the same thing but addTargetClasses only remove Tether class from the element which trigger the tooltip not the tooltip.

I didn't check the Tether code, but is it possible that's it's actually a Tether bug ? It would be reasonable to think that when you activate the same element twice Tether should position it at the same position.
Or is the problem due to something Bootstrap does that prevent Tether to handle its own generated classes ?

@Johann-S
Copy link
Member Author

Johann-S commented Mar 28, 2017

Currently they are no way of cleaning the tooltip element in Tether and I didn't found any PR which target this issue on Tether repository.

Edit :
IMO it's due to Tether classes so until they don't fix this issue it's better to have this PR

@pvdlg
Copy link
Contributor

pvdlg commented Mar 28, 2017

Currently they are no way of cleaning the tooltip element in Tether and I didn't found any PR which target this issue on Tether repository.
IMO it's due to Tether classes so until they don't fix this issue it's better to have this PR

So in that case it might be better to open an issue with Tether and apply this PR if it's not fix fast enough? And we could remove this workaround once it's fixed ?

@Johann-S
Copy link
Member Author

Finally after a quick search is possibly an issue in BS classes (but not sure about that) see :
https://github.com/twbs/bootstrap/blob/v4-dev/scss/_tooltip.scss#L17-L59

FYI this issue exist since the merged of #17094 because before this PR BS4 cleaned Tether classes

@pvdlg
Copy link
Contributor

pvdlg commented Mar 28, 2017

Finally after a quick search is possibly an issue in BS classes (but not sure about that) see :
https://github.com/twbs/bootstrap/blob/v4-dev/scss/_tooltip.scss#L17-L59
FYI this issue exist since the merged of #17094 because before this PR BS4 cleaned Tether classes

Right ! Actually the code removed in #17094 was mentioning the Tether issue shipshapecode/tether#36, that is still opened....since March 2014...

@Johann-S
Copy link
Member Author

Until they are a fix published by Tether I'll merge this PR.

Thank you for your help and feedbacks @vanduynslagerp 👍

@Johann-S Johann-S merged commit 904efc0 into v4-dev Mar 28, 2017
@Johann-S Johann-S deleted the v4-tooltip-offset branch March 28, 2017 13:55
@mdo mdo mentioned this pull request Mar 28, 2017
@IdanCo
Copy link
Contributor

IdanCo commented Mar 29, 2017

@Johann-S kuddos for the fix, but even after the merge there is a strange behavior - when scrolling away and then back, the margin changes for the top and left tooltips. try scrolling down and up here -
http://jsbin.com/gopevakala/edit?html,output

I'm assuming it has something to do with the negative margins top and left tooltips has -

// scss/_tooltip.scss
  &.tooltip-top,
  &.bs-tether-element-attached-bottom {
    margin-top: -$tooltip-margin;
// scss/_tooltip.scss
  &.tooltip-left,
  &.bs-tether-element-attached-right {
    margin-left: -$tooltip-margin;

While right and bottom tooltips has positive margin -

// scss/_tooltip.scss
  &.tooltip-right,
  &.bs-tether-element-attached-left {
    margin-left: $tooltip-margin;
// scss/_tooltip.scss
  &.tooltip-bottom,
  &.bs-tether-element-attached-top {
    margin-top: $tooltip-margin;

Also, you can notice that the initial state lacks margins -
tooltips-margin

I tried looking for a quick win here by replacing the negative with positive, but the inconsistency continues. If you want i'll be happy to dig deeper.

@Johann-S
Copy link
Member Author

Please @IdanCo open a new issue to expose this problem thank you 👍

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