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

Clean tooltip component unneeded functionality #32692

Merged
merged 2 commits into from
Nov 25, 2021
Merged

Conversation

GeoSot
Copy link
Member

@GeoSot GeoSot commented Jan 6, 2021

@GeoSot GeoSot requested a review from a team as a code owner January 6, 2021 22:41
@XhmikosR
Copy link
Member

XhmikosR commented Jan 8, 2021

So, basically this removes the ability to use the CSS classes for positioning the tooltips? Not sure I follow all the changes.

@MartijnCuppens you recently had a look at popper too.

@GeoSot
Copy link
Member Author

GeoSot commented Jan 8, 2021

So, basically this removes the ability to use the CSS classes for positioning the tooltips?

This is the half truth ;)

it still uses this:

.bs-tooltip-auto {
  &[data-popper-placement^="top"] {
    @extend .bs-tooltip-top;
  }
  &[data-popper-placement^="right"] {
    @extend .bs-tooltip-end;
  }
  &[data-popper-placement^="bottom"] {
    @extend .bs-tooltip-bottom;
  }
  &[data-popper-placement^="left"] {
    @extend .bs-tooltip-start;
  }
}

and as important on my opinion, it removes many of js, to be easier for future maintenance.

This can do the difference too

I only chose this solution as it pulls out some extra stuff

js/src/tooltip.js Outdated Show resolved Hide resolved
js/src/tooltip.js Outdated Show resolved Hide resolved
@XhmikosR XhmikosR force-pushed the fix-tooltip branch 2 times, most recently from 0f2d321 to 1cbddbf Compare February 4, 2021 09:32
@XhmikosR
Copy link
Member

XhmikosR commented Feb 4, 2021

So, this patch still has the issue with tooltip randomly showing up in the wrong position when hovering fast, multiple times. But at least, it does show.

@GeoSot
Copy link
Member Author

GeoSot commented Feb 4, 2021

So, this patch still has the issue with tooltip randomly showing up in the wrong position when hovering fast, multiple times. But at least, it does show.

If we remove the fallbackPlacements: this.config.fallbackPlacements (and probably disable the possibility to be overridden by config, as Rohit correctly pointed out), it fixes this issue

Also popper overrides the css properties position and margin so I omitted them

@mdo
Copy link
Member

mdo commented Feb 8, 2021

@GeoSot @rohit2sharma95 What work remains here? I see tests are failing and we might still have the same issue with the fast hovering bug. Do we need to do something similar as #32843? Something else?

@GeoSot
Copy link
Member Author

GeoSot commented Feb 8, 2021

@mdo

Only the coverage is failing and I am not sure why, as I removed many lines.

If you see my previous messages, at least till now, I am on the proposal to remove fallbackPlacements as #32843 and I think we will be much better, than now

@mdo
Copy link
Member

mdo commented Feb 8, 2021

If you see my previous messages, at least till now, I am on the proposal to remove fallbackPlacements as #32843 and I think we will be much better, than now

I'm down to try anything at this point :). @rohit2sharma95 and @XhmikosR would know better what the implications of that might be though.

@twbs twbs deleted a comment from GeoSot Feb 9, 2021
@XhmikosR XhmikosR changed the title Bootstrap tooltips don't appear after numbers of focusing - fix proposal Fix tooltips that don't appear after numbers of focusing Feb 9, 2021
@XhmikosR
Copy link
Member

XhmikosR commented Feb 9, 2021

I'm still seeing the issue with tooltip showing to the wrong position when hovering too fast. :/

@GeoSot
Copy link
Member Author

GeoSot commented Feb 9, 2021

@XhmikosR
You see the glitch because we decided to revert the fallbackPlacements
I deleted again, so give it one more try

@XhmikosR XhmikosR added this to Inbox in v5.0.0-beta2 via automation Feb 9, 2021
@XhmikosR
Copy link
Member

XhmikosR commented Feb 9, 2021

Confirmed, now it works! @rohit2sharma95 please review this, I added it in beta2 :)

@XhmikosR XhmikosR moved this from Inbox to Review in v5.0.0-beta2 Feb 9, 2021
@XhmikosR
Copy link
Member

XhmikosR commented Feb 9, 2021

@GeoSot do the current tests in this branch catch this issue? We should make sure this doesn't happen again :)

@GeoSot
Copy link
Member Author

GeoSot commented Feb 9, 2021

if you have any idea, how to do these tests, I would appreciate, any help or opinion

@XhmikosR
Copy link
Member

XhmikosR commented Feb 9, 2021

Unfortunately, I don't know how either. Maybe @rohit2sharma95 can help us here?

@XhmikosR XhmikosR moved this from In progress to Review in progress in v5.2.0 Nov 25, 2021
v5.2.0 automation moved this from Review in progress to Reviewer approved Nov 25, 2021
@XhmikosR XhmikosR marked this pull request as ready for review November 25, 2021 18:07
@XhmikosR XhmikosR merged commit 6f077ff into twbs:main Nov 25, 2021
v5.2.0 automation moved this from Reviewer approved to Done Nov 25, 2021
@GeoSot GeoSot deleted the fix-tooltip branch November 25, 2021 18:30
@XhmikosR XhmikosR mentioned this pull request Nov 30, 2021
3 tasks
GeoSot added a commit that referenced this pull request Dec 1, 2021
XhmikosR added a commit that referenced this pull request Dec 1, 2021
Remove a leftover after #32692

Co-authored-by: XhmikosR <xhmikosr@gmail.com>
GeoSot added a commit that referenced this pull request Dec 10, 2021
XhmikosR pushed a commit that referenced this pull request Dec 16, 2021
XhmikosR added a commit that referenced this pull request Dec 21, 2021
)

Regression of #32692

Co-authored-by: XhmikosR <xhmikosr@gmail.com>
GeoSot added a commit that referenced this pull request Feb 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
v5.2.0
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants