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

Fine-tune delays in Tooltip and associated components #2368

Draft
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

r100-stack
Copy link
Member

@r100-stack r100-stack commented Dec 11, 2024

Note

Merging this PR in the next minor release (#2368 (comment)).

Changes

After we fixed the default Tooltip delay in #2263, a couple of components had the problem of lingering tooltips. To fix it, this PR wraps some of the parts of the affected components within Floating UI's FloatingDelayGroup.

Components where FloatingDelayGroup wrapper is added:

While this PR fixes the lingering tooltip problem within the items of the component itself, it doesn't fix the problem between two components. To fix the latter part also, I had considered moving FloatingDelayGroup to the ThemeProvider but then didn't go that route based on our internal discussion. One of the reasons to not go that route was to prevent unintended issues.

According to our discussion, I also changed the default tooltip delay from { open: 50, close: 250 } to { open: 100, close: 200 }. If we later find research regarding the best delay values, we can further fine-tune this default delay.

Testing

CI passing after small updates to the tests.

Before and After
Component Before After
Stepper
Before.mov
After.mov
SideNavigation
Before.mov
After.mov
Slider
Before.mov
After.mov

Docs

Added a changeset.

After PR TODOs:

  • See how to solve lingering tooltips across different components. (#2368 (review))
  • Document more about the placement of the Tooltip (#2368 (review))

@r100-stack r100-stack self-assigned this Dec 11, 2024
@r100-stack r100-stack marked this pull request as ready for review December 11, 2024 22:32
@r100-stack r100-stack requested a review from a team as a code owner December 11, 2024 22:32
@r100-stack r100-stack requested review from mayank99 and smmr-dn and removed request for a team December 11, 2024 22:32
Copy link
Contributor

@smmr-dn smmr-dn left a comment

Choose a reason for hiding this comment

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

I also notice the problem caused by delay when the label prop is passed into IconButton. Does it still need to be wrapped with FloatingDelayGroup?

tooltip-delay-in-menu.mp4

@r100-stack
Copy link
Member Author

I also notice the problem caused by delay when the label prop is passed into IconButton. Does it still need to be wrapped with FloatingDelayGroup?
tooltip-delay-in-menu.mp4

Since those Tooltips are across different instances of IconButtons, this PR doesn't address that atm. While putting FloatingDelayGroup into something global like the ThemeProvider will fix the issue, it might lead to other issues which is why this PR went away from that approach (more info in the PR description). Instead, this PR just solves the lingering Tooltips issues within an instance of a component and not across different instances of same or different components.

Also, noticed an issue in the example where the text parts of the ListItems were not visible. Fixed that in b9b2d1c.

Screenshots
Old New
image image

Copy link
Contributor

@mayank99 mayank99 left a comment

Choose a reason for hiding this comment

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

I also notice the problem caused by delay when the label prop is passed into IconButton. Does it still need to be wrapped with FloatingDelayGroup?

Since those Tooltips are across different instances of IconButtons, this PR doesn't address that atm.

Could we use FloatingDelayGroup in List? It makes sense because it is an isolated component containing repeated items (similar to SideNavigation/Stepper/ButtonGroup/etc).

@r100-stack
Copy link
Member Author

r100-stack commented Dec 12, 2024

I also notice the problem caused by delay when the label prop is passed into IconButton. Does it still need to be wrapped with FloatingDelayGroup?

Since those Tooltips are across different instances of IconButtons, this PR doesn't address that atm.

Could we use FloatingDelayGroup in List? It makes sense because it is an isolated component containing repeated items (similar to SideNavigation/Stepper/ButtonGroup/etc).

Makes sense, wrapped List with a FloatingDelayGroup (c889805). In that example, it helps when users are quickly hovering from one more options icon button to another in a top to bottom way. But when going from bottom to top, there's not much improvement. Also, the top to bottom way is improved only in the case where the user is hovering quickly. With a regular hover speed, there is actually little difference with or without the FloatingDelayGroup.

However, added the FloatingDelayGroup as it is at least an improvement.

Screen-recording
Without FloatingDelayGroup With FloatingDelayGroup
Without.mov
With.mov

Copy link
Contributor

@mayank99 mayank99 left a comment

Choose a reason for hiding this comment

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

But when going from bottom to top, there's not much improvement.

One fix for that would be to change the tooltip's placement, which consumers can do on their own.

I think this is quite a common issue for lists, and we should probably recommend changing placement in our documentation somewhere.

Copy link
Contributor

@mayank99 mayank99 left a comment

Choose a reason for hiding this comment

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

LGTM

@r100-stack
Copy link
Member Author

But when going from bottom to top, there's not much improvement.

One fix for that would be to change the tooltip's placement, which consumers can do on their own.

I think this is quite a common issue for lists, and we should probably recommend changing placement in our documentation somewhere.

Sounds good, added the documentation part as an after-PR TODO since I saw your approval.

@r100-stack r100-stack marked this pull request as draft December 15, 2024 13:43
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.

3 participants