Skip to content

Fix scatter and scattergl hover on axes with period and adjust the unified hover position in respect to mean #5836

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Jul 20, 2021
Merged

Fix scatter and scattergl hover on axes with period and adjust the unified hover position in respect to mean #5836

merged 6 commits into from
Jul 20, 2021

Conversation

archmoj
Copy link
Contributor

@archmoj archmoj commented Jul 19, 2021

Closes #5822.

@plotly/plotly_js

@archmoj archmoj added bug something broken status: reviewable labels Jul 19, 2021
@archmoj archmoj added this to the NEXT milestone Jul 19, 2021
nicolaskruchten
nicolaskruchten reviewed Jul 19, 2021
View reviewed changes
src/traces/scatter/calc.js
@@ -51,23 +51,13 @@ function calc(gd, trace) {
calcAxisExpansion(gd, trace, xa, ya, x, y, ppad);
}

var hasPeriodX = !!trace.xperiodalignment;
Copy link
Contributor

Choose a reason for hiding this comment

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

interesting that this PR is mostly removals :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Less is more :)

@nicolaskruchten
Copy link
Contributor

Playing with this a bit, I'm noticing that the y-position of the unified label feels a bit weird... I thought it was set to be at the median of all the points it contained, or something like that, no?

@nicolaskruchten
Copy link
Contributor

Also, if I run my cursor carefully up and down the left-most spikeline, I see the hoverlabel jump around in the y-dimension:

unified

@archmoj
Copy link
Contributor Author

archmoj commented Jul 19, 2021

Playing with this a bit, I'm noticing that the y-position of the unified label feels a bit weird... I thought it was set to be at the median of all the points it contained, or something like that, no?

No. That's based on the winning point at the moment.

plotly.js/src/components/fx/hover.js

Lines 1073 to 1076 in 990e260

// Position the hover
var winningPoint = hoverData[0];
var ly = (winningPoint.y0 + winningPoint.y1) / 2;
var lx = (winningPoint.x0 + winningPoint.x1) / 2;

However we could possibly use median of y values on x unified and median of x on y unified.
If that's what you want please open a new issue for that.

@nicolaskruchten
Copy link
Contributor

That's based on the winning point at the moment

Is this something we changed in the v2 set of changes? I feel like I recall that when we first did this x unified feature it was based on the midpoint or something.

@archmoj
Copy link
Contributor Author

archmoj commented Jul 19, 2021

That's based on the winning point at the moment

Is this something we changed in the v2 set of changes? I feel like I recall that when we first did this x unified feature it was based on the midpoint or something.

Positive:
https://github.com/plotly/plotly.js/pull/5683/files
Screenshot from 2021-07-19 16-06-31

@archmoj
Copy link
Contributor Author

archmoj commented Jul 19, 2021

Now addressed in b44e11d

@nicolaskruchten
Copy link
Contributor

Ah, much nicer! For some reason the hover for the left-most seems higher with respect to the data than the others... is that normal?

@archmoj
Copy link
Contributor Author

archmoj commented Jul 19, 2021

Ah, much nicer! For some reason the hover for the left-most seems higher with respect to the data than the others... is that normal?

Look normal to me: demo

@archmoj archmoj requested a review from alexcjohnson July 19, 2021 20:52
@nicolaskruchten
Copy link
Contributor

Check out the position of the hover wrt the yellow and blue lines for the right-most (sorry, I mistakenly said left-most above)
unified

@archmoj
Copy link
Contributor Author

archmoj commented Jul 19, 2021

Check out the position of the hover wrt the yellow and blue lines for the right-most (sorry, I mistakenly said left-most above)
unified

That works fine on my ubuntu using Chrome.

@nicolaskruchten
Copy link
Contributor

Ok, we can fix that particular oddity later I guess :)

@nicolaskruchten
Copy link
Contributor

Huh, this only happens in Observable, not in CodePen. Annoying! https://codepen.io/nicolaskruchten/pen/JjNJgxE

But ✨ from me!

alexcjohnson
alexcjohnson reviewed Jul 20, 2021
View reviewed changes
test/jasmine/tests/gl2d_click_test.js Outdated
@@ -627,6 +627,9 @@ describe('hover with (x|y)period positioning', function() {

it('@gl shows hover info for scattergl', function(done) {
Plotly.newPlot(gd, require('@mocks/gl2d_period_positioning.json'))
.then(function() { return Plotly.restyle(gd, 'xperiodalignment', 'start'); })
.then(function() { return Plotly.restyle(gd, 'yperiodalignment', 'start'); })
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nonblocking, but these tests can be simplified (and sped up) with the object form

Plotly.restyle(gd,{xperiodalignment: 'start', yperiodalignment: 'start'})

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 17215eb

alexcjohnson
alexcjohnson reviewed Jul 20, 2021
View reviewed changes
test/jasmine/tests/hover_label_test.js
@@ -2663,14 +2663,14 @@ describe('hover on traces with (x|y)period positioning', function() {
.then(function() {
assertHoverLabelContent({
name: 'scatter',
nums: '(Jan 2001, 1)'
nums: '(Jul 2001, 1)'
Copy link
Collaborator

Choose a reason for hiding this comment

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

So these tests were actually locking down the incorrect behavior previously... This is why in general I think such tests should be self-contained rather than relying on a separate mock: you can read the test carefully and if the dimensions are nice and simple (ie you set round numbers for the size, margins, & axis ranges) you can manually verify that it's the desired behavior.

alexcjohnson
alexcjohnson approved these changes Jul 20, 2021
View reviewed changes
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! Only non-blocking comments. 💃

@archmoj archmoj changed the title Fix scatter hover on axes with period Fix scatter and scattergl hover on axes with period and adjust the unified hover position in respect to mean Jul 20, 2021
@archmoj archmoj changed the title Fix scatter and scattergl hover on axes with period and adjust the unified hover position in respect to mean Fix scatter and scattergl hover on axes with period and adjust the unified hover position in respect to mean Jul 20, 2021
@archmoj archmoj merged commit ff41d70 into master Jul 20, 2021
@archmoj archmoj deleted the fix5822-hover-position branch July 20, 2021 12:23
@archmoj archmoj mentioned this pull request Jul 20, 2021
Merged
@archmoj archmoj removed this from the v2.3.0 milestone Jul 20, 2021
@archmoj archmoj mentioned this pull request Jul 20, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolaskruchten nicolaskruchten nicolaskruchten left review comments

@alexcjohnson alexcjohnson alexcjohnson approved these changes

Assignees
No one assigned
Labels
bug something broken
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

hover misplaced with period positioning
3 participants
@archmoj @nicolaskruchten @alexcjohnson