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: Sticky position and support for dynamic content (e.g. via React's createPortal) #402

Merged
merged 18 commits into from
Jul 23, 2024

Conversation

rokotyan
Copy link
Contributor

@rokotyan rokotyan commented Jun 18, 2024

This PR will add support for sticky tooltip positioning:

Screen.Recording.2024-06-18.at.11.48.31.AM.mp4
Screen.Recording.2024-06-20.at.2.45.23.PM.mp4

@rokotyan rokotyan force-pushed the feature/tooltip-positioning branch from 693e218 to ecea6ee Compare June 18, 2024 19:46
@rokotyan rokotyan changed the title Tooltip: Sticky position Tooltip: Sticky position and support for dynamic content (e.g. via React's createPortal) Jun 20, 2024
@rokotyan rokotyan marked this pull request as ready for review June 20, 2024 22:37
@rokotyan rokotyan marked this pull request as draft June 20, 2024 23:54
@rokotyan rokotyan force-pushed the feature/tooltip-positioning branch 4 times, most recently from c6a6bad to 4ec0bce Compare June 24, 2024 22:44
@rokotyan rokotyan marked this pull request as ready for review June 26, 2024 18:31
@rokotyan
Copy link
Contributor Author

@reb-dev I think this is ready for review, but I'm having troubles finding time to update the docs to tell about these new features. If you think that either you or @lee00678 can do it, I would really appreciate it.

@reb-dev
Copy link
Collaborator

reb-dev commented Jun 26, 2024

@rokotyan Nice feature! I will add the docs within the next couple days.

Copy link
Collaborator

@reb-dev reb-dev left a comment

Choose a reason for hiding this comment

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

@rokotyan When testing I noticed inconsistent behavior with crosshair when followCursor is false. The place where tooltip is hoverable seems to be random, and it sometimes follows the cursor even when crosshair isn't active.

It is difficult to showcase with a recording but here is an example of what I mean:

Screen.Recording.2024-06-26.at.1.58.43.PM.mov

I'm wondering if we can rework the logic so crosshair's tooltip can support this feature, or if we should automatically disable it?

@rokotyan
Copy link
Contributor Author

@reb-dev You're right, I haven't tested this use case, thanks for your attention to detail! I think Crosshair doesn't need a hoverable tooltip, so we can safely disable it. I'll update the PR.

@rokotyan
Copy link
Contributor Author

rokotyan commented Jul 3, 2024

@reb-dev Updated the code

@reb-dev reb-dev requested a review from lee00678 July 5, 2024 21:39
reb-dev
reb-dev previously approved these changes Jul 5, 2024
@reb-dev
Copy link
Collaborator

reb-dev commented Jul 5, 2024

I made some changes to the Tooltip doc page and added the new info in the recent commit.

@lee00678 can you review? Approval needs to be from someone besides the last pusher.

@lee00678
Copy link
Collaborator

lee00678 commented Jul 5, 2024

@reb-dev looks good. Tested the dev examples and docs, both working as expected.

lee00678
lee00678 previously approved these changes Jul 5, 2024
@rokotyan
Copy link
Contributor Author

rokotyan commented Jul 9, 2024

Adding support for custom class names which is needed when using createPortal with multiple tooltips. Planning to finish this today.

@rokotyan rokotyan marked this pull request as draft July 9, 2024 17:07
@rokotyan rokotyan dismissed stale reviews from lee00678 and reb-dev via 924598a July 9, 2024 22:12
@rokotyan rokotyan force-pushed the feature/tooltip-positioning branch from ff98ee9 to 924598a Compare July 9, 2024 22:12
@rokotyan rokotyan marked this pull request as ready for review July 9, 2024 22:13
@rokotyan
Copy link
Contributor Author

rokotyan commented Jul 9, 2024

@reb-dev @lee00678 Pushed a few more commits. Ready for review again.

@rokotyan
Copy link
Contributor Author

rokotyan commented Jul 9, 2024

New in the docs
image

image

image

@rokotyan rokotyan force-pushed the feature/tooltip-positioning branch from 19d6a4a to a9daa23 Compare July 18, 2024 22:43
@rokotyan
Copy link
Contributor Author

Rebased from main

@lee00678 lee00678 merged commit 890c074 into f5:main Jul 23, 2024
4 checks passed
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