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 1 commit
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
5 changes: 3 additions & 2 deletions src/components/dragelement/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 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
27 changes: 25 additions & 2 deletions src/components/legend/draw.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -329,9 +329,31 @@ 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;
fullLayout._infolayer.selectAll('g.traces').each(function() { d3.select(this).call(function() { traces.push(this); }); });
for(var i = 0; i < traces.length; i++) {
var tracei = traces[i];
var p = tracei[0][0].getBoundingClientRect();
if(e.clientX >= p.left && e.clientX <= p.right && e.clientY >= p.top && e.clientY <= p.bottom) {
clickedTrace = tracei;
break;
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a funny pattern... the two this's got me totally confused at first. Can't you just do something like:

clickedTrace = 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);
});
if(clickedTrace.size()) {
    ...

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 wrestled with this piece for a while. Your way definitely looks cleaner, but I think may need a slight adjustment since what comes back from the selectAll isn't the full object and would still need to be passed to d3.select() in order to get the right g that works within handleClick()

if(clickedTrace) {
if(numClicks === 1) {
legend._clickTimeout = setTimeout(function() { handleClick(clickedTrace, gd, numClicks); }, 300);
Copy link
Collaborator

Choose a reason for hiding this comment

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

DBLCLICKDELAY?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Caught again. Didn't have it in the right scope, hardcoded to unblock for a bit and forgot all about it.

} else if(numClicks === 2) {
if(legend._clickTimeout) {
clearTimeout(legend._clickTimeout);
}
handleClick(clickedTrace, gd, numClicks);
}
}
}
}
});
Expand Down Expand Up @@ -426,7 +448,7 @@ function setupTraceToggle(g, gd) {
}
});
traceToggle.on('mouseup', function() {
if(gd._dragged) return;
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((new Date()).getTime() - gd._legendMouseDownTime > DBLCLICKDELAY) {
Expand All @@ -445,6 +467,7 @@ function setupTraceToggle(g, gd) {
}

function handleClick(g, gd, numClicks) {
if(gd._dragged || gd._editing) return;
var hiddenSlices = gd._fullLayout.hiddenlabels ?
gd._fullLayout.hiddenlabels.slice() :
[];
Expand Down
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