Skip to content

Allow toggling legend to show just 1 series (or group) by double clicking #1432

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 8 commits into from
Mar 13, 2017
Merged

Allow toggling legend to show just 1 series (or group) by double clicking #1432

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
afeaf5e
Allow toggling legend to show just 1 series (or group) by double clic…
Mar 1, 2017
895ba89
Trace Isolation PR Feedback
Mar 3, 2017
afcf4f4
Sort out interaction between drag/edit and legend toggles.
Mar 3, 2017
c5a2ff8
Move DBLCLICKDELAY and make clicked series detection cleaner
Mar 4, 2017
fb37088
Test related fixes
Mar 4, 2017
1fe3b08
New legend interaction tests
Mar 6, 2017
edc30ac
Reduce test delays and fix uncovered bug
rpaskowitz Mar 11, 2017
a927b9f
Merge branch 'master' into legend_show_only
rpaskowitz Mar 13, 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
9 changes: 5 additions & 4 deletions src/components/dragelement/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ var Plotly = require('../../plotly');
var Lib = require('../../lib');

var constants = require('../../plots/cartesian/constants');

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

var dragElement = module.exports = {};

Expand All @@ -37,12 +37,13 @@ dragElement.unhoverRaw = unhover.raw;
* dx and dy are the net pixel offset of the drag,
* dragged is true/false, has the mouse moved enough to
* constitute a drag
* doneFn (optional) function(dragged, numClicks)
* doneFn (optional) function(dragged, numClicks, e)
* executed on mouseup, or mouseout of window since
* we don't get events after that
* dragged is as in moveFn
* numClicks is how many clicks we've registered within
* a doubleclick time
* e is the original event
* setCursor (optional) function(event)
* executed on mousemove before mousedown
* the purpose of this callback is to update the mouse cursor before
Expand All @@ -51,7 +52,7 @@ dragElement.unhoverRaw = unhover.raw;
dragElement.init = function init(options) {
var gd = Lib.getPlotDiv(options.element) || {},
numClicks = 1,
DBLCLICKDELAY = constants.DBLCLICKDELAY,
DBLCLICKDELAY = interactConstants.DBLCLICKDELAY,
startX,
startY,
newMouseDownTime,
Expand Down Expand Up @@ -136,7 +137,7 @@ dragElement.init = function init(options) {
numClicks = Math.max(numClicks - 1, 1);
}

if(options.doneFn) options.doneFn(gd._dragged, numClicks);
if(options.doneFn) options.doneFn(gd._dragged, numClicks, e);

if(!gd._dragged) {
var e2 = document.createEvent('MouseEvents');
Expand Down
153 changes: 127 additions & 26 deletions src/components/legend/draw.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,18 +21,23 @@ var Color = require('../color');
var svgTextUtils = require('../../lib/svg_text_utils');

var constants = require('./constants');
var interactConstants = require('../../constants/interactions');
var getLegendData = require('./get_legend_data');
var style = require('./style');
var helpers = require('./helpers');
var anchorUtils = require('./anchor_utils');

var SHOWISOLATETIP = true;
var DBLCLICKDELAY = interactConstants.DBLCLICKDELAY;

module.exports = function draw(gd) {
var fullLayout = gd._fullLayout;
var clipId = 'legend' + fullLayout._uid;

if(!fullLayout._infolayer || !gd.calcdata) return;

if(!gd._legendMouseDownTime) gd._legendMouseDownTime = 0;

var opts = fullLayout.legend,
legendData = fullLayout.showlegend && getLegendData(gd.calcdata, opts),
hiddenSlices = fullLayout.hiddenlabels || [];
Expand Down Expand Up @@ -325,9 +330,28 @@ module.exports = function draw(gd) {
xf = dragElement.align(newX, 0, gs.l, gs.l + gs.w, opts.xanchor);
yf = dragElement.align(newY, 0, gs.t + gs.h, gs.t, opts.yanchor);
},
doneFn: function(dragged) {
doneFn: function(dragged, numClicks, e) {
if(dragged && xf !== undefined && yf !== undefined) {
Plotly.relayout(gd, {'legend.x': xf, 'legend.y': yf});
} else {
var traces = [],
clickedTrace;
traces = fullLayout._infolayer.selectAll('g.traces').filter(function() {
var bbox = this.getBoundingClientRect();
return (e.clientX >= bbox.left && e.clientX <= bbox.right &&
e.clientY >= bbox.top && e.clientY <= bbox.bottom);
})[0];
if(traces.length > 0) {
clickedTrace = d3.select(traces[0]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

non-blocking, but I find it confusing to dig into d3's nested arrays directly. Preferable to leave traces as a selection, then use traces.size() instead of traces[0].length, and then you don't need to re-select it to make clickedTrace.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixing.

if(numClicks === 1) {
legend._clickTimeout = setTimeout(function() { handleClick(clickedTrace, gd, numClicks); }, DBLCLICKDELAY);
} else if(numClicks === 2) {
if(legend._clickTimeout) {
clearTimeout(legend._clickTimeout);
}
handleClick(clickedTrace, gd, numClicks);
}
}
}
}
});
Expand Down Expand Up @@ -395,9 +419,8 @@ function drawTexts(g, gd) {
}

function setupTraceToggle(g, gd) {
var hiddenSlices = gd._fullLayout.hiddenlabels ?
gd._fullLayout.hiddenlabels.slice() :
[];
var newMouseDownTime,
numClicks = 1;

var traceToggle = g.selectAll('rect')
.data([0]);
Expand All @@ -408,41 +431,119 @@ function setupTraceToggle(g, gd) {
.attr('pointer-events', 'all')
.call(Color.fill, 'rgba(0,0,0,0)');

traceToggle.on('click', function() {
if(gd._dragged) return;

var legendItem = g.data()[0][0],
fullData = gd._fullData,
trace = legendItem.trace,
legendgroup = trace.legendgroup,
traceIndicesInGroup = [],
tracei,
newVisible;
traceToggle.on('mousedown', function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we could reuse the dragelement abstraction here? This is a non- ⛔ comment of course.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Possibly - but this reminds me that we had better ensure that this change is compatible with the additional interactions the legend supports in {editable: true} config. For clarity, here's how that works currently and should continue to work:

  • clicking on legend symbols toggles the trace (so doubleclicking them should isolate the trace)
  • clicking on legend text, which outside of editable mode also toggles the trace, instead edits the text (so doubleclick should do nothing there)
  • dragging anywhere in the legend - symbol, text, or background/border - moves the legend and does not toggle anything.

@rpaskowitz you can convert the plot you're testing to editable by calling:

Plotly.newPlot(gd, gd.data, gd.layout, {editable: true})

newMouseDownTime = (new Date()).getTime();
if(newMouseDownTime - gd._legendMouseDownTime < DBLCLICKDELAY) {
// in a click train
numClicks += 1;
}
else {
// new click train
numClicks = 1;
gd._legendMouseDownTime = newMouseDownTime;
}
});
traceToggle.on('mouseup', function() {
if(gd._dragged || gd._editing) return;
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 like to get @etpinard 's take on this, but my feeling is we don't want to introduce another gd._flag - if anything we'd like to get rid of more of these as they're rather brittle and hard to maintain.

Also it's not clear to me that the right course of action is to quit the new handler if we're editing something else. I wonder if it would work to instead make a helper to ensure the edit box's blur handler gets called as it should? Honestly I don't understand why that's not already happening, but what if we just made a helper (as an export of svg_text_utils) like:

function blurEditable(gd) { d3.select(gd).selectAll('.editable').each(function() { this.blur(); }); }

If what the user did was clicked on the editable item accidentally, then wanted to move on to the interaction they really intended, quitting here will just add an extra click for them, and editable.blur() should be a simple removal of the edit box that doesn't affect the plot, so I see no harm in continuing the new interaction in that case.

I guess there could be weird side-effects if you type something in an edit box, then go click somewhere else, such that before the new handler can execute the plot has to redraw. The most extreme case I think would be if you have the legend off the right side, then you edit some legend text and make it longer, and we auto-expand the plot margin to accommodate it. That results in a very aggressive redraw that might actually delete and recreate the whole rest of the plot (not very d3-idiomatic, I know... we're working on it!). Would need a bit of playing around to see if that would cause any problems...

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'll admit that blocking other interactions while the edit to go on was more of a secondary benefit to introducing this - the primary motivation was that it served as a way to prevent the mouseup event from having any real effect when the click fired from the svgTextUtils.makeEditable otherwise when you clicked on the text, it would both make the field editable and toggle the series.

When the traceToggle use to use click before my change, I'm not sure what was preventing both click handlers from firing when the label was clicked.

If there's a suggestion on how to prevent both actions from firing, I can remove this flag and leave the question on how interactions should be dealt with generally to another ticket.

Copy link
Contributor

Choose a reason for hiding this comment

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

@alexcjohnson made some valid points above. But as editable: true is still a niche feature, I don't it's worth the time to improve its interactions with other components. So, I'm very much a fan of @rpaskowitz's gd._editing solution in this PR.

With regards to adding another gd._ keys: yes I agree, we should try to move away from this pattern in the long run. But with this test (that @rpaskowitz will need to patch 😉 ) keeps us honest.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good call @etpinard - we'll dig into this a little more with #1437 but @rpaskowitz lets keep it as is for this PR.

var legend = gd._fullLayout.legend;

if(Registry.traceIs(trace, 'pie')) {
var thisLabel = legendItem.label,
thisLabelIndex = hiddenSlices.indexOf(thisLabel);
if((new Date()).getTime() - gd._legendMouseDownTime > DBLCLICKDELAY) {
numClicks = Math.max(numClicks - 1, 1);
}

if(numClicks === 1) {
legend._clickTimeout = setTimeout(function() { handleClick(g, gd, numClicks); }, DBLCLICKDELAY);
} else if(numClicks === 2) {
if(legend._clickTimeout) {
clearTimeout(legend._clickTimeout);
}
handleClick(g, gd, numClicks);
}
});
}

function handleClick(g, gd, numClicks) {
if(gd._dragged || gd._editing) return;
var hiddenSlices = gd._fullLayout.hiddenlabels ?
gd._fullLayout.hiddenlabels.slice() :
[];

var legendItem = g.data()[0][0],
fullData = gd._fullData,
trace = legendItem.trace,
legendgroup = trace.legendgroup,
traceIndicesInGroup = [],
tracei,
newVisible;


if(numClicks === 1 && SHOWISOLATETIP && gd.data && gd._context.showTips) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We don't need gd.data here - SHOWZOOMOUTTIP needed it because you can't autoscale a plot with nothing on it, but there wouldn't be a legend anyway without data!

Lib.notifier('Double click on legend to isolate individual trace', 'long');
SHOWISOLATETIP = false;
} else {
SHOWISOLATETIP = false;
}
if(Registry.traceIs(trace, 'pie')) {
var thisLabel = legendItem.label,
thisLabelIndex = hiddenSlices.indexOf(thisLabel);

if(numClicks === 1) {
if(thisLabelIndex === -1) hiddenSlices.push(thisLabel);
else hiddenSlices.splice(thisLabelIndex, 1);
} else if(numClicks === 2) {
hiddenSlices = [];
gd.calcdata[0].forEach(function(d) {
if(thisLabel !== d.label) {
hiddenSlices.push(d.label);
}
});
if(gd._fullLayout.hiddenlabels && gd._fullLayout.hiddenlabels.length === hiddenSlices.length && thisLabelIndex === -1) {
hiddenSlices = [];
}
}

Plotly.relayout(gd, 'hiddenlabels', hiddenSlices);
} else {
var allTraces = [],
traceVisibility = [],
i;

Plotly.relayout(gd, 'hiddenlabels', hiddenSlices);
for(i = 0; i < fullData.length; i++) {
allTraces.push(i);
traceVisibility.push('legendonly');
}

if(legendgroup === '') {
traceIndicesInGroup = [trace.index];
traceVisibility[trace.index] = true;
} else {
if(legendgroup === '') {
traceIndicesInGroup = [trace.index];
} else {
for(var i = 0; i < fullData.length; i++) {
tracei = fullData[i];
if(tracei.legendgroup === legendgroup) {
traceIndicesInGroup.push(tracei.index);
}
for(i = 0; i < fullData.length; i++) {
tracei = fullData[i];
if(tracei.legendgroup === legendgroup) {
traceIndicesInGroup.push(tracei.index);
traceVisibility[allTraces.indexOf(i)] = true;
}
}
}

if(numClicks === 1) {
newVisible = trace.visible === true ? 'legendonly' : true;
Plotly.restyle(gd, 'visible', newVisible, traceIndicesInGroup);
} else if(numClicks === 2) {
var sameAsLast = true;
for(i = 0; i < fullData.length; i++) {
if(fullData[i].visible !== traceVisibility[i]) {
sameAsLast = false;
break;
}
}
if(sameAsLast) {
traceVisibility = true;
}
Plotly.restyle(gd, 'visible', traceVisibility, allTraces);
}
});
}
}

function computeTextDimensions(g, gd) {
Expand Down
6 changes: 5 additions & 1 deletion src/constants/interactions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,5 +14,9 @@ module.exports = {
* Timing information for interactive elements
*/
SHOW_PLACEHOLDER: 100,
HIDE_PLACEHOLDER: 1000
HIDE_PLACEHOLDER: 1000,

// ms between first mousedown and 2nd mouseup to constitute dblclick...
// we don't seem to have access to the system setting
DBLCLICKDELAY: 300
};
6 changes: 5 additions & 1 deletion src/lib/svg_text_utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -467,7 +467,8 @@ exports.makeEditable = function(context, _delegate, options) {
}

function appendEditable() {
var plotDiv = d3.select(Lib.getPlotDiv(that.node())),
var gd = Lib.getPlotDiv(that.node()),
plotDiv = d3.select(gd),
container = plotDiv.select('.svg-container'),
div = container.append('div');
div.classed('plugin-editable editable', true)
Expand All @@ -487,6 +488,7 @@ exports.makeEditable = function(context, _delegate, options) {
.text(options.text || that.attr('data-unformatted'))
.call(alignHTMLWith(that, container, options))
.on('blur', function() {
gd._editing = false;
that.text(this.textContent)
.style({opacity: 1});
var svgClass = d3.select(this).attr('class'),
Expand All @@ -503,13 +505,15 @@ exports.makeEditable = function(context, _delegate, options) {
})
.on('focus', function() {
var context = this;
gd._editing = true;
d3.select(document).on('mouseup', function() {
if(d3.event.target === context) return false;
if(document.activeElement === div.node()) div.node().blur();
});
})
.on('keyup', function() {
if(d3.event.which === 27) {
gd._editing = false;
that.style({opacity: 1});
d3.select(this)
.style({opacity: 0})
Expand Down
4 changes: 0 additions & 4 deletions src/plots/cartesian/constants.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,6 @@ module.exports = {
AX_ID_PATTERN: /^[xyz][0-9]*$/,
AX_NAME_PATTERN: /^[xyz]axis[0-9]*$/,

// ms between first mousedown and 2nd mouseup to constitute dblclick...
// we don't seem to have access to the system setting
DBLCLICKDELAY: 300,

// pixels to move mouse before you stop clamping to starting point
MINDRAG: 8,

Expand Down