Contour legend #2891

alexcjohnson · 2018-08-09T21:18:55Z

Addresses most of #2880, all but the question about colorscales avoiding the legend.

Any contour (or contourcarpet) trace can now appear in the legend
Fills or lines with colorscales get a matching gradient in the legend
Drawing.gradient is extended to support arbitrary stops, as used by our built-in and custom colorscales. (Side note: we could use this to simplify continuous colorbar drawing, ie instead of making lots of little constant-color rectangles just make one rectangle with the colorscale applied as a gradient. I did not do that in this PR. But doing so would probably make it easier to finally close colorbar draws outside the bounds #970)
Two more bugs showed up while I was working on this, and I fixed them both here:
- It was not possible to make two contour traces both with heatmap coloring. I moved heatmap coloring into the contour group, rather than at the root of the contour layer, to solve this.
- Hiding and reshowing a contour trace (which is now easy with them all available in the legend) would result in the wrong ordering. Particularly problematic if you have one trace with colored fills and various others in front of it. Fixed this (for contour and contourcarpet) by, surprise surprise, moving to more standard d3 enter/exit/each(order) logic.
Moved the legend symbols for traces with fills up a bit - centered if fill-only, and slightly below center if fill-and-line, but no change if there are also markers or text.

etpinard

Great PR 💪

I misread parts of #2880, where I thought the individual contour levels would optionally get corresponding legend items too. Was this ever discussed?

Thanks for fixing those heatmap/contour ordering bugs that I probably encountered before in #2574, but didn't bother fixing. My bad.

I made a few comments. I found one potential red flag: the additional showlegend: false in colorscale_opacity,json and contour_scatter.json.

etpinard · 2018-08-10T18:15:44Z

src/components/legend/style.js

+        var markersOrText = subTypes.hasMarkers(trace) || subTypes.hasText(trace);
+        var anyFill = showFill || showGradientFill;
+        var anyLine = showLine || showGradientLine;
+        var pathStart = (markersOrText || !anyFill) ? 'M5,0' :


nice touch!

etpinard · 2018-08-10T18:19:50Z

src/traces/contour/plot.js

@@ -28,32 +27,35 @@ var constants = require('./constants');
 var costConstants = constants.LABELOPTIMIZER;

 exports.plot = function plot(gd, plotinfo, cdcontours, contourLayer) {
-    var uidLookup = getUidsFromCalcData(cdcontours);
+    var contours = contourLayer.selectAll('g.contour')


Ah yeah. Way better.

By the looks of it, heatmap and carpet probably suffer from similar bugs:

From which we could 🔪 getUidsFromCalcdata entirely:

No need to do this in this PR of course, but opening a new issue about it would be nice.

Good call -> #2907

etpinard · 2018-08-10T18:38:42Z

test/jasmine/tests/contour_test.js

@@ -439,21 +441,21 @@ describe('contour plotting and editing', function() {
        .then(function() {
            _assert({
                hmIndex: 0,
-                contoursIndex: 1
+                contoursIndex: 3


Oh ok, that's tricky. Now that <.hm> is inside <g.contour>, this test is a little tricky to interpret.

Maybe we could instead assert:

Array.prototype.slice.call(contourPlot.children).map(n => n.classList[0]) // to check ordering Array.prototype.slice.call(contourPlot.children).map(n => n.children.length) // to check the number of children

Since this is really about layer ordering, which is a consistently testable feature in SVG and likely to be useful elsewhere in our tests, I added a generic custom assertion for this. See what you think e25b9ff

See what you think e25b9ff

💯

etpinard · 2018-08-10T18:39:04Z

test/jasmine/tests/contour_test.js

@@ -501,4 +503,34 @@ describe('contour plotting and editing', function() {
        .catch(fail)
        .then(done);
    });
+
+    it('keeps the correct ordering after hide and show', function(done) {


Nice test. Thanks!

etpinard · 2018-08-10T18:39:23Z

test/jasmine/tests/contour_test.js

+            expect(getIndices()).toEqual([0, 1]);
+        })
+        .catch(fail)
+        .then(done);


Oh would you mind s/fail/failTest in this file?

failTest in contour & carpet -> 7843d83

etpinard · 2018-08-10T18:46:00Z

test/jasmine/tests/carpet_test.js

+            expect(getIndices()).toEqual([1, 2]);
+        })
+        .catch(fail)
+        .then(done);


one more s/fail/failTest - we must be getting pretty close 🤞

etpinard · 2018-08-10T18:48:05Z

src/components/legend/style.js

+        function lineGradient(s) {
+            if(s.size()) {
+                var gradientID = 'legendline-' + trace.uid;
+                Drawing.lineGroupStyle(s);


We should double-check that this works ok with line.dash and other line stylings.

Good call, added to a mock in 73f279b

etpinard · 2018-08-10T18:50:42Z

src/traces/contour/style_defaults.js

@@ -27,6 +27,10 @@ module.exports = function handleStyleDefaults(traceIn, traceOut, coerce, layout,
    }

    if(coloring !== 'none') {
+        // plots/plots always coerces showlegend to true, but in this case
+        // we default to false and (by default) show a colorbar instead
+        if(traceIn.showlegend !== true) traceOut.showlegend = false;


Ah nice ♻️ . This gets used in

etpinard · 2018-08-10T18:53:52Z

test/image/mocks/contour_legend.json

@@ -0,0 +1,54 @@
+{
+  "data":[{


Nice mock! Could we lock down the contourcarpet legend behavior in a mock as well?

... and for histogram2dcontour.

Good catch, histogram2dcontour didn't get this functionality before -> added and tested in 73f279b

and contourcarpet colored contours in 74e7f84

etpinard · 2018-08-10T19:12:28Z

and referencing #2642 where work in this PR could somewhat easily be extended to heatmap, histogram2d and surface traces.

and add assertNodeOrder as a custom assertion

…m2dcontour

alexcjohnson · 2018-08-15T19:21:01Z

src/components/legend/helpers.js

@@ -9,16 +9,6 @@

 'use strict';

-exports.legendGetsTrace = function legendGetsTrace(trace) {


This was used in getLegendData and in legendDefaults, but even before one of them overrode half the logic, and now that logic has diverged further, so it didn't make sense to pull it out.

nice 🔪 job here

alexcjohnson · 2018-08-15T19:24:32Z

src/plots/plots.js

            coerce('showlegend');
            coerce('legendgroup');
        }
+        else {
+            traceOut._dfltShowLegend = false;


For the record: I added _dfltShowLegend because there's otherwise no robust way to tell from just traceIn and traceOut what the default is... so there would be some situations where we would NOT show a legend (eg scatter + colored contour) but then setting showlegend: false explicitly in the trace that already defaults to showlegend: false would make the legend appear 🤔

yeah, that's what I expected. I don't think there's a nicer way to do this than what you coded in this commit 👌

etpinard · 2018-08-15T19:42:30Z

src/plots/layout_attributes.js

+            'a) Two or more traces would by default be shown in the legend.',
+            'b) One pie trace is shown in the legend.',
+            'c) One trace is explicitly given with `showlegend: true`.'
+        ].join(' ')


thanks for 📚

etpinard · 2018-08-15T19:52:36Z

Thanks for restoring backward-compatibility in 1d05081

All my blocking comments have been addressed, so 💃

We should still open an issue about (potential) heatmap and carpet layer ordering bugs as mentioned in #2891 (comment)

alexcjohnson · 2018-08-15T20:04:56Z

I misread parts of #2880, where I thought the individual contour levels would optionally get corresponding legend items too. Was this ever discussed?

Ah no, that's not what we're after here. Interesting though, kind of an alternative a colorbar, I guess? Related to #1747 and #1785

alexcjohnson added 4 commits August 8, 2018 22:26

extend Drawing.gradient and use it for contour legends

44aa70e

fix dynamic contour ordering and multi-heatmap-contour plots

597a845

better centering of legend items with fill

7f0db7c

fix show/hide ordering for contourcarpet matching contour

0f532f3

alexcjohnson added bug something broken feature something new status: reviewable labels Aug 9, 2018

alexcjohnson added this to the v1.40.0 milestone Aug 9, 2018

etpinard reviewed Aug 10, 2018

View reviewed changes

alexcjohnson added 5 commits August 14, 2018 12:01

fail->failTest in 2 more test files

7843d83

clearer contour heatmap ordering test

e25b9ff

and add assertNodeOrder as a custom assertion

add line styling to contour_legend test image and fix it for histogra…

73f279b

…m2dcontour

update airfoil mock to show contourcarpet colored legend styling

74e7f84

update showlegend logic for better backward compatibility

1d05081

alexcjohnson commented Aug 15, 2018

View reviewed changes

etpinard reviewed Aug 15, 2018

View reviewed changes

alexcjohnson mentioned this pull request Aug 15, 2018

heatmap ordering bug #2907

Closed

alexcjohnson merged commit 6f35b5e into master Aug 15, 2018

alexcjohnson deleted the contour-legend branch August 15, 2018 20:07

etpinard mentioned this pull request Aug 15, 2018

colorbar draws outside the bounds #970

Closed

alexcjohnson mentioned this pull request Aug 15, 2018

Contour plots with no colorscale should go in the legend #2880

Closed

etpinard mentioned this pull request Aug 16, 2018

Fix toSVG for graphs that call Drawing.gradient #2914

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Contour legend #2891

Contour legend #2891

alexcjohnson commented Aug 9, 2018

etpinard left a comment •

edited

Loading

etpinard Aug 10, 2018

etpinard Aug 10, 2018

etpinard Aug 10, 2018

alexcjohnson Aug 15, 2018

etpinard Aug 10, 2018 •

edited

Loading

alexcjohnson Aug 15, 2018

etpinard Aug 15, 2018

etpinard Aug 10, 2018

etpinard Aug 10, 2018

alexcjohnson Aug 15, 2018

etpinard Aug 10, 2018

etpinard Aug 10, 2018

alexcjohnson Aug 15, 2018

etpinard Aug 10, 2018

etpinard Aug 10, 2018

etpinard Aug 10, 2018

alexcjohnson Aug 15, 2018

alexcjohnson Aug 15, 2018

etpinard commented Aug 10, 2018

alexcjohnson Aug 15, 2018

etpinard Aug 15, 2018

alexcjohnson Aug 15, 2018

alexcjohnson Aug 15, 2018

etpinard Aug 15, 2018

etpinard Aug 15, 2018

etpinard commented Aug 15, 2018

alexcjohnson commented Aug 15, 2018

		@@ -9,16 +9,6 @@

		'use strict';

		exports.legendGetsTrace = function legendGetsTrace(trace) {

Contour legend #2891

Contour legend #2891

Conversation

alexcjohnson commented Aug 9, 2018

etpinard left a comment • edited Loading

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

etpinard Aug 10, 2018 • edited Loading

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

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

etpinard commented Aug 10, 2018

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

etpinard commented Aug 15, 2018

alexcjohnson commented Aug 15, 2018

etpinard left a comment •

edited

Loading

etpinard Aug 10, 2018 •

edited

Loading