-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
fix for spikedistance=-1 #4637
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 for spikedistance=-1 #4637
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -404,6 +404,29 @@ describe('spikeline hover', function() { | |
.then(done); | ||
}); | ||
|
||
it('correctly select the closest bar even when setting spikedistance to -1', function(done) { | ||
var mock = require('@mocks/bar_stack-with-gaps'); | ||
var mockCopy = Lib.extendDeep({}, mock); | ||
mockCopy.layout.xaxis.showspikes = true; | ||
mockCopy.layout.yaxis.showspikes = true; | ||
mockCopy.layout.spikedistance = -1; | ||
|
||
Plotly.newPlot(gd, mockCopy) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Non-blocking: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is that desirable? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the note. Resolving... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
.then(function() { | ||
_hover({xpx: 600, ypx: 400}); | ||
var lines = d3.selectAll('line.spikeline'); | ||
expect(lines.size()).toBe(4); | ||
expect(lines[0][1].getAttribute('stroke')).toBe('#2ca02c'); | ||
|
||
_hover({xpx: 600, ypx: 200}); | ||
lines = d3.selectAll('line.spikeline'); | ||
expect(lines.size()).toBe(4); | ||
expect(lines[0][1].getAttribute('stroke')).toBe('#1f77b4'); | ||
}) | ||
.catch(failTest) | ||
.then(done); | ||
}); | ||
|
||
it('correctly responds to setting the spikedistance to 0 by disabling ' + | ||
'the search for points to draw the spikelines', function(done) { | ||
var _mock = makeMock('toaxis', 'closest'); | ||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Non-blocking: |
||
{type: 'bar', y: [5, 7, 9, 6, 4, 3]}, | ||
{y: [5, 7, 9, 6, 4, 3]}, | ||
{y: [5, 7, 9, 6, 4, 3], marker: {color: 'red'}} | ||
], { | ||
xaxis: {showspikes: true}, | ||
yaxis: {showspikes: true}, | ||
spikedistance: -1, | ||
width: 400, height: 400, | ||
showlegend: false | ||
}) | ||
.then(function() { | ||
_hover({xpx: 150, ypx: 250}); | ||
|
||
var lines = d3.selectAll('line.spikeline'); | ||
expect(lines.size()).toBe(4); | ||
expect(lines[0][1].getAttribute('stroke')).toBe('red'); | ||
expect(lines[0][3].getAttribute('stroke')).toBe('red'); | ||
}) | ||
.catch(failTest) | ||
.then(done); | ||
}); | ||
|
||
describe('works across all cartesian traces', function() { | ||
var schema = Plotly.PlotSchema.get(); | ||
var traces = Object.keys(schema.traces); | ||
var tracesSchema = []; | ||
var i, j, k; | ||
for(i = 0; i < traces.length; i++) { | ||
tracesSchema.push(schema.traces[traces[i]]); | ||
} | ||
var excludedTraces = [ 'image' ]; | ||
var cartesianTraces = tracesSchema.filter(function(t) { | ||
return t.categories.length && | ||
t.categories.indexOf('cartesian') !== -1 && | ||
t.categories.indexOf('noHover') === -1 && | ||
excludedTraces.indexOf(t.type) === -1; | ||
}); | ||
|
||
function makeData(type, axName, a, b) { | ||
var input = [a, b]; | ||
var cat = input[axName === 'yaxis' ? 1 : 0]; | ||
var data = input[axName === 'yaxis' ? 0 : 1]; | ||
|
||
var measure = []; | ||
for(j = 0; j < data.length; j++) { | ||
measure.push('absolute'); | ||
} | ||
|
||
var z = Lib.init2dArray(cat.length, data.length); | ||
for(j = 0; j < z.length; j++) { | ||
for(k = 0; k < z[j].length; k++) { | ||
z[j][k] = 0; | ||
} | ||
} | ||
if(axName === 'xaxis') { | ||
for(j = 0; j < b.length; j++) { | ||
z[0][j] = b[j]; | ||
} | ||
} | ||
if(axName === 'yaxis') { | ||
for(j = 0; j < b.length; j++) { | ||
z[j][0] = b[j]; | ||
} | ||
} | ||
|
||
return Lib.extendDeep({}, { | ||
orientation: axName === 'yaxis' ? 'h' : 'v', | ||
type: type, | ||
x: cat, | ||
a: cat, | ||
|
||
b: data, | ||
y: data, | ||
z: z, | ||
|
||
// For OHLC | ||
open: data, | ||
close: data, | ||
high: data, | ||
low: data, | ||
|
||
// For histogram | ||
nbinsx: cat.length, | ||
nbinsy: data.length, | ||
|
||
// For waterfall | ||
measure: measure, | ||
|
||
// For splom | ||
dimensions: [ | ||
{ | ||
label: 'DimensionA', | ||
values: a | ||
}, | ||
{ | ||
label: 'DimensionB', | ||
values: b | ||
} | ||
] | ||
}); | ||
} | ||
|
||
cartesianTraces.forEach(function(trace) { | ||
it('correctly responds to setting the spikedistance to -1 for ' + trace.type, function(done) { | ||
var type = trace.type; | ||
var x = [4, 5, 6]; | ||
var data = [7, 2, 3]; | ||
|
||
var mock = { | ||
data: [makeData(type, 'xaxis', x, data)], | ||
layout: { | ||
spikedistance: -1, | ||
xaxis: {showspikes: true}, | ||
yaxis: {showspikes: true}, | ||
zaxis: {showspikes: true}, | ||
title: {text: trace.type}, | ||
width: 400, height: 400 | ||
} | ||
}; | ||
|
||
Plotly.newPlot(gd, mock) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Non-blocking: |
||
.then(function() { | ||
_hover({xpx: 200, ypx: 100}); | ||
|
||
var lines = d3.selectAll('line.spikeline'); | ||
expect(lines.size()).toBe(4); | ||
}) | ||
.catch(failTest) | ||
.then(done); | ||
}); | ||
}); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maxHoverDistance
is necessary here because it is added in the functionsizeFn
. Now, why would a function computing the size of a bar would containmaxHoverDistance
is beyond my comprehension.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keeping

maxSpikeDistance
in leads to:I'll be honest: I also don't understand its purpose. What is
spikedistance
supposed to represent? Why would we addmaxSpikeDistance
and substractmaxHoverDistance
to it at the trace-level? It seems to me that this logic should live infx/hover.js
no?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah this logic should certainly be cleaned up at some point... super hacky. Perhaps as part of #4638 🙏