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/ins 1660 tooltip issues #5239

Merged
merged 5 commits into from
Oct 6, 2022
Merged

Conversation

gatzjames
Copy link
Contributor

@gatzjames gatzjames commented Oct 3, 2022

Fixes tooltip positioning and open/close state issues.

Highlights:

  • Adds react-aria to make the tooltip accessible and handle positioning
  • Removes the usage of insomnia-components tooltip. Kept out of scope completely removing it since it's used by other insomnia-components internals.

React-Aria pros/cons

changelog(Fixes): Fixed an issue with tooltip

@gatzjames gatzjames requested a review from a team October 3, 2022 12:59
@gatzjames gatzjames self-assigned this Oct 3, 2022
@gatzjames gatzjames force-pushed the fix/ins-1660-tooltip branch from 84e8426 to b954a39 Compare October 3, 2022 14:42
@gatzjames gatzjames closed this Oct 3, 2022
@gatzjames gatzjames reopened this Oct 3, 2022
@gatzjames gatzjames marked this pull request as draft October 3, 2022 14:44
@marckong
Copy link
Contributor

marckong commented Oct 4, 2022

It took some time to read upon code of react-stately as it didn't seem to have good documentation. It's a cool headless UI, I have to admit.

One interesting thing I discovered though is captured here:
https://www.loom.com/share/7595338814c7478099b2c94bbcd8153b. The video demonstrates some delay to actually pop a tooltip popover. However once it is open, you can switch between different tooltip blocks and show tooltip popovers immediately.

I wonder if we should discuss what benefits this library would entail. I am looking at it positively, but how will we incorporate theme context with this for instance? Will it ever have a conflict if we put all the design tokens into the theme context/provider and read from it?

@gatzjames gatzjames force-pushed the fix/ins-1660-tooltip branch from b954a39 to 5863f66 Compare October 5, 2022 13:23
Copy link
Contributor

@marckong marckong left a comment

Choose a reason for hiding this comment

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

Ran smoke tests and it works as expected. I wonder if we can shorten the delay time to display but that is just my personal opinion. Thanks for explanation on the call.

@gatzjames gatzjames force-pushed the fix/ins-1660-tooltip branch from 5863f66 to d8c172a Compare October 6, 2022 11:56
@gatzjames gatzjames marked this pull request as ready for review October 6, 2022 11:56
@gatzjames
Copy link
Contributor Author

I wonder if we can shorten the delay time to display but that is just my personal opinion.

Good one! Moving to a function component I didn't keep the default value of 400ms.
Did some quick search in the codebase and we seem to have different delay values for our tooltips (400/500/800/1000 ms). I don't see any reason for having different delays but will keep it out of this PR's scope to fix them and do some research before updating this.

@gatzjames gatzjames force-pushed the fix/ins-1660-tooltip branch from ccc7261 to 9f36d88 Compare October 6, 2022 12:55
@gatzjames gatzjames enabled auto-merge (squash) October 6, 2022 13:00
@gatzjames gatzjames merged commit 58fd810 into Kong:develop Oct 6, 2022
@gatzjames gatzjames deleted the fix/ins-1660-tooltip branch October 6, 2022 13:19
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