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

Fixup hover filters #5668

Merged
merged 12 commits into from
May 24, 2021
Merged

Fixup hover filters #5668

merged 12 commits into from
May 24, 2021

Conversation

archmoj
Copy link
Contributor

@archmoj archmoj commented May 19, 2021

Fixes bug mentioned in #5554 (comment),
also simplifies period and non-period hovers in respect to #5553 and #5554.

Also closes #5572 : Demo Before vs After.

And closes #4787 : Demo Before vs After

@plotly/plotly_js

@archmoj archmoj added bug something broken status: reviewable labels May 19, 2021
@archmoj archmoj added this to the NEXT milestone May 19, 2021
@nicolaskruchten
Copy link
Contributor

With this PR it's possible to get two scatter hovers now:

image

@archmoj
Copy link
Contributor Author

archmoj commented May 20, 2021

With this PR it's possible to get two scatter hovers now:

image

I was not able to replicate this. Could you please share the figure?

@nicolaskruchten
Copy link
Contributor

https://observablehq.com/@nicolaskruchten/plotly-js-branch-tip-chooser on this current branch. Start with your mouse on the far right and very slowly scan it towards the left. There is a very narrow zone between the two right-most markers where this happens.

@archmoj
Copy link
Contributor Author

archmoj commented May 20, 2021

https://observablehq.com/@nicolaskruchten/plotly-js-branch-tip-chooser on this current branch. Start with your mouse on the far right and very slowly scan it towards the left. There is a very narrow zone between the two right-most markers where this happens.

Thanks. Fixed by 3b85534.

@archmoj
Copy link
Contributor Author

archmoj commented May 20, 2021

@nicolaskruchten @alexcjohnson I fixed the issues we noticed today and updated the codepens.

@nicolaskruchten
Copy link
Contributor

Wow, this looks pretty great, behaviour-wise, thanks! Nice bug-fixing too 👍

@alexcjohnson
Copy link
Collaborator

One concern: looks like a scatter point in the gap between two bars, but clearly within the data range of one of them, can be paired with hover from the other one if the mouse is over that other half of the gap.
Screen Shot 2021-05-21 at 3 08 23 PM
Screen Shot 2021-05-21 at 3 10 04 PM

Plotly.newPlot(gd,[
    {x:[1,2,3,4],y:[1,2,3,4],type:'bar'},
    {x:[0.6,0.8,1,1.2,1.49,1.75,2.51,3.2,5],y:[3,3,3,3,3,3,3,3,3],mode:'markers'}
],{hovermode:'x'})

@alexcjohnson
Copy link
Collaborator

Actually even if the scatter point isn't over the gap, but the gap is small enough compared to the hover distance, you can get the same thing in more severe form:
Screen Shot 2021-05-21 at 3 19 44 PM

@alexcjohnson
Copy link
Collaborator

The situation I'm describing in the above two comments violates the second rule in #5554:

the bar is in the hover set if-and-only-if the point that wins the hover is within the range of the bar (bar.x +/- width/2)

Note that that rule references "the point that wins the hover" and says nothing about where the cursor is.

@archmoj
Copy link
Contributor Author

archmoj commented May 21, 2021

The situation I'm describing in the above two comments violates the second rule in #5554:

the bar is in the hover set if-and-only-if the point that wins the hover is within the range of the bar (bar.x +/- width/2)

Note that that rule references "the point that wins the hover" and says nothing about where the cursor is.

After trying out a number of potential fixes towards enforcing that rule, and after looking at x unified variant I think this cursor dependant behavior might be actually desirable. So perhaps what we need is to relax that rule.

@nicolaskruchten
Copy link
Contributor

@archmoj please expand on why you think this needs relaxing?

@archmoj
Copy link
Contributor Author

archmoj commented May 21, 2021

@archmoj please expand on why you think this needs relaxing?

The point at 1.49 wins by hovering at 1.75. There is no harm to display bar at position 2 which is the closest bar. If we instead pick bar at position 1, it would be misleading. Please note that the unified hover is attached to cursor as well.

@nicolaskruchten
Copy link
Contributor

Thanks for the explanation. I still think the invariant as described is the right one but I'll think about it a bit more over the weekend. It's too hard to reason about things if the hover set depends on the cursor position and the winning point IMO.

My next question is how comes a scatter point at 1.49 wins with the cursor at 1.75 if there is a bar at 2? My instinct is to stick with the invariant as described and dig into the winning point logic after 2.0.

@archmoj
Copy link
Contributor Author

archmoj commented May 22, 2021

Thanks for the explanation. I still think the invariant as described is the right one but I'll think about it a bit more over the weekend. It's too hard to reason about things if the hover set depends on the cursor position and the winning point IMO.

My next question is how comes a scatter point at 1.49 wins with the cursor at 1.75 if there is a bar at 2? My instinct is to stick with the invariant as described and dig into the winning point logic after 2.0.

Yeah. Let's think about it during weekend.
I mean 1.6 not 1.75...

@archmoj
Copy link
Contributor Author

archmoj commented May 22, 2021

The situation I'm describing in the above two comments violates the second rule in #5554:

the bar is in the hover set if-and-only-if the point that wins the hover is within the range of the bar (bar.x +/- width/2)

Note that that rule references "the point that wins the hover" and says nothing about where the cursor is.

OK. I made the adjustment to respect the rule. It doesn't look bad :)

Copy link
Collaborator

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

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

Looks perfect now! 💃

@archmoj archmoj merged commit 58353f7 into master May 24, 2021
@archmoj archmoj deleted the fixup-hover-filters branch May 24, 2021 18:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug something broken
Projects
None yet
3 participants