Skip to content

Box points hover & select #2094

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

Merged
merged 19 commits into from
Oct 19, 2017
Merged

Box points hover & select #2094

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
cfc8725
lint (mostly var\n , -> var\n var blocks)
etpinard Oct 16, 2017
53c446b
revamp Box.calc and Box.plot handling of boxpoints
etpinard Oct 16, 2017
581f3fa
add lasso/select dragmode support to box traces
etpinard Oct 17, 2017
e59f283
flatten box test suite
etpinard Oct 17, 2017
5c58049
add some box hover tests
etpinard Oct 17, 2017
aef61ae
introduce box 'hoveron'
etpinard Oct 17, 2017
d352fa5
add 'text' attribute to box traces
etpinard Oct 17, 2017
6a878b0
more linting in Box.plot
etpinard Oct 17, 2017
0027b84
Merge branch 'master' into boxpoints-hover-select
etpinard Oct 17, 2017
a1ea1e8
fixup box hoveron description
etpinard Oct 18, 2017
6bfcbe0
make 'boxes+points' the box hoveron dflt
etpinard Oct 18, 2017
9ca410a
improve `boxpoints: false` default logic
etpinard Oct 18, 2017
7e44c9c
add `boxmode: 'overlay'` hover test case
etpinard Oct 18, 2017
f228a5e
aj-proof box hover logic
etpinard Oct 18, 2017
b4d8044
hide modebar select2d & lasso2d buttons for boxes w/o 'all' boxpoints
etpinard Oct 18, 2017
9e69900
rm useless 'c' value in scattercarper point event data
etpinard Oct 19, 2017
8901528
report coords in input-data space in pt event data
etpinard Oct 19, 2017
b66628e
introduce Axes.hoverLabelText
etpinard Oct 19, 2017
a3ec75a
add prefix box stats hover labels
etpinard Oct 19, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/traces/box/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ Box.setPositions = require('./set_positions');
Box.plot = require('./plot');
Box.style = require('./style');
Box.hoverPoints = require('./hover');
Box.selectPoints = require('./select');

Box.moduleType = 'trace';
Box.name = 'box';
Expand Down
9 changes: 6 additions & 3 deletions src/traces/box/plot.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ module.exports = function plot(gd, plotinfo, cdbox) {
var cd0 = d[0];
var t = cd0.t;
var trace = cd0.trace;
var sel = cd0.node3 = d3.select(this);

var group = (fullLayout.boxmode === 'group' && gd.numboxes > 1);
// box half width
Expand Down Expand Up @@ -80,7 +81,7 @@ module.exports = function plot(gd, plotinfo, cdbox) {
seed();

// boxes and whiskers
d3.select(this).selectAll('path.box')
sel.selectAll('path.box')
.data(Lib.identity)
.enter().append('path')
.style('vector-effect', 'non-scaling-stroke')
Expand All @@ -99,6 +100,7 @@ module.exports = function plot(gd, plotinfo, cdbox) {
Math.min(q1, q3) + 1, Math.max(q1, q3) - 1),
lf = valAxis.c2p(trace.boxpoints === false ? d.min : d.lf, true),
uf = valAxis.c2p(trace.boxpoints === false ? d.max : d.uf, true);

if(trace.orientation === 'h') {
d3.select(this).attr('d',
'M' + m + ',' + pos0 + 'V' + pos1 + // median line
Expand All @@ -118,7 +120,7 @@ module.exports = function plot(gd, plotinfo, cdbox) {

// draw points, if desired
if(trace.boxpoints) {
d3.select(this).selectAll('g.points')
sel.selectAll('g.points')
// since box plot points get an extra level of nesting, each
// box needs the trace styling info
.data(function(d) {
Expand Down Expand Up @@ -212,9 +214,10 @@ module.exports = function plot(gd, plotinfo, cdbox) {
.classed('point', true)
.call(Drawing.translatePoints, xa, ya);
}

// draw mean (and stdev diamond) if desired
if(trace.boxmean) {
d3.select(this).selectAll('path.mean')
sel.selectAll('path.mean')
.data(Lib.identity)
.enter().append('path')
.attr('class', 'mean')
Expand Down
57 changes: 57 additions & 0 deletions src/traces/box/select.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
/**
* Copyright 2012-2017, Plotly, Inc.
* All rights reserved.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/

'use strict';

var DESELECTDIM = require('../../constants/interactions').DESELECTDIM;

module.exports = function selectPoints(searchInfo, polygon) {
var cd = searchInfo.cd;
var xa = searchInfo.xaxis;
var ya = searchInfo.yaxis;
var trace = cd[0].trace;
var node3 = cd[0].node3;
var selection = [];
var i, j;

if(trace.visible !== true) return [];

if(polygon === false) {
for(i = 0; i < cd.length; i++) {
for(j = 0; j < (cd[i].pts || []).length; j++) {
// clear selection
cd[i].pts[j].dim = 0;
}
}
} else {
for(i = 0; i < cd.length; i++) {
for(j = 0; j < (cd[i].pts || []).length; j++) {
var pt = cd[i].pts[j];
var x = xa.c2p(pt.x);
var y = ya.c2p(pt.y);

if(polygon.contains([x, y])) {
selection.push({
pointNumber: pt.i,
x: pt.x,
y: pt.y
});
pt.dim = 0;
} else {
pt.dim = 1;
}
}
}
}

node3.selectAll('.point').style('opacity', function(d) {
return d.dim ? DESELECTDIM : 1;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that this style update isn't compatible with the current trace attribute set.

To do so, we would need to add arrayOk support for marker.color, marker.opacity and marker.symbol would be pretty easy to add after d352fa5

Copy link
Contributor Author

@etpinard etpinard Oct 17, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

... that is if no one finds too awkward to have both box-wide (e.g. marker.outliercolor) and per-sample-pt attributes in the same trace.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems reasonable. outliercolor is only for boxpoints: 'suspectedoutliers', and in that mode selecting would be a bit weird. (side note: outliercolor should probably be described better in attributes... and perhaps the logic in defaults cleaned up or amended so we don't have the outliercolor attributes in _fullData if boxpoints !== 'suspectedoutliers'

Begs the question though of what selection (or crossfilter) should do in modes other than boxpoints: 'all' when at least the points within the box are hidden - what does selection do now? I could imagine it being cool to still be able to select the points, and have them pop into view when you do (likewise for crossfilter)... but that might be too complicated, dunno.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Begs the question though of what selection (or crossfilter) should do in modes other than boxpoints: 'all' when at least the points within the box are hidden - what does selection do now?

I'd vote for hiding the lasso and select mode bar buttons whenever boxpoints !== 'all' - similar to how we currently hide them from scatter-line-only graphs here. But maybe that's just because I'm lazy. cc @chriddyp @cpsievert any thoughts on this?

and perhaps the logic in defaults cleaned up or amended so we don't have the outliercolor attributes in _fullData if boxpoints !== 'suspectedoutliers'

I believe that's done already.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd vote for hiding the lasso and select mode bar buttons whenever boxpoints !== 'all'

We can certainly start that way anyhow, that's fine for this PR, we can revisit if there's interest later.

I believe that's done already.

It's messy logic - but that's actually the second coerce of one of them so the easy thing to do would be to delete them from the full trace if they're not needed. I guess the reason it's done this way is so you can set the default boxpoints to 'suspectedoutliers' if either of the outliercolor values is a real color - stricter than just existing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ha I see. I've been wanting to replace Lib.coerce2 with Lib.validate (which doesn't fill in fullData). I'll do this in this PR 🔨

Copy link
Contributor Author

@etpinard etpinard Oct 18, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

🔪ing Lib.coerce2 turned out to be a little trickier than I thought - and than I want to do in this PR. In brief, we can just replace coerce2 by validate, validate return false whenever an value isn't set while coerce2 returns the default value.

So, here's a quick 🛠️ and 🔒 in 9ca410a

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd vote for hiding the lasso and select mode bar buttons whenever boxpoints !== 'all'
We can certainly start that way anyhow, that's fine for this PR, we can revisit if there's interest later.

that's in b4d8044

});

return selection;
};
55 changes: 54 additions & 1 deletion test/jasmine/tests/select_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -848,7 +848,60 @@ describe('Test select box and lasso per trace:', function() {
]);
assertRanges([[1.66, 3.59], [0.69, 2.17]]);
},
null, BOXEVENTS, 'bar select'
null, BOXEVENTS, 'histogram select'
);
})
.catch(fail)
.then(done);
});

it('should work for box traces', function(done) {
var assertPoints = makeAssertPoints(['curveNumber', 'y']);
var assertRanges = makeAssertRanges();
var assertLassoPoints = makeAssertLassoPoints();

var fig = Lib.extendDeep({}, require('@mocks/box_grouped'));
fig.data.forEach(function(trace) {
trace.boxpoints = 'all';
});
fig.layout.dragmode = 'lasso';
fig.layout.width = 600;
fig.layout.height = 500;
addInvisible(fig);

Plotly.plot(gd, fig)
.then(function() {
return _run(
[[200, 200], [400, 200], [400, 350], [200, 350], [200, 200]],
function() {
assertPoints([
[0, 0.2], [0, 0.3], [0, 0.5], [0, 0.7],
[1, 0.2], [1, 0.5], [1, 0.7], [1, 0.7],
[2, 0.3], [2, 0.6], [2, 0.6]
]);
assertLassoPoints([
['day 1', 'day 2', 'day 2', 'day 1', 'day 1'],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alexcjohnson Is this the desired behavior, or did an extra c2d conversion get forgotten? I believe outputting the linearized continuous category equivalent would make sense here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

... we might have to wait for v2 though.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess c format would make sense for applications that want to perhaps overlay something on the selected points... but I'd think the d value is what the user would expect (and what they would generally want to use), wouldn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My argument for c:

d is lossy for categories. Converting c to d can easily be done by the user, the opposite cannot (unless we officially expose ax.d2c and friends).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So do you think the x/y coordinate should be converted to d too here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

d is lossy for categories.

These values had really better correspond to the actual data, not offset or jittered values... It's not important for category axes, but for non-lossy axes it is, otherwise you'll be pointed to the wrong data value! So in the end I don't think that this conversion is lossy, even though in general you're right.

Converting c to d can easily be done by the user, the opposite cannot (unless we officially expose ax.d2c and friends).

How do you convert c to d for categories without ax.c2d? It depends on all sorts of details of the graph. Granted enough people use those methods already that I think we have to consider them exposed...

So do you think the x/y coordinate should be converted to d too here?

Good catch - yeah, I guess so. Everything we report back out with events should be d, I would think. That's a pretty sweeping statement though, so it's worth discussing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How do you convert c to d for categories without ax.c2d?

Yeah, I guess that's kinda hard to in general. Put that in the 9am brain-cramp bin.

Everything we report back out with events should be d

I think so too. We should at the very least report everything in the same coordinate space. Right now, lasso coords are in d but point coords in are c even for scatter - which is ridiculous. I would consider this a bug, do you agree?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would consider this a bug, do you agree?

🐞
I hope not too many people depend on the current behavior with date axes, but yeah, I think we should unify on d ASAP. I guess the lesson is make sure there are tests on non-numeric axes whenever we add new features like this... otherwise the c/d distinction goes unnoticed.

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 8901528

[0.71, 0.71, 0.1875, 0.1875, 0.71]
]);
},
null, LASSOEVENTS, 'box lasso'
);
})
.then(function() {
return Plotly.relayout(gd, 'dragmode', 'select');
})
.then(function() {
return _run(
[[200, 200], [400, 350]],
function() {
assertPoints([
[0, 0.2], [0, 0.3], [0, 0.5], [0, 0.7],
[1, 0.2], [1, 0.5], [1, 0.7], [1, 0.7],
[2, 0.3], [2, 0.6], [2, 0.6]
]);
assertRanges([['day 1', 'day 2'], [0.1875, 0.71]]);
},
null, BOXEVENTS, 'box select'
);
})
.catch(fail)
Expand Down