Skip to content

Pie colors fix & enhancements #2870

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

Merged
merged 3 commits into from
Aug 6, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 0 additions & 6 deletions src/plot_api/plot_api.js
Original file line number Diff line number Diff line change
Expand Up @@ -292,11 +292,6 @@ exports.plot = function(gd, data, layout, config) {
return;
}

Plots.doCrossTraceCalc(gd);

// calc and autorange for errorbars
Registry.getComponentMethod('errorbars', 'calc')(gd);

// TODO: autosize extra for text markers and images
// see https://github.com/plotly/plotly.js/issues/1111
return Lib.syncOrAsync([
Expand Down Expand Up @@ -331,7 +326,6 @@ exports.plot = function(gd, data, layout, config) {
];

if(hasCartesian) seq.push(positionAndAutorange);
else seq.push(Plots.doCrossTraceCalc);

seq.push(subroutines.layoutStyles);
if(hasCartesian) seq.push(drawAxes);
Expand Down
30 changes: 18 additions & 12 deletions src/plots/plots.js
Original file line number Diff line number Diff line change
Expand Up @@ -2236,8 +2236,6 @@ plots.transition = function(gd, data, layout, traces, frameOpts, transitionOpts)

plots.supplyDefaults(gd);
plots.doCalcdata(gd);
plots.doCrossTraceCalc(gd);
Registry.getComponentMethod('errorbars', 'calc')(gd);

return Promise.resolve();
}
Expand Down Expand Up @@ -2435,8 +2433,6 @@ plots.doCalcdata = function(gd, traces) {

// for sharing colors across pies (and for legend)
fullLayout._piecolormap = {};
fullLayout._piecolorway = null;
fullLayout._piedefaultcolorcount = 0;

// If traces were specified and this trace was not included,
// then transfer it over from the old calcdata:
Expand Down Expand Up @@ -2555,7 +2551,10 @@ plots.doCalcdata = function(gd, traces) {
for(i = 0; i < fullData.length; i++) calci(i, true);
for(i = 0; i < fullData.length; i++) calci(i, false);

doCrossTraceCalc(gd);

Registry.getComponentMethod('fx', 'calc')(gd);
Registry.getComponentMethod('errorbars', 'calc')(gd);
};

function clearAxesCalc(axList) {
Expand All @@ -2564,7 +2563,7 @@ function clearAxesCalc(axList) {
}
}

plots.doCrossTraceCalc = function(gd) {
function doCrossTraceCalc(gd) {
var fullLayout = gd._fullLayout;
var modules = fullLayout._visibleModules;
var hash = {};
Expand All @@ -2591,18 +2590,25 @@ plots.doCrossTraceCalc = function(gd) {
var methods = hash[k];
var subplots = fullLayout._subplots[k];

for(i = 0; i < subplots.length; i++) {
var sp = subplots[i];
var spInfo = k === 'cartesian' ?
fullLayout._plots[sp] :
fullLayout[sp];
if(Array.isArray(subplots)) {
for(i = 0; i < subplots.length; i++) {
var sp = subplots[i];
var spInfo = k === 'cartesian' ?
fullLayout._plots[sp] :
fullLayout[sp];

for(j = 0; j < methods.length; j++) {
methods[j](gd, spInfo);
}
}
}
else {
for(j = 0; j < methods.length; j++) {
methods[j](gd, spInfo);
methods[j](gd);
}
}
}
};
}

plots.rehover = function(gd) {
if(gd._fullLayout._rehover) {
Expand Down
104 changes: 54 additions & 50 deletions src/traces/pie/calc.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,25 +15,20 @@ var tinycolor = require('tinycolor2');
var Color = require('../../components/color');
var helpers = require('./helpers');

module.exports = function calc(gd, trace) {
exports.calc = function calc(gd, trace) {
var vals = trace.values;
var hasVals = isArrayOrTypedArray(vals) && vals.length;
var labels = trace.labels;
var colors = trace.marker.colors || [];
var cd = [];
var fullLayout = gd._fullLayout;
var colorWay = fullLayout.colorway;
var colorMap = fullLayout._piecolormap;
var allThisTraceLabels = {};
var vTotal = 0;
var hiddenLabels = fullLayout.hiddenlabels || [];

var i, v, label, hidden, pt;

if(!fullLayout._piecolorway && colorWay !== Color.defaults) {
fullLayout._piecolorway = generateDefaultColors(colorWay);
}

if(trace.dlabel) {
labels = new Array(vals.length);
for(i = 0; i < vals.length; i++) {
Expand Down Expand Up @@ -79,7 +74,7 @@ module.exports = function calc(gd, trace) {
cd.push({
v: v,
label: label,
color: pullColor(colors[i]),
color: pullColor(colors[i], label),
i: i,
pts: [i],
hidden: hidden
Expand All @@ -99,29 +94,6 @@ module.exports = function calc(gd, trace) {

if(trace.sort) cd.sort(function(a, b) { return b.v - a.v; });

/**
* now go back and fill in colors we're still missing
* this is done after sorting, so we pick defaults
* in the order slices will be displayed
*/

for(i = 0; i < cd.length; i++) {
pt = cd[i];
if(pt.color === false) {
// have we seen this label and assigned a color to it in a previous trace?
if(colorMap[pt.label]) {
pt.color = colorMap[pt.label];
}
else {
colorMap[pt.label] = pt.color = nextDefaultColor(
fullLayout._piedefaultcolorcount,
fullLayout._piecolorway
);
fullLayout._piedefaultcolorcount++;
}
}
}

// include the sum of all values in the first point
if(cd[0]) cd[0].vTotal = vTotal;

Expand Down Expand Up @@ -151,34 +123,66 @@ module.exports = function calc(gd, trace) {
return cd;
};

/**
* pick a default color from the main default set, augmented by
* itself lighter then darker before repeating
/*
* `calc` filled in (and collated) explicit colors.
* Now we need to propagate these explicit colors to other traces,
* and fill in default colors.
* This is done after sorting, so we pick defaults
* in the order slices will be displayed
*/
var pieDefaultColors;
exports.crossTraceCalc = function(gd) {
var fullLayout = gd._fullLayout;
var calcdata = gd.calcdata;
var pieColorWay = fullLayout.piecolorway;
var colorMap = fullLayout._piecolormap;

function nextDefaultColor(index, pieColorWay) {
if(!pieDefaultColors) {
// generate this default set on demand (but then it gets saved in the module)
var mainDefaults = Color.defaults;
pieDefaultColors = generateDefaultColors(mainDefaults);
if(fullLayout.extendpiecolors) {
pieColorWay = generateExtendedColors(pieColorWay);
}
var dfltColorCount = 0;

var i, j, cd, pt;
for(i = 0; i < calcdata.length; i++) {
cd = calcdata[i];
if(cd[0].trace.type !== 'pie') continue;

for(j = 0; j < cd.length; j++) {
pt = cd[j];
if(pt.color === false) {
// have we seen this label and assigned a color to it in a previous trace?
if(colorMap[pt.label]) {
pt.color = colorMap[pt.label];
}
else {
colorMap[pt.label] = pt.color = pieColorWay[dfltColorCount % pieColorWay.length];
dfltColorCount++;
}
}
}
}
};

var pieColors = pieColorWay || pieDefaultColors;
return pieColors[index % pieColors.length];
}
/**
* pick a default color from the main default set, augmented by
* itself lighter then darker before repeating
*/
var extendedColorWays = {};

function generateDefaultColors(colorList) {
function generateExtendedColors(colorList) {
var i;
var colorString = JSON.stringify(colorList);
var pieColors = extendedColorWays[colorString];
if(!pieColors) {
pieColors = colorList.slice();

var pieColors = colorList.slice();

for(i = 0; i < colorList.length; i++) {
pieColors.push(tinycolor(colorList[i]).lighten(20).toHexString());
}
for(i = 0; i < colorList.length; i++) {
pieColors.push(tinycolor(colorList[i]).lighten(20).toHexString());
}

for(i = 0; i < colorList.length; i++) {
pieColors.push(tinycolor(colorList[i]).darken(20).toHexString());
for(i = 0; i < colorList.length; i++) {
pieColors.push(tinycolor(colorList[i]).darken(20).toHexString());
}
extendedColorWays[colorString] = pieColors;
}

return pieColors;
Expand Down
6 changes: 5 additions & 1 deletion src/traces/pie/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,11 @@ Pie.attributes = require('./attributes');
Pie.supplyDefaults = require('./defaults');
Pie.supplyLayoutDefaults = require('./layout_defaults');
Pie.layoutAttributes = require('./layout_attributes');
Pie.calc = require('./calc');

var calcModule = require('./calc');
Pie.calc = calcModule.calc;
Pie.crossTraceCalc = calcModule.crossTraceCalc;

Pie.plot = require('./plot');
Pie.style = require('./style');
Pie.styleOne = require('./style_one');
Expand Down
27 changes: 27 additions & 0 deletions src/traces/pie/layout_attributes.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,5 +17,32 @@ module.exports = {
hiddenlabels: {
valType: 'data_array',
editType: 'calc'
},
piecolorway: {
valType: 'colorlist',
role: 'style',
editType: 'calc',
description: [
'Sets the default pie slice colors. Defaults to the main',
'`colorway` used for trace colors. If you specify a new',
'list here it can still be extended with lighter and darker',
'colors, see `extendpiecolors`.'
].join(' ')
},
extendpiecolors: {
valType: 'boolean',
dflt: true,
role: 'style',
editType: 'calc',
description: [
'If `true`, the pie slice colors (whether given by `piecolorway` or',
'inherited from `colorway`) will be extended to three times its',
'original length by first repeating every color 20% lighter then',
'each color 20% darker. This is intended to reduce the likelihood',
'of reusing the same color when you have many slices, but you can',
'set `false` to disable.',
'Colors provided in the trace, using `marker.colors`, are never',
'extended.'
].join(' ')
}
};
2 changes: 2 additions & 0 deletions src/traces/pie/layout_defaults.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,4 +17,6 @@ module.exports = function supplyLayoutDefaults(layoutIn, layoutOut) {
return Lib.coerce(layoutIn, layoutOut, layoutAttributes, attr, dflt);
}
coerce('hiddenlabels');
coerce('piecolorway', layoutOut.colorway);
coerce('extendpiecolors');
};
2 changes: 1 addition & 1 deletion test/jasmine/tests/annotations_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -775,7 +775,7 @@ describe('annotations autorange', function() {
});
})
.then(function() {
_assert('auto rng / big tx', [-0.22, 3.57], [0.84, 3.365]);
_assert('auto rng / big tx', [-0.22, 3.59], [0.84, 3.365]);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Doesn't matter for CI, and has nothing to do with this PR, but this change makes this test pass on my Mac.

return Plotly.relayout(gd, 'annotations[0].text', 'a');
})
.then(function() {
Expand Down
8 changes: 0 additions & 8 deletions test/jasmine/tests/bar_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1801,14 +1801,6 @@ function mockBarPlot(dataWithoutTraceType, layout) {
supplyAllDefaults(gd);
Plots.doCalcdata(gd);

var plotinfo = {
xaxis: gd._fullLayout.xaxis,
yaxis: gd._fullLayout.yaxis
};

// call Bar.crossTraceCalc
Bar.crossTraceCalc(gd, plotinfo);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

⚠️ ⚠️ ⚠️ Removed because this is included in doCalcdata now, but in fact the test FAILS if we leave this in. That means Bar.crossTraceCalc is not idempotent, though I didn't really look into what it's doing. I don't think this is particularly a problem, but it might indicate it's more fragile than it should be.

Copy link
Contributor

Choose a reason for hiding this comment

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

That means Bar.crossTraceCalc is not idempotent, though I didn't really look into what it's doing.

Yep, I noticed that last week. This commit dfada6a helps, but there are more cases where Bar.setPositions oops I mean Bar.crossTraceCalc mutates gd.calcdata.


return gd;
}

Expand Down
44 changes: 44 additions & 0 deletions test/jasmine/tests/pie_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,50 @@ describe('Pie traces:', function() {
.catch(failTest)
.then(done);
});

function _checkSliceColors(colors) {
return function() {
d3.select(gd).selectAll('.slice path').each(function(d, i) {
expect(this.style.fill.replace(/(\s|rgb\(|\))/g, '')).toBe(colors[i], i);
});
};
}

it('propagates explicit colors to the same labels in earlier OR later traces', function(done) {
var data1 = [
{type: 'pie', values: [3, 2], marker: {colors: ['red', 'black']}, domain: {x: [0.5, 1]}},
{type: 'pie', values: [2, 5], domain: {x: [0, 0.5]}}
];
var data2 = Lib.extendDeep([], [data1[1], data1[0]]);

Plotly.newPlot(gd, data1)
.then(_checkSliceColors(['255,0,0', '0,0,0', '0,0,0', '255,0,0']))
.then(function() {
return Plotly.newPlot(gd, data2);
})
.then(_checkSliceColors(['0,0,0', '255,0,0', '255,0,0', '0,0,0']))
.catch(failTest)
.then(done);
});

it('can use a separate pie colorway and disable extended colors', 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.

Great tests!

Plotly.newPlot(gd, [{type: 'pie', values: [7, 6, 5, 4, 3, 2, 1]}], {colorway: ['#777', '#F00']})
.then(_checkSliceColors(['119,119,119', '255,0,0', '170,170,170', '255,102,102', '68,68,68', '153,0,0', '119,119,119']))
.then(function() {
return Plotly.relayout(gd, {extendpiecolors: false});
})
.then(_checkSliceColors(['119,119,119', '255,0,0', '119,119,119', '255,0,0', '119,119,119', '255,0,0', '119,119,119']))
.then(function() {
return Plotly.relayout(gd, {piecolorway: ['#FF0', '#0F0', '#00F']});
})
.then(_checkSliceColors(['255,255,0', '0,255,0', '0,0,255', '255,255,0', '0,255,0', '0,0,255', '255,255,0']))
.then(function() {
return Plotly.relayout(gd, {extendpiecolors: null});
})
.then(_checkSliceColors(['255,255,0', '0,255,0', '0,0,255', '255,255,102', '102,255,102', '102,102,255', '153,153,0']))
.catch(failTest)
.then(done);
});
});

describe('pie hovering', function() {
Expand Down