-
Notifications
You must be signed in to change notification settings - Fork 842
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
[EuiTooltip] Revert "Remove tooltip focus on mousemove (#3335)" and force visibility while element is focused #5066
Conversation
This reverts commit fd3dd84. # Conflicts: # CHANGELOG.md # src/components/tool_tip/tool_tip.tsx
Preview documentation changes for this PR: https://eui.elastic.co/pr_5066/ |
1 similar comment
Preview documentation changes for this PR: https://eui.elastic.co/pr_5066/ |
FYI: I've decided to update the docs too, working on that now. Reviews can hold until I've pushed the updates. |
Preview documentation changes for this PR: https://eui.elastic.co/pr_5066/ |
Ok, this is reviewable again 😸 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes LGTM! Tested in the deployed docs & verified some changes locally
Preview documentation changes for this PR: https://eui.elastic.co/pr_5066/ |
Closes #5013 by reverting #3335 and applied the
onFocus
andonBlur
event handlers to the cloned child which basically means that while the wrapped item hasfocus
the tooltip will never close.Before
Screen.Recording.2021-08-23.at.12.03.17.PM.mp4
Just reverting
Screen.Recording.2021-08-23.at.12.07.00.PM.mp4
Forces visibility while in focus
Screen.Recording.2021-08-23.at.12.19.20.PM.mp4
What would be even better
Is if we could also keep the tooltip shown when a user tries to interact with the contents. I know we've mostly said that interactive content should not be placed within tooltips, but use popovers instead. I still think this is correct, but I do think it would be helpful for when users might want to copy text.
Example of it not working right now:
Screen.Recording.2021-08-23.at.12.30.56.PM.mp4
Thought process:
Originally posted by @cchaos on #5013 (comment)
Checklist
- [ ] Props have proper autodocs and playground toggles- [ ] Checked Code Sandbox works for the any docs examples- [ ] Added or updated jest tests