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_flickering_fix #2457

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

RitikBora
Copy link

This pull request has been raised for the issue #2225 pointed out by Ma1kovich.

When tooltip component hovers over an element (Say Button), and you scroll near the area where the tooltip and the element meet. The tooltip starts to flicker.

I saw in the default moveby offset was {main: 4 , secondary: 0} which was not for enought for all top and bottom tooltips.
For the rest is was ok.

So I added a little more main offset to increase the gap between the tooltip and the element for which tooltip is there. This solved the flickering issue for all top and bottom tooltips

I'm attaching a video with the fix for your reference.

Vite.+.React.+.TS.mp4

@talkor let me know your thoughts on this.

Thanks,
Ritik

@RitikBora RitikBora requested a review from a team as a code owner October 3, 2024 08:28
Copy link
Member

@talkor talkor left a comment

Choose a reason for hiding this comment

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

Thanks for this PR @RitikBora ! Can you please check my comment?
Also I feel like we'll have to test it a bit to make sure it behaves well in all scenarios

let moveAmount : MoveBy = this.props.moveBy;

if (position && (position.includes("top") || position.includes("bottom"))) {
moveAmount = { main: 11, secondary: 0 };
Copy link
Member

Choose a reason for hiding this comment

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

How will it apply to smaller components/elements? I feel like it will distant the tooltip from the referencing element

@RitikBora
Copy link
Author

Hey @talkor ,

Thank you for the feedback.

The default prop of {main: 4 , secondary: 0} seemed a little less to me and thats why I modified move by. But I do get your point here . It looks okay on the button but might not look good on some other elements

I'll try out a couple of things here.

  1. Test it out with some smaller elements. Any leads to what all should a try would be appriated :)
  2. Try a different approach where the space between is dynamic and just enough for the tooltip and element to be considered separate .

Let me know if there is anything else you'd like to suggest.

Thanks,
Ritik

@talkor
Copy link
Member

talkor commented Oct 13, 2024

Hey @talkor ,

Thank you for the feedback.

The default prop of {main: 4 , secondary: 0} seemed a little less to me and thats why I modified move by. But I do get your point here . It looks okay on the button but might not look good on some other elements

I'll try out a couple of things here.

  1. Test it out with some smaller elements. Any leads to what all should a try would be appriated :)

  2. Try a different approach where the space between is dynamic and just enough for the tooltip and element to be considered separate .

Let me know if there is anything else you'd like to suggest.

Thanks,

Ritik

That's awesome @RitikBora, thank you! I'll try to check it out and find some leads as well, and update here 🙏

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

Successfully merging this pull request may close these issues.

2 participants