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

Fix hover filter to display close period points #5543

Merged
merged 7 commits into from
Apr 10, 2021
Merged

Conversation

archmoj
Copy link
Contributor

@archmoj archmoj commented Mar 10, 2021

Fix for #5292 the hover part.
Please note that the spikeline part was addressed in #5542.

@plotly/plotly_js

@archmoj archmoj added bug something broken status: reviewable labels Mar 10, 2021
@nicolaskruchten
Copy link
Contributor

Some high-level feedback on the behaviour here:

  1. we would like to keep the invariant we currently have that "whatever points show up in the one x unified hoverlabel are the same points that get a hoverlabel in x mode", which I don't believe is uniformly true with this PR
  2. when the scatter points are in period mode with periodalignment = end we would like the hover calculation to be made based on the data coordinate of the point rather than the pixel coordinate.
  3. I could see a case for saying that if I have bars in period=m3 mode, and scatter points (in any mode) on the first of the month, then I should not be able to see a hoverlabel with a Jan 1 point and a bar that represents Q4 i.e. the last three months of the previous year: Jan 1 is not in Q4.

@nicolaskruchten
Copy link
Contributor

We should also remember to test this against grouped bars.

@archmoj
Copy link
Contributor Author

archmoj commented Mar 15, 2021

  1. we would like to keep the invariant we currently have that "whatever points show up in the one x unified hoverlabel are the same points that get a hoverlabel in x mode", which I don't believe is uniformly true with this PR

x hovermode does not appear to be perfect IMHO. See this codepen. Should we log a bug for "bars and scatters don't show together in x hovermode when hovering over middle or right side of the bars"?

@archmoj
Copy link
Contributor Author

archmoj commented Mar 15, 2021

  1. we would like to keep the invariant we currently have that "whatever points show up in the one x unified hoverlabel are the same points that get a hoverlabel in x mode", which I don't believe is uniformly true with this PR

x hovermode does not appear to be perfect IMHO. See this codepen. Should we log a bug for "bars and scatters don't show together in x hovermode when hovering over middle or right side of the bars"?

In addition this codepen may illustrate another bug concerning x hovermode and period alignements.

@archmoj archmoj added this to the NEXT milestone Mar 16, 2021
@nicolaskruchten
Copy link
Contributor

Re compare mode (aka hovermode='x') please see #5553 and #5554 for a description of the desired behaviours, which could be implemented either in this PR or a different one.

@archmoj
Copy link
Contributor Author

archmoj commented Mar 24, 2021

  1. we would like to keep the invariant we currently have that "whatever points show up in the one x unified hoverlabel are the same points that get a hoverlabel in x mode", which I don't believe is uniformly true with this PR

Good call. Addressed in 1e59d89.

@archmoj archmoj force-pushed the fix5292-unified-hover branch from 431e12f to 900ded3 Compare March 24, 2021 18:32
@archmoj archmoj changed the title Fix unified hover filter to display close points Fix hover filter to display close period points Mar 25, 2021
@alexcjohnson
Copy link
Collaborator

I see one bit of problematic behavior: If I make the plot from your test and zoom in a bit, it's possible to get the spikelines and hover labels out of sync. Oddly enough the behavior is somewhat reversed between x and x unified but both are problematic:

Screen Shot 2021-04-06 at 10 32 01 AM

Screen Shot 2021-04-06 at 10 57 47 AM

@archmoj
Copy link
Contributor Author

archmoj commented Apr 9, 2021

I see one bit of problematic behavior: If I make the plot from your test and zoom in a bit, it's possible to get the spikelines and hover labels out of sync. Oddly enough the behavior is somewhat reversed between x and x unified but both are problematic:

Thanks for the comment. It seems like the different bug fixed by the other PR #5542.
So I merged master into this branch in 4d831e2.

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.

Excellent! Good catch that the problem I mentioned was just another flavor of the one you already solved 🎉
💃

@archmoj archmoj merged commit 963f362 into master Apr 10, 2021
@archmoj archmoj deleted the fix5292-unified-hover branch April 10, 2021 14:11
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
Development

Successfully merging this pull request may close these issues.

3 participants