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

Recompute hover on click to increase click robustness #1646

Merged
merged 1 commit into from
Jun 16, 2017

Conversation

rreusser
Copy link
Contributor

@rreusser rreusser commented May 4, 2017

Rehover caused a lot of problems with dropped clicks. This PR recomputes the hover data on click so that when it's used in the click, it's up to date. It's perhaps a bit of wasted effort, but it should solve the problems with dropped clicks.

@etpinard
Copy link
Contributor

etpinard commented May 8, 2017

Rehover caused a lot of problems with dropped clicks

Oh, at first I thought this PR was fixing #1640, sounds like it does something else. @rreusser can you elaborate?

@rreusser
Copy link
Contributor Author

Hmm… has nothing to do with #1640, to the best of my knowledge. I was having a lot of trouble while working on this example in which, depending on how I move the mouse, maybe half of the clicks on the points go unregistered. The fundamental problem is that the point you're hovering over is only computed when the mouse moves or when the data changes, not when you click. That seems to be inadequate and result in a lot of dropped clicks. The solution here is to also just compute which point has been clicked when you actually click.

@etpinard
Copy link
Contributor

etpinard commented May 10, 2017

@rreusser great. Thanks for the info!

Your patch will conflict with #1582 where graph_interact.js got split up in an fx component - which needs to get merge today!

If you really wants this in the next release 1.27.0, you can try applying your patch on top of hoverlabel-custom branch of PR #1582

@rreusser
Copy link
Contributor Author

Working on rebasing…

@etpinard
Copy link
Contributor

@rreusser nice. I'm about to merge #1582, so it might be easier to wait for that and merge master onto this branch.

@etpinard
Copy link
Contributor

#1582 is now in master

@etpinard
Copy link
Contributor

@rreusser I'm going to put this under the 1.28.0 milestone. Let me know if you need help wrapping up this PR.

@etpinard etpinard added this to the 1.28.0 milestone May 23, 2017
@rreusser
Copy link
Contributor Author

Sorry this one sat for a while. I really need that plugin that lets me flag github notifications I need to return to. I've rebased and am waiting for the test to confirm the fix is transferred over.

@etpinard
Copy link
Contributor

@rreusser tests are ✅

Did you want to add anything else to this PR? Sounds like a solid improvement.

@etpinard
Copy link
Contributor

@rreusser ping.

@rreusser
Copy link
Contributor Author

I'm content with this. It's rebased and tested. 👍

@etpinard
Copy link
Contributor

Amazing! Thanks @rreusser 💃

@rreusser rreusser merged commit 2940d95 into master Jun 16, 2017
@rreusser rreusser deleted the recompute-hover-on-click branch June 16, 2017 18:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants