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

rpaskowitz · 2017-03-01T22:27:41Z

I find myself often wanting to look at just 1 series out of many. This PR enables double clicking on a legend entry to have just that series (or legend group) shown. It introduces a delay (DBLCLICKDELAY) on the single-click case, otherwise there was some flashing as the click handler fired, and then the double click handler.

TODO: Tests, if this otherwise looks good.

…king

alexcjohnson · 2017-03-02T05:05:18Z

src/components/legend/constants.js

@@ -12,5 +12,6 @@ module.exports = {
    scrollBarWidth: 4,
    scrollBarHeight: 20,
    scrollBarColor: '#808BA4',
-    scrollBarMargin: 4
+    scrollBarMargin: 4,
+    DBLCLICKDELAY: 300


To avoid duplication we should probably put this (and a few other things) into constants/interactions and reference it from legend and dragelement - which for obsolete reasons currently gets these constants out of cartesian/constants

alexcjohnson · 2017-03-02T05:22:36Z

I like it, nice start @rpaskowitz !

We need a way to undo this isolation. Can you make that happen on doubleclicking the same trace again?

alexcjohnson · 2017-03-02T05:25:20Z

src/components/legend/draw.js

            newVisible = trace.visible === true ? 'legendonly' : true;
            Plotly.restyle(gd, 'visible', newVisible, traceIndicesInGroup);
+        } else if(numClicks === 2) {
+            Plotly.restyle(gd, 'visible', true, traceIndicesInGroup);
+            Plotly.restyle(gd, 'visible', 'legendonly', otherTraces);


Can you work this into a single call Plotly.restyle(gd, 'visible', [true, 'legendonly', ...], allTraces);?

In a way this may make things easier, no more array splicing, I can just build up a traceVisibility array as I iterate.

Yes. Please convert this to a single restyle call 🐎

rpaskowitz · 2017-03-02T05:25:30Z

src/components/legend/draw.js

-            var thisLabel = legendItem.label,
-                thisLabelIndex = hiddenSlices.indexOf(thisLabel);
+        if(numClicks === 1) {
+            legend._clickTimeout = setTimeout(function() { handleClick(g, gd, numClicks); }, 300);


This should be DBLCLICKDELAY, will correct when refactoring where that's defined.

rpaskowitz · 2017-03-02T05:30:24Z

Re: duplication of DBLCLICKDELAY, makes sense. It being in cartesian seemed a bit out of place. Will put in in interactions.

Re: undo isolation - if we define "undo" to mean "enable all traces", it seems easy enough. If 'undo' were "restore previously visible/hidden series", will take a bit more work. I also anticipate the implementation being such that if only 1 series is visible, regardless of if it got that way through a double click or a manual disabling of all other series, that a double click on that one series would result in enabling all hidden series.

alexcjohnson · 2017-03-02T05:32:18Z

In other places we tell people about doubleclick opportunities through Lib.notifier (eg 'Double-click to<br>zoom back out') - perhaps we should do that here the first time someone single-clicks a legend item?
@etpinard thoughts?

alexcjohnson · 2017-03-02T05:34:36Z

if we define "undo" to mean "enable all traces", it seems easy enough.

Hah, exactly this question occurred to me too after posting my comment 🥇
Yes, lets just enable all traces. Having it be precisely an undo would not only be a lot more work, I think it would actually be more confusing to users.

etpinard · 2017-03-02T16:22:20Z

src/components/legend/draw.js

@@ -408,41 +410,98 @@ function setupTraceToggle(g, gd) {
        .attr('pointer-events', 'all')
        .call(Color.fill, 'rgba(0,0,0,0)');

-    traceToggle.on('click', function() {
+
+    traceToggle.on('mousedown', function() {


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

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})

etpinard · 2017-03-02T16:39:54Z

@rpaskowitz first off, thanks very much for this PR! Judging by the activity on #674, this will make a lot of people happy.

Re: undo isolation - if we define "undo" to mean "enable all traces", it seems easy enough.

Like @alexcjohnson, I agree with ⬆️ 100%.

Lib.notifier (eg 'Double-click to<br>zoom back out') - perhaps we should do that here the first time someone single-clicks a legend item?

I'd be ok with adding a notifier as this case is very similar to the zoom / zoom back case. 👍

That said, I'm not a big fan that zoom / zoom back notifier, as it only serves new users and experienced users probably get annoyed by it. I think a help mode bar button would be a better alternative, but that's a different story. 📚

alexcjohnson · 2017-03-02T16:46:22Z

That said, I'm not a big fan that zoom / zoom back notifier, as it only serves new users and experienced users probably get annoyed by it.

I agree, it's a bit of a blunt instrument but it's what we have for the moment. Lets put it in for now but I'll make an issue to discuss alternatives.

- Show Lib.notify on first single-click toggle - Refactor DBLCLICKDELAY into interaction constants - Double click on the only visible trace (or group) re-enables all traces - Single call to restyle TODO: Tests, fix interaction on editable: true

rpaskowitz · 2017-03-03T02:14:47Z

(Almost) all PR comments addressed. There are problems with the interaction when editable: true that I haven't had a chance to look into yet. When it's enabled, neither single nor double click interactions work at the moment.

Introduces a global _editing flag to be able to block other interactions while edit is in progress. Could be one solution for plotly#1437. This makes it more clear that there should be some refactor to consolidate the double click behavior, since when the drag layer is on, that code path's click detection is used. The other path needs to remain since the drag layer doesn't exist when editable: false, but it's now easier to picture a world where these two paths converge.

rpaskowitz · 2017-03-03T03:43:43Z

alexcjohnson · 2017-03-03T03:58:26Z

src/components/legend/draw.js

@@ -455,6 +457,13 @@ function handleClick(g, gd, numClicks) {
        tracei,
        newVisible;

+
+    if(numClicks === 1 && SHOWISOLATETIP && gd.data && gd._context.showTips) {


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!

alexcjohnson · 2017-03-03T04:01:10Z

src/components/legend/draw.js

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


DBLCLICKDELAY?

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

alexcjohnson · 2017-03-03T04:27:07Z

src/components/legend/draw.js

+                            clickedTrace = tracei;
+                            break;
+                        }
+                    }


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()) { ...

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()

alexcjohnson · 2017-03-03T05:00:24Z

src/components/legend/draw.js

@@ -426,7 +448,7 @@ function setupTraceToggle(g, gd) {
        }
    });
    traceToggle.on('mouseup', function() {
-        if(gd._dragged) return;
+        if(gd._dragged || gd._editing) return;


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...

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.

@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.

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

rpaskowitz · 2017-03-04T03:56:49Z

Current PR feedback should all be covered now. Barring any other comments, I'll be able to fix broken tests (likely due to change from 'click' to 'mousedown+mouseup'), and add new tests for the double click behaviour.

- Update Purge() to account for new field. - Update import for DBLCLICKDELAY - Update tests for mousedown+mouseup and DBLCLICKDELAY before toggle fades.

alexcjohnson · 2017-03-10T14:17:54Z

src/components/legend/draw.js

+                            e.clientY >= bbox.top && e.clientY <= bbox.bottom);
+                    })[0];
+                    if(traces.length > 0) {
+                        clickedTrace = d3.select(traces[0]);


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.

alexcjohnson · 2017-03-10T15:40:51Z

test/jasmine/tests/legend_test.js

+            });
+        });
+    });
+});


These tests are great, and they lock down a lot more behavior than just what you added, thanks! 🍻

Can we reduce the delays a bit? I guess the DBLCLICKDELAY * 2 delays just need to be > DBLCLICKDELAY, can we drop them to DBLCLICKDELAY + 20 or something? And the delays after a double click can presumably be very short (perhaps 20)?

Non-blocking, but you might also want to make helpers to reduce repetition generating the mouse events. Either click(element) and doubleclick(element) or click(element, count).

Oh, but this one is blocking: we have click and double_click helpers in test assets - which aren't useful here because they're linked to mouse coordinates rather than an element - but I see that double_click uses DBLCLICKDELAY so needs its reference updated. I guess the tests that use it will still pass with no delay, but not exactly what we wanted them to be testing...

Lowered the delays which uncovered a bug in the implementation, so both have been fixed.

Updated to DBLCLICKDELAY done in the double_click asset.

Didn't introduce any new click helpers for now, because those existing assets will be cause for confusion. Probably worth a general refactor as there are a few dozen mousedown+mouseup combo clicks across other tests.

alexcjohnson · 2017-03-10T15:44:41Z

@rpaskowitz apologies for the delay reviewing your fixes. I think the only blocking issues now are the double_click helper hiding in test assets and seeing if you can reduce the test delays, then we'll be ready to 💃

Fixup double_click asset Cleanup legend item selection code in editable mode

alexcjohnson · 2017-03-12T06:01:37Z

@rpaskowitz the present CI test failure is due to #1455 - if you merge master into this branch it should work again.

alexcjohnson · 2017-03-13T16:47:27Z

Looks great to me! 💃 (which at Plotly means "approved to merge" in case it's not clear!)

fixxxr · 2017-03-21T07:26:47Z

This is great news. Finally this nice feature on this awesome library.

How can we use the new feature on existing charts.

Thanks

rpaskowitz · 2017-03-21T18:40:04Z

If you upgrade to 1.25.0 released yesterday you should see it on your charts. For example, you can see it in action on the first chart at https://plot.ly/javascript/

Allow toggling legend to show just 1 series (or group) by double clic…

afeaf5e

…king

rpaskowitz mentioned this pull request Mar 1, 2017

[Enhancement] Legend and traces toggling #674

Closed

alexcjohnson reviewed Mar 2, 2017

View reviewed changes

rpaskowitz commented Mar 2, 2017

View reviewed changes

etpinard added status: in progress feature something new labels Mar 2, 2017

etpinard reviewed Mar 2, 2017

View reviewed changes

This was referenced Mar 2, 2017

Interaction discoverability #1436

Closed

Editable text should stop being edited on other interactions #1437

Closed

Legend drag gets misaligned in the bottom 2/3 of the plot #1438

Closed

Trace Isolation PR Feedback

895ba89

- Show Lib.notify on first single-click toggle - Refactor DBLCLICKDELAY into interaction constants - Double click on the only visible trace (or group) re-enables all traces - Single call to restyle TODO: Tests, fix interaction on editable: true

alexcjohnson reviewed Mar 3, 2017

View reviewed changes

Move DBLCLICKDELAY and make clicked series detection cleaner

c5a2ff8

Test related fixes

fb37088

- Update Purge() to account for new field. - Update import for DBLCLICKDELAY - Update tests for mousedown+mouseup and DBLCLICKDELAY before toggle fades.

rpaskowitz force-pushed the legend_show_only branch from d671811 to fb37088 Compare March 4, 2017 05:01

New legend interaction tests

1fe3b08

etpinard added this to the v1.25.0 milestone Mar 6, 2017

alexcjohnson reviewed Mar 10, 2017

View reviewed changes

Reduce test delays and fix uncovered bug

edc30ac

Fixup double_click asset Cleanup legend item selection code in editable mode

Merge branch 'master' into legend_show_only

a927b9f

etpinard added status: reviewable and removed status: in progress labels Mar 13, 2017

alexcjohnson merged commit d5303ab into plotly:master Mar 13, 2017

rpaskowitz deleted the legend_show_only branch April 14, 2017 01:39

etpinard mentioned this pull request Apr 26, 2017

Decreased show/hide trace performance #1623

Closed

contactsandygo mentioned this pull request Jun 13, 2018

Traces visibility is out of sync with corresponding legend state on mouse click #2733

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

rpaskowitz commented Mar 1, 2017

alexcjohnson Mar 2, 2017

alexcjohnson commented Mar 2, 2017

alexcjohnson Mar 2, 2017

rpaskowitz Mar 2, 2017

etpinard Mar 2, 2017

rpaskowitz Mar 2, 2017

rpaskowitz commented Mar 2, 2017

alexcjohnson commented Mar 2, 2017

alexcjohnson commented Mar 2, 2017

etpinard Mar 2, 2017

alexcjohnson Mar 2, 2017

etpinard commented Mar 2, 2017 •

edited

Loading

alexcjohnson commented Mar 2, 2017

rpaskowitz commented Mar 3, 2017

rpaskowitz commented Mar 3, 2017

alexcjohnson Mar 3, 2017

alexcjohnson Mar 3, 2017

rpaskowitz Mar 3, 2017

alexcjohnson Mar 3, 2017

rpaskowitz Mar 3, 2017

alexcjohnson Mar 3, 2017

rpaskowitz Mar 3, 2017

etpinard Mar 3, 2017

alexcjohnson Mar 3, 2017

rpaskowitz commented Mar 4, 2017

alexcjohnson Mar 10, 2017

rpaskowitz Mar 11, 2017

alexcjohnson Mar 10, 2017

rpaskowitz Mar 11, 2017

alexcjohnson commented Mar 10, 2017

alexcjohnson commented Mar 12, 2017

alexcjohnson commented Mar 13, 2017

fixxxr commented Mar 21, 2017

rpaskowitz commented Mar 21, 2017

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

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

Conversation

rpaskowitz commented Mar 1, 2017

Choose a reason for hiding this comment

alexcjohnson commented Mar 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rpaskowitz commented Mar 2, 2017

alexcjohnson commented Mar 2, 2017

alexcjohnson commented Mar 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard commented Mar 2, 2017 • edited Loading

alexcjohnson commented Mar 2, 2017

rpaskowitz commented Mar 3, 2017

rpaskowitz commented Mar 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rpaskowitz commented Mar 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcjohnson commented Mar 10, 2017

alexcjohnson commented Mar 12, 2017

alexcjohnson commented Mar 13, 2017

fixxxr commented Mar 21, 2017

rpaskowitz commented Mar 21, 2017

etpinard commented Mar 2, 2017 •

edited

Loading