fix for spikedistance=-1 #4637
fix for spikedistance=-1 #4637
Conversation
|
@antoinerg thanks for the follow ups! |
| @@ -161,7 +160,7 @@ function hoverOnBars(pointData, xval, yval, hovermode) { | |||
| pointData.valueLabel = hoverLabelText(sa, pointData[sizeLetter + 'LabelVal']); | |||
|
|
|||
| // spikelines always want "closest" distance regardless of hovermode | |||
| pointData.spikeDistance = (sizeFn(di) + thisBarPositionFn(di)) / 2 + maxSpikeDistance - maxHoverDistance; | |||
| pointData.spikeDistance = (sizeFn(di) + thisBarPositionFn(di)) / 2 - maxHoverDistance; | |||
There was a problem hiding this comment.
maxHoverDistance is necessary here because it is added in the function sizeFn. Now, why would a function computing the size of a bar would contain maxHoverDistance is beyond my comprehension.
There was a problem hiding this comment.
Yeah, super confusing. It's not computing the size of a bar, it's computing a distance measure along the size axis. Is it necessary to remove maxSpikeDistance here? Or it just doesn't have an understood purpose?
There was a problem hiding this comment.
Is it necessary to remove maxSpikeDistance here?
Keeping maxSpikeDistance in leads to:
Or it just doesn't have an understood purpose?
I'll be honest: I also don't understand its purpose. What is spikedistance supposed to represent? Why would we add maxSpikeDistance and substract maxHoverDistance to it at the trace-level? It seems to me that this logic should live in fx/hover.js no?
There was a problem hiding this comment.
yeah this logic should certainly be cleaned up at some point... super hacky. Perhaps as part of #4638 🙏
| mockCopy.layout.yaxis.showspikes = true; | ||
| mockCopy.layout.spikedistance = -1; | ||
|
|
||
| Plotly.newPlot(gd, mockCopy) |
There was a problem hiding this comment.
Non-blocking:
We could us newPlot > plot here.
There was a problem hiding this comment.
Is that desirable? plot is not part of the API we want anyone using, so I'd kind of rather not use it in tests either, even if it's a little faster.
There was a problem hiding this comment.
Thanks for the note. Resolving...
And please feel free to open an issue if you prefer to chage similar cases in all the tests.
There was a problem hiding this comment.
I wouldn't bother converting the rest unless there are problems. Sometimes there's a good reason to go for the faster one, in particular I seem to recall a lot of overhead to newPlot in WebGL tests? But that overhead comes with more assurance that there's no cross-test interaction, which has lead to hard-to-debug errors in the past. @etpinard any words of wisdom to add here?
| @@ -593,4 +616,139 @@ describe('spikeline hover', function() { | |||
| .catch(failTest) | |||
| .then(done); | |||
| }); | |||
|
|
|||
| it('correctly draws lines up to the last point', function(done) { | |||
| Plotly.newPlot(gd, [ | |||
There was a problem hiding this comment.
Non-blocking:
We could us newPlot > plot here.
| } | ||
| }; | ||
|
|
||
| Plotly.newPlot(gd, mock) |
There was a problem hiding this comment.
Non-blocking:
We could us newPlot > plot here.
Supersedes #4627 and fixes #4626 and #3805
For #4626:
Before: https://codepen.io/antoinerg/pen/BaNYJve
After: https://codepen.io/antoinerg/pen/oNXEENy
For #3805:
Before: https://codepen.io/antoinerg/pen/eYNMrBq
After: https://codepen.io/antoinerg/pen/ExjQQYw
cc @archmoj @alexcjohnson @etpinard
I opened issue #4638 to deal with an edge case.