Skip to content
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.

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

Contour legend #2891

Merged
merged 9 commits into from
Aug 15, 2018
7 changes: 7 additions & 0 deletions src/components/drawing/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -453,6 +453,13 @@ drawing.singlePointStyle = function(d, sel, trace, fns, gd) {
if(gradientType) perPointGradient = true;
else gradientType = markerGradient && markerGradient.type;

// for legend - arrays will propagate through here, but we don't need
// to treat it as per-point.
if(Array.isArray(gradientType)) {
gradientType = gradientType[0];
if(!gradientInfo[gradientType]) gradientType = 0;
}

if(gradientType && gradientType !== 'none') {
var gradientColor = d.mgc;
if(gradientColor) perPointGradient = true;
Expand Down
3 changes: 2 additions & 1 deletion src/components/legend/style.js
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,8 @@ module.exports = function style(s, gd) {
showGradientLine = true;
}
else {
showLine = coloring === 'none' || contours.showlines;
showLine = coloring === 'none' || coloring === 'heatmap' ||
contours.showlines;
}

if(contours.type === 'constraint') {
Expand Down
43 changes: 22 additions & 21 deletions src/traces/contour/plot.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ var Drawing = require('../../components/drawing');
var svgTextUtils = require('../../lib/svg_text_utils');
var Axes = require('../../plots/cartesian/axes');
var setConvert = require('../../plots/cartesian/set_convert');
var getUidsFromCalcData = require('../../plots/get_data').getUidsFromCalcData;

var heatmapPlot = require('../heatmap/plot');
var makeCrossings = require('./make_crossings');
Expand All @@ -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')
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah yeah. Way better.

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

image

From which we could 🔪 getUidsFromCalcdata entirely:

image

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good call -> #2907

.data(
cdcontours.map(function(d) { return d[0]; }),
function(cd) { return cd.trace.uid; }
);

contourLayer.selectAll('g.contour').each(function(d) {
if(!uidLookup[d.trace.uid]) {
d3.select(this).remove();
}
});
contours.exit().remove();

for(var i = 0; i < cdcontours.length; i++) {
plotOne(gd, plotinfo, cdcontours[i], contourLayer);
}
contours.enter().append('g')
.classed('contour', true);

contours.each(function(cd) {
plotOne(gd, plotinfo, cd, d3.select(this));
})
.order();
};

function plotOne(gd, plotinfo, cd, contourLayer) {
var trace = cd[0].trace;
var x = cd[0].x;
var y = cd[0].y;
function plotOne(gd, plotinfo, cd, plotGroup) {
var trace = cd.trace;
var x = cd.x;
var y = cd.y;
var contours = trace.contours;
var id = 'contour' + trace.uid;
var xa = plotinfo.xaxis;
var ya = plotinfo.yaxis;
var fullLayout = gd._fullLayout;
var pathinfo = emptyPathinfo(contours, plotinfo, cd[0]);
var pathinfo = emptyPathinfo(contours, plotinfo, cd);

// use a heatmap to fill - draw it behind the lines
var heatmapColoringLayer = Lib.ensureSingle(contourLayer, 'g', 'heatmapcoloring');
var heatmapColoringLayer = Lib.ensureSingle(plotGroup, 'g', 'heatmapcoloring');
var cdheatmaps = [];
if(contours.coloring === 'heatmap') {
if(trace.zauto && (trace.autocontour === false)) {
Expand All @@ -62,7 +64,7 @@ function plotOne(gd, plotinfo, cd, contourLayer) {
trace._input.zmax = trace.zmax =
trace.zmin + pathinfo.length * contours.size;
}
cdheatmaps = [cd];
cdheatmaps = [[cd]];
}
heatmapPlot(gd, plotinfo, cdheatmaps, heatmapColoringLayer);

Expand All @@ -87,11 +89,10 @@ function plotOne(gd, plotinfo, cd, contourLayer) {
}

// draw everything
var plotGroup = exports.makeContourGroup(contourLayer, cd, id);
makeBackground(plotGroup, perimeter, contours);
makeFills(plotGroup, fillPathinfo, perimeter, contours);
makeLinesAndLabels(plotGroup, pathinfo, gd, cd[0], contours, perimeter);
clipGaps(plotGroup, plotinfo, fullLayout._clips, cd[0], perimeter);
makeLinesAndLabels(plotGroup, pathinfo, gd, cd, contours, perimeter);
clipGaps(plotGroup, plotinfo, fullLayout._clips, cd, perimeter);
}

exports.makeContourGroup = function(layer, cd, id) {
Expand Down
Binary file modified test/image/baselines/contour_legend.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
18 changes: 18 additions & 0 deletions test/image/mocks/contour_legend.json
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,24 @@
"z": [[3, 2, 1], [6, 5, 4], [9, 8, 7]],
"line": {"color": "#f88"},
"name": "constraint"
}, {
"type": "contour",
"contours": {"coloring": "heatmap"},
"z": [[1, 2], [3, 4]],
"showscale": false,
"showlegend": true,
"y": [2, 3],
"name": "heatmap 1"
}, {
"type": "contour",
"contours": {"coloring": "heatmap"},
"z": [[1, 2], [3, 4]],
"showscale": false,
"showlegend": true,
"colorscale": "Viridis",
"x": [1, 2],
"y": [2, 3],
"name": "heatmap 2"
}],
"layout":{
"autosize":false,
Expand Down
44 changes: 38 additions & 6 deletions test/jasmine/tests/contour_test.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
var d3 = require('d3');

var Plotly = require('@lib/index');
var Plots = require('@src/plots/plots');
var Lib = require('@src/lib');
Expand Down Expand Up @@ -414,12 +416,12 @@ describe('contour plotting and editing', function() {

function _assert(exp) {
var msg = ' index in <g.contourlayer> (call #' + cnt + ')';
var contourLayer = gd.querySelector('.xy > .plot > .contourlayer');
var contourPlot = gd.querySelector('.xy > .plot > .contourlayer > .contour');
var hmIndex = -1;
var contoursIndex = -1;

for(var i in contourLayer.children) {
var child = contourLayer.children[i];
for(var i in contourPlot.children) {
var child = contourPlot.children[i];
if(child.querySelector) {
if(child.querySelector('.hm')) hmIndex = +i;
else if(child.querySelector('.contourlevel')) contoursIndex = +i;
Expand All @@ -439,21 +441,21 @@ describe('contour plotting and editing', function() {
.then(function() {
_assert({
hmIndex: 0,
contoursIndex: 1
contoursIndex: 3
Copy link
Contributor

@etpinard etpinard Aug 10, 2018

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

See what you think e25b9ff

💯

});
return Plotly.restyle(gd, 'contours.coloring', 'lines');
})
.then(function() {
_assert({
hmIndex: -1,
contoursIndex: 1
contoursIndex: 3
});
return Plotly.restyle(gd, 'contours.coloring', 'heatmap');
})
.then(function() {
_assert({
hmIndex: 0,
contoursIndex: 1
contoursIndex: 3
});
})
.catch(fail)
Expand Down Expand Up @@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice test. Thanks!

function getIndices() {
var out = [];
d3.selectAll('.contour').each(function(d) { out.push(d.trace.index); });
return out;
}

Plotly.newPlot(gd, [{
type: 'contour',
z: [[1, 2], [3, 4]]
}, {
type: 'contour',
z: [[2, 1], [4, 3]],
contours: {coloring: 'lines'}
}])
.then(function() {
expect(getIndices()).toEqual([0, 1]);
return Plotly.restyle(gd, 'visible', false, [0]);
})
.then(function() {
expect(getIndices()).toEqual([1]);
return Plotly.restyle(gd, 'visible', true, [0]);
})
.then(function() {
expect(getIndices()).toEqual([0, 1]);
})
.catch(fail)
.then(done);
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

failTest in contour & carpet -> 7843d83

});
});