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

Pick closest points to the winning point in compare mode and position unified hover box in front of winning point #5683

Merged
merged 10 commits into from
May 28, 2021

Conversation

archmoj
Copy link
Contributor

@archmoj archmoj commented May 25, 2021

Fixes #5681. cc: #5553

And fixes #5680 by c692eda.

Also closes ##5656 | Demo

@plotly/plotly_js

@archmoj archmoj added bug something broken status: reviewable labels May 25, 2021
@archmoj archmoj added this to the NEXT milestone May 25, 2021
@archmoj archmoj requested a review from nicolaskruchten May 25, 2021 17:34
@archmoj archmoj requested a review from alexcjohnson May 25, 2021 23:58
@nicolaskruchten
Copy link
Contributor

If you look at this pen https://codepen.io/nicolaskruchten/pen/MWpoMJK?editors=0010 (built with the latest artifact from this branch) and move your mouse back and forth over the third point from the left (the "March 1" point period-positioned to the end of the month) you will see that it breaks the invariant: when the cursor is on the left, the bar point in the hoverset is Q1 and when the cursor is on the right, the bar point in the hoverset is Q2.

March is in Q1 and so when March is the winning point, Q2 should never appear in the hoverset, regardless where the cursor is.

@archmoj archmoj changed the title Pick closest point in compare mode Pick closest points to the winning point in compare mode and position unified hover box in front of winning point May 26, 2021
@archmoj
Copy link
Contributor Author

archmoj commented May 26, 2021

If you look at this pen https://codepen.io/nicolaskruchten/pen/MWpoMJK?editors=0010 (built with the latest artifact from this branch) and move your mouse back and forth over the third point from the left (the "March 1" point period-positioned to the end of the month) you will see that it breaks the invariant: when the cursor is on the left, the bar point in the hoverset is Q1 and when the cursor is on the right, the bar point in the hoverset is Q2.

March is in Q1 and so when March is the winning point, Q2 should never appear in the hoverset, regardless where the cursor is.

If we draw a bar instead of a scatter point, and comment periodalignment for a moment it may become more clear what is going on.
As illustrated in this codepen the monthly hover points are stricted inside quarterly i.e. when using periodalignment: 'middle'. And if one sets the periodalignmentof the second trace toend`, then the end points of orange bars would be located inside two different blue bars.
Having fixed a number of issues here I think this PR in a good state to merge 🚀

@nicolaskruchten
Copy link
Contributor

The value of periodalignment should not impact the invariants listed in #5553. In this case the second one is not being honoured: the Q1 bar's span covers the period that the point represents and so it should be in the hoverset.

@archmoj
Copy link
Contributor Author

archmoj commented May 26, 2021

The value of periodalignment should not impact the invariants listed in #5553. In this case the second one is not being honoured: the Q1 bar's span covers the period that the point represents and so it should be in the hoverset.

Good call. Addressed in 4f92ccf.

@archmoj
Copy link
Contributor Author

archmoj commented May 26, 2021

@nicolaskruchten
Here is the updated codepen.

@nicolaskruchten
Copy link
Contributor

This is looking much better!

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.

I like it! That's a lot of closed issues 💪
💃

@archmoj archmoj merged commit 1063ecb into master May 28, 2021
@archmoj archmoj deleted the fix5681-closest-points-in-compare-mode branch May 28, 2021 02:59
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.

Incorrect hover invariant in period mode Unified hover box location problem
3 participants