Skip to content

Commit 1d05081

Browse files
alexcjohnsonalexcjohnson
committed
update showlegend logic for better backward compatibility
1 parent 74e7f84 commit 1d05081

File tree

9 files changed

+144
-57
lines changed

9 files changed

+144
-57
lines changed

‎src/components/legend/defaults.js

Lines changed: 22 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -21,18 +21,33 @@ var helpers = require('./helpers');
2121
module.exports = function legendDefaults(layoutIn, layoutOut, fullData) {
2222
var containerIn = layoutIn.legend || {};
2323

24-
var visibleTraces = 0;
24+
var legendTraceCount = 0;
25+
var legendReallyHasATrace = false;
2526
var defaultOrder = 'normal';
2627

2728
var defaultX, defaultY, defaultXAnchor, defaultYAnchor;
2829

2930
for(var i = 0; i < fullData.length; i++) {
3031
var trace = fullData[i];
3132

32-
if(helpers.legendGetsTrace(trace)) {
33-
visibleTraces++;
34-
// always show the legend by default if there's a pie
35-
if(Registry.traceIs(trace, 'pie')) visibleTraces++;
33+
if(!trace.visible) continue;
34+
35+
// Note that we explicitly count any trace that is either shown or
36+
// *would* be shown by default, toward the two traces you need to
37+
// ensure the legend is shown by default, because this can still help
38+
// disambiguate.
39+
if(trace.showlegend || trace._dfltShowLegend) {
40+
legendTraceCount++;
41+
if(trace.showlegend) {
42+
legendReallyHasATrace = true;
43+
// Always show the legend by default if there's a pie,
44+
// or if there's only one trace but it's explicitly shown
45+
if(Registry.traceIs(trace, 'pie') ||
46+
trace._input.showlegend === true
47+
) {
48+
legendTraceCount++;
49+
}
50+
}
3651
}
3752

3853
if((Registry.traceIs(trace, 'bar') && layoutOut.barmode === 'stack') ||
@@ -48,7 +63,8 @@ module.exports = function legendDefaults(layoutIn, layoutOut, fullData) {
4863
}
4964

5065
var showLegend = Lib.coerce(layoutIn, layoutOut,
51-
basePlotLayoutAttributes, 'showlegend', visibleTraces > 1);
66+
basePlotLayoutAttributes, 'showlegend',
67+
legendReallyHasATrace && legendTraceCount > 1);
5268

5369
if(showLegend === false) return;
5470

‎src/components/legend/get_legend_data.js

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -41,12 +41,12 @@ module.exports = function getLegendData(calcdata, opts) {
4141

4242
// build an { legendgroup: [cd0, cd0], ... } object
4343
for(i = 0; i < calcdata.length; i++) {
44-
var cd = calcdata[i],
45-
cd0 = cd[0],
46-
trace = cd0.trace,
47-
lgroup = trace.legendgroup;
44+
var cd = calcdata[i];
45+
var cd0 = cd[0];
46+
var trace = cd0.trace;
47+
var lgroup = trace.legendgroup;
4848

49-
if(!helpers.legendGetsTrace(trace) || !trace.showlegend) continue;
49+
if(!trace.visible || !trace.showlegend) continue;
5050

5151
if(Registry.traceIs(trace, 'pie')) {
5252
if(!slicesShown[lgroup]) slicesShown[lgroup] = {};

‎src/components/legend/helpers.js

Lines changed: 0 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -9,16 +9,6 @@
99

1010
'use strict';
1111

12-
exports.legendGetsTrace = function legendGetsTrace(trace) {
13-
// traceIs(trace, 'showLegend') is not sufficient anymore, due to contour(carpet)?
14-
// which are legend-eligible only if type: constraint. Otherwise, showlegend gets deleted.
15-
16-
// Note that we explicitly include showlegend: false, so a trace that *could* be
17-
// in the legend but is not shown still counts toward the two traces you need to
18-
// ensure the legend is shown by default, because this can still help disambiguate.
19-
return trace.visible && (trace.showlegend !== undefined);
20-
};
21-
2212
exports.isGrouped = function isGrouped(legendLayout) {
2313
return (legendLayout.traceorder || '').indexOf('grouped') !== -1;
2414
};

‎src/plots/layout_attributes.js

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -173,7 +173,13 @@ module.exports = {
173173
valType: 'boolean',
174174
role: 'info',
175175
editType: 'legend',
176-
description: 'Determines whether or not a legend is drawn.'
176+
description: [
177+
'Determines whether or not a legend is drawn.',
178+
'Default is `true` if there is a trace to show and any of these:',
179+
'a) Two or more traces would by default be shown in the legend.',
180+
'b) One pie trace is shown in the legend.',
181+
'c) One trace is explicitly given with `showlegend: true`.'
182+
].join(' ')
177183
},
178184
colorway: {
179185
valType: 'colorlist',

‎src/plots/plots.js

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1144,9 +1144,13 @@ plots.supplyTraceDefaults = function(traceIn, traceOut, colorIndex, layout, trac
11441144
coerce('ids');
11451145

11461146
if(Registry.traceIs(traceOut, 'showLegend')) {
1147+
traceOut._dfltShowLegend = true;
11471148
coerce('showlegend');
11481149
coerce('legendgroup');
11491150
}
1151+
else {
1152+
traceOut._dfltShowLegend = false;
1153+
}
11501154

11511155
Registry.getComponentMethod(
11521156
'fx',

‎src/traces/contour/style_defaults.js

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ module.exports = function handleStyleDefaults(traceIn, traceOut, coerce, layout,
3030
// plots/plots always coerces showlegend to true, but in this case
3131
// we default to false and (by default) show a colorbar instead
3232
if(traceIn.showlegend !== true) traceOut.showlegend = false;
33+
traceOut._dfltShowLegend = false;
3334

3435
colorscaleDefaults(
3536
traceIn, traceOut, layout, coerce, {prefix: '', cLetter: 'z'}

‎test/image/mocks/colorscale_opacity.json

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -567,7 +567,6 @@
567567
},
568568
"height": 450,
569569
"width": 1100,
570-
"autosize": true,
571-
"showlegend": false
570+
"autosize": true
572571
}
573572
}

‎test/image/mocks/contour_scatter.json

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -341,8 +341,7 @@
341341
"line": {
342342
"color": "black"
343343
},
344-
"type": "scatter",
345-
"showlegend": false
344+
"type": "scatter"
346345
}
347346
]
348347
}

‎test/jasmine/tests/legend_test.js

Lines changed: 103 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -33,12 +33,81 @@ describe('legend defaults', function() {
3333
};
3434
});
3535

36+
function allShown(fullData) {
37+
return fullData.map(function(trace) {
38+
return Lib.extendDeep({
39+
visible: true,
40+
showlegend: true,
41+
_dfltShowLegend: true,
42+
_input: {}
43+
}, trace);
44+
});
45+
}
46+
47+
it('hides by default if there is only one legend item by default', function() {
48+
fullData = allShown([
49+
{type: 'scatter'},
50+
{type: 'scatter', visible: false}, // ignored
51+
{type: 'contour', _dfltShowLegend: false, showlegend: false} // hidden by default
52+
]);
53+
54+
supplyLayoutDefaults({}, layoutOut, fullData);
55+
expect(layoutOut.showlegend).toBe(false);
56+
});
57+
58+
it('shows if there are two legend items by default but only one is shown', function() {
59+
fullData = allShown([
60+
{type: 'scatter'},
61+
{type: 'scatter', showlegend: false} // not shown but still triggers legend
62+
]);
63+
64+
supplyLayoutDefaults({}, layoutOut, fullData);
65+
expect(layoutOut.showlegend).toBe(true);
66+
});
67+
68+
it('hides if no items are actually shown', function() {
69+
fullData = allShown([
70+
{type: 'scatter', showlegend: false},
71+
{type: 'scatter', showlegend: false}
72+
]);
73+
74+
supplyLayoutDefaults({}, layoutOut, fullData);
75+
expect(layoutOut.showlegend).toBe(false);
76+
});
77+
78+
it('shows with one visible pie', function() {
79+
fullData = allShown([
80+
{type: 'pie'}
81+
]);
82+
83+
supplyLayoutDefaults({}, layoutOut, fullData);
84+
expect(layoutOut.showlegend).toBe(true);
85+
});
86+
87+
it('does not show with a hidden pie', function() {
88+
fullData = allShown([
89+
{type: 'pie', showlegend: false}
90+
]);
91+
92+
supplyLayoutDefaults({}, layoutOut, fullData);
93+
expect(layoutOut.showlegend).toBe(false);
94+
});
95+
96+
it('shows if even a default hidden single item is explicitly shown', function() {
97+
fullData = allShown([
98+
{type: 'contour', _dfltShowLegend: false, _input: {showlegend: true}}
99+
]);
100+
101+
supplyLayoutDefaults({}, layoutOut, fullData);
102+
expect(layoutOut.showlegend).toBe(true);
103+
});
104+
36105
it('should default traceorder to reversed for stack bar charts', function() {
37-
fullData = [
38-
{ type: 'bar' },
39-
{ type: 'bar' },
40-
{ type: 'scatter' }
41-
];
106+
fullData = allShown([
107+
{type: 'bar', visible: 'legendonly'},
108+
{type: 'bar', visible: 'legendonly'},
109+
{type: 'scatter'}
110+
]);
42111

43112
supplyLayoutDefaults(layoutIn, layoutOut, fullData);
44113
expect(layoutOut.legend.traceorder).toEqual('normal');
@@ -50,20 +119,20 @@ describe('legend defaults', function() {
50119
});
51120

52121
it('should default traceorder to reversed for filled tonext scatter charts', function() {
53-
fullData = [
54-
{ type: 'scatter' },
55-
{ type: 'scatter', fill: 'tonexty' }
56-
];
122+
fullData = allShown([
123+
{type: 'scatter'},
124+
{type: 'scatter', fill: 'tonexty'}
125+
]);
57126

58127
supplyLayoutDefaults(layoutIn, layoutOut, fullData);
59128
expect(layoutOut.legend.traceorder).toEqual('reversed');
60129
});
61130

62131
it('should default traceorder to grouped when a group is present', function() {
63-
fullData = [
64-
{ type: 'scatter', legendgroup: 'group' },
65-
{ type: 'scatter'}
66-
];
132+
fullData = allShown([
133+
{type: 'scatter', legendgroup: 'group'},
134+
{type: 'scatter'}
135+
]);
67136

68137
supplyLayoutDefaults(layoutIn, layoutOut, fullData);
69138
expect(layoutOut.legend.traceorder).toEqual('grouped');
@@ -74,6 +143,27 @@ describe('legend defaults', function() {
74143
expect(layoutOut.legend.traceorder).toEqual('grouped+reversed');
75144
});
76145

146+
it('does not consider invisible traces for traceorder default', function() {
147+
fullData = allShown([
148+
{type: 'bar', visible: false},
149+
{type: 'bar', visible: false},
150+
{type: 'scatter'}
151+
]);
152+
153+
layoutOut.barmode = 'stack';
154+
155+
supplyLayoutDefaults(layoutIn, layoutOut, fullData);
156+
expect(layoutOut.legend.traceorder).toEqual('normal');
157+
158+
fullData = allShown([
159+
{type: 'scatter', legendgroup: 'group', visible: false},
160+
{type: 'scatter'}
161+
]);
162+
163+
supplyLayoutDefaults(layoutIn, layoutOut, fullData);
164+
expect(layoutOut.legend.traceorder).toEqual('normal');
165+
});
166+
77167
it('should default orientation to vertical', function() {
78168
supplyLayoutDefaults(layoutIn, layoutOut, []);
79169
expect(layoutOut.legend.orientation).toEqual('v');
@@ -382,24 +472,6 @@ describe('legend getLegendData', function() {
382472
describe('legend helpers:', function() {
383473
'use strict';
384474

385-
describe('legendGetsTraces', function() {
386-
var legendGetsTrace = helpers.legendGetsTrace;
387-
388-
it('should return true when trace is visible and supports legend', function() {
389-
expect(legendGetsTrace({ visible: true, showlegend: true })).toBe(true);
390-
expect(legendGetsTrace({ visible: false, showlegend: true })).toBe(false);
391-
expect(legendGetsTrace({ visible: 'legendonly', showlegend: true })).toBe(true);
392-
393-
expect(legendGetsTrace({ visible: true, showlegend: false })).toBe(true);
394-
expect(legendGetsTrace({ visible: false, showlegend: false })).toBe(false);
395-
expect(legendGetsTrace({ visible: 'legendonly', showlegend: false })).toBe(true);
396-
397-
expect(legendGetsTrace({ visible: true })).toBe(false);
398-
expect(legendGetsTrace({ visible: false })).toBe(false);
399-
expect(legendGetsTrace({ visible: 'legendonly' })).toBe(false);
400-
});
401-
});
402-
403475
describe('isGrouped', function() {
404476
var isGrouped = helpers.isGrouped;
405477

0 commit comments

Comments
 (0)