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

Add spikelines #2247

Merged
merged 12 commits into from Jan 17, 2018

Large diffs are not rendered by default.

@@ -38,7 +38,28 @@ module.exports = {
editType: 'modebar',
description: 'Determines the mode of hover interactions.'
},

hoverdistance: {
valType: 'integer',
min: 0,

This comment has been minimized.

Copy link
@etpinard

etpinard Jan 16, 2018

Member

As Infinity isn't JSON-serializable, we declare attributes of the likes (e.g. hoverlabel.namelength) with min: -1, so that users can set hoverdistance: -1 for an infinite search radius. Morevover, hoverdistance: 0 should imply no hover labels/events, similarly for spikedistance.

This comment has been minimized.

Copy link
@apalchys

apalchys Jan 17, 2018

Author Contributor

Thanks! Done.

dflt: 20,
role: 'style',
editType: 'none',
description: [
'Sets the default distance (in points) to look for data',

This comment has been minimized.

Copy link
@etpinard

etpinard Jan 16, 2018

Member

We usually write (in pixels) instead of (in points) in our attribute descriptions.

'to add hover labels (zero means no cutoff)'
].join(' ')
},
spikedistance: {
valType: 'integer',
min: 0,
dflt: 20,
role: 'style',
editType: 'none',
description: [
'Sets the default distance (in points) to look for data to draw',
'spikelines to (zero means no cutoff).'
].join(' ')
},
hoverlabel: {
bgcolor: {
valType: 'color',
@@ -28,6 +28,8 @@ module.exports = function supplyLayoutDefaults(layoutIn, layoutOut, fullData) {
else hovermodeDflt = 'closest';

coerce('hovermode', hovermodeDflt);
coerce('hoverdistance');

This comment has been minimized.

Copy link
@etpinard

etpinard Jan 16, 2018

Member

We could get fancy here by making this:

var hoverMode = coerce('hovermode', hovermodeDflt);
if(hoverDistance) coerce('hoverdistance');

as hoverdistance is useless when hovermode: false.

Unfortunately, we can't add similar logic for spikedistance here as showspikes is per axis.

This comment has been minimized.

Copy link
@apalchys

apalchys Jan 17, 2018

Author Contributor

Actually, we can, because if hovermode is false, we are not firing any hover events, so the spikelines are disabled too.
So we can write it like this:

    var hoverMode = coerce('hovermode', hovermodeDflt);
    if(hoverMode) {
        coerce('hoverdistance');
        coerce('spikedistance');
    }

This comment has been minimized.

Copy link
@etpinard

etpinard Jan 17, 2018

Member

Good catch. Thanks!

coerce('spikedistance');

// if only mapbox or geo subplots is present on graph,
// reset 'zoom' dragmode to 'pan' until 'zoom' is implemented,
@@ -237,9 +237,6 @@ function handleCartesian(gd, ev) {
if(astr === 'hovermode' && (val === 'x' || val === 'y')) {
val = fullLayout._isHoriz ? 'y' : 'x';
button.setAttribute('data-val', val);
if(val !== 'closest') {
fullLayout._cartesianSpikesEnabled = 'off';
}
} else if(astr === 'hovermode' && val === 'closest') {
for(i = 0; i < axList.length; i++) {
ax = axList[i];
@@ -551,12 +548,10 @@ modeBarButtons.toggleSpikelines = {
click: function(gd) {
var fullLayout = gd._fullLayout;

fullLayout._cartesianSpikesEnabled = fullLayout.hovermode === 'closest' ?
(fullLayout._cartesianSpikesEnabled === 'on' ? 'off' : 'on') : 'on';
fullLayout._cartesianSpikesEnabled = fullLayout._cartesianSpikesEnabled === 'on' ? 'off' : 'on';

This comment has been minimized.

Copy link
@etpinard

etpinard Jan 16, 2018

Member

We might still need something similar here.

For example, with

var layout = {
    xaxis: {
      showspikes: true
    },
   yaxis: {
     showspikes: false
   }
  };

then click on toggleSpikeLines gives:

peek 2018-01-16 11-58

with no way to get back to the original behavior using the mode bar buttons.

This comment has been minimized.

Copy link
@apalchys

apalchys Jan 17, 2018

Author Contributor

Done

This comment has been minimized.

Copy link
@etpinard

etpinard Jan 17, 2018

Member

Great. Thanks!

Now, could you restore and update the modebar_test.js cases you removed in your first commit?


var aobj = setSpikelineVisibility(gd);

aobj.hovermode = 'closest';
Plotly.relayout(gd, aobj);
}
};
@@ -2048,7 +2048,7 @@ axes.doTicks = function(gd, axid, skipTitle) {
top: pos,
bottom: pos,
left: ax._offset,
rigth: ax._offset + ax._length,
right: ax._offset + ax._length,

This comment has been minimized.

Copy link
@etpinard

etpinard Jan 16, 2018

Member

Good catch. Did this cause any bugs previously?

This comment has been minimized.

Copy link
@apalchys

apalchys Jan 17, 2018

Author Contributor

It did in one of my early implementations

width: ax._length,
height: 0
};
@@ -387,6 +387,14 @@ module.exports = {
'plotted on'
].join(' ')
},
spikesnap: {
valType: 'enumerated',
values: ['data', 'cursor'],
dflt: 'data',
role: 'style',

This comment has been minimized.

Copy link
@etpinard

etpinard Jan 16, 2018

Member

Though always debatable, all the new attributes you added should be role: 'info' as they do affect the behavior of the graph, not just the aesthetics.

This comment has been minimized.

Copy link
@apalchys

apalchys Jan 17, 2018

Author Contributor

Thanks! Changed it for hoverdistance and spikedistance. Didn't change it for the spikesnap because all other axis properties (like showspikes and spikemode) have role style.

editType: 'none',
description: 'Determines whether spikelines are stuck to the cursor or to the closest datapoints.'
},
tickfont: fontAttrs({
editType: 'ticks',
description: 'Sets the tick font.'
@@ -145,6 +145,7 @@ module.exports = function supplyLayoutDefaults(layoutIn, layoutOut, fullData) {
coerce('spikethickness');
coerce('spikedash');
coerce('spikemode');
coerce('spikesnap');

This comment has been minimized.

Copy link
@etpinard

etpinard Jan 16, 2018

Member

If you're interested, we should covert this coerce block to a coerce2 pattern (see example) where showspikes: true is implied whenever one of the spike* is correctly set by the user. Non-blocking to get this PR merged though.

This comment has been minimized.

Copy link
@apalchys

apalchys Jan 17, 2018

Author Contributor

Thanks! Done.

This comment has been minimized.

Copy link
@etpinard

etpinard Jan 17, 2018

Member

Wow. Thanks for doing this!

}

var positioningOptions = {
@@ -1723,3 +1723,175 @@ describe('ohlc hover interactions', function() {
expect(d3.select('.hovertext').size()).toBe(1);
});
});

describe('hover distance', function() {
'use strict';

var mock = require('@mocks/19.json');

afterEach(destroyGraphDiv);

describe('closest hovermode', function() {
var mockCopy = Lib.extendDeep({}, mock);
mockCopy.layout.hovermode = 'closest';

beforeEach(function(done) {
Plotly.plot(createGraphDiv(), mockCopy.data, mockCopy.layout).then(done);
});

it('does not render if distance to the point is larger than default (>20)', function() {
var gd = document.getElementById('graph');
var evt = { xpx: 475, ypx: 175 };
Fx.hover('graph', evt, 'xy');

expect(gd._hoverdata).toEqual(undefined);
});

it('render if distance to the point is less than default (<20)', function() {
var gd = document.getElementById('graph');
var evt = { xpx: 475, ypx: 155 };
Fx.hover('graph', evt, 'xy');

var hoverTrace = gd._hoverdata[0];

expect(hoverTrace.curveNumber).toEqual(0);
expect(hoverTrace.pointNumber).toEqual(1);
expect(hoverTrace.x).toEqual(2);
expect(hoverTrace.y).toEqual(3);

assertHoverLabelContent({
nums: '(2, 3)',
name: 'trace 0'
});
});

it('responds to hoverdistance change', function() {
var gd = document.getElementById('graph');
var evt = { xpx: 475, ypx: 180 };
gd._fullLayout.hoverdistance = 30;

Fx.hover('graph', evt, 'xy');

var hoverTrace = gd._hoverdata[0];

expect(hoverTrace.curveNumber).toEqual(0);
expect(hoverTrace.pointNumber).toEqual(1);
expect(hoverTrace.x).toEqual(2);
expect(hoverTrace.y).toEqual(3);

assertHoverLabelContent({
nums: '(2, 3)',
name: 'trace 0'
});
});

it('correctly responds to setting the hoverdistance to 0 by increasing ' +
'the range of search for points to hover to Infinity', function() {
var gd = document.getElementById('graph');
var evt = { xpx: 475, ypx: 180 };
gd._fullLayout.hoverdistance = 0;

Fx.hover('graph', evt, 'xy');

var hoverTrace = gd._hoverdata[0];

expect(hoverTrace.curveNumber).toEqual(0);
expect(hoverTrace.pointNumber).toEqual(1);
expect(hoverTrace.x).toEqual(2);
expect(hoverTrace.y).toEqual(3);

assertHoverLabelContent({
nums: '(2, 3)',
name: 'trace 0'
});
});
});

describe('x hovermode', function() {
var mockCopy = Lib.extendDeep({}, mock);
mockCopy.layout.hovermode = 'x';

beforeEach(function(done) {
Plotly.plot(createGraphDiv(), mockCopy.data, mockCopy.layout).then(done);
});

it('does not render if distance to the point is larger than default (>20)', function() {
var gd = document.getElementById('graph');
var evt = { xpx: 450, ypx: 155 };
Fx.hover('graph', evt, 'xy');

expect(gd._hoverdata).toEqual(undefined);
});

it('render if distance to the point is less than default (<20)', function() {
var gd = document.getElementById('graph');
var evt = { xpx: 475, ypx: 155 };
Fx.hover('graph', evt, 'xy');

var hoverTrace = gd._hoverdata[0];

expect(hoverTrace.curveNumber).toEqual(0);
expect(hoverTrace.pointNumber).toEqual(1);
expect(hoverTrace.x).toEqual(2);
expect(hoverTrace.y).toEqual(3);

assertHoverLabelContent({
nums: '3',
axis: '2',
name: 'trace 0'
});
});

it('responds to hoverdistance change part 1', function() {
var gd = document.getElementById('graph');
var evt = { xpx: 450, ypx: 155 };
gd._fullLayout.hoverdistance = 10;

Fx.hover('graph', evt, 'xy');

expect(gd._hoverdata).toEqual(undefined);
});

it('responds to hoverdistance change part 2', function() {
var gd = document.getElementById('graph');
var evt = { xpx: 450, ypx: 155 };
gd._fullLayout.hoverdistance = 30;

Fx.hover('graph', evt, 'xy');

var hoverTrace = gd._hoverdata[0];

expect(hoverTrace.curveNumber).toEqual(0);
expect(hoverTrace.pointNumber).toEqual(1);
expect(hoverTrace.x).toEqual(2);
expect(hoverTrace.y).toEqual(3);

assertHoverLabelContent({
nums: '3',
axis: '2',
name: 'trace 0'
});
});

it('correctly responds to setting the hoverdistance to 0 by increasing ' +
'the range of search for points to hover to Infinity', function() {
var gd = document.getElementById('graph');
var evt = { xpx: 450, ypx: 155 };
gd._fullLayout.hoverdistance = 0;

Fx.hover('graph', evt, 'xy');

var hoverTrace = gd._hoverdata[0];

expect(hoverTrace.curveNumber).toEqual(0);
expect(hoverTrace.pointNumber).toEqual(1);
expect(hoverTrace.x).toEqual(2);
expect(hoverTrace.y).toEqual(3);

assertHoverLabelContent({
nums: '(2, 3)',
name: 'trace 0'
});
});
});
});
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.