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

Add hover exit delay to HoverPlugin #53

Merged
merged 3 commits into from
May 4, 2023
Merged

Conversation

majeekg
Copy link
Contributor

@majeekg majeekg commented May 4, 2023

No description provided.

@marijnh
Copy link
Member

marijnh commented May 4, 2023

  • I'd like the default to be 100ms, to not deviate too strongly from the existing behavior.
  • All three branches in mousemove call clearTimeout. You could just move it to the top of the function.
  • Please use ?? instead of || for the defaulting so that people can set this to 0.

@majeekg
Copy link
Contributor Author

majeekg commented May 4, 2023

Thanks for the feedback @marijnh ! I've modified as you said.

@marijnh marijnh merged commit f123c7c into codemirror:main May 4, 2023
@marijnh
Copy link
Member

marijnh commented May 4, 2023

Thanks!

marijnh added a commit that referenced this pull request May 5, 2023
@marijnh
Copy link
Member

marijnh commented May 5, 2023

This breaks the tests, and on closer inspection has a bunch of issues with sloppy state management and race conditions, so I have reverted it again. I.e. what happens if the timeout has been set but the hover tooltip reopens for another hover action in the meantime?

@majeekg
Copy link
Contributor Author

majeekg commented May 5, 2023

Ah ok.

This breaks the tests

I see, I'll look into updating the tests.

what happens if the timeout has been set but the hover tooltip reopens for another hover action in the meantime?

If it uses same HoverPlugin instance then adding clearTimeout(this.hoverExitTimeout) in startHover should fix this i suppose.

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