From d50334e8a16b033f4613cc310a17559e03865afd Mon Sep 17 00:00:00 2001 From: alexcjohnson Date: Fri, 3 Aug 2018 11:24:23 -0400 Subject: [PATCH 1/3] pie color fix & enhancements - fix inheritance of explicit colors by later traces - allow inheritance (of explicit colors) by earlier traces too - add `piecolorway` and `extendpiecolors` for more control over pie colors --- src/plots/plots.js | 12 +++- src/traces/pie/calc.js | 103 ++++++++++++++-------------- src/traces/pie/index.js | 6 +- src/traces/pie/layout_attributes.js | 27 ++++++++ src/traces/pie/layout_defaults.js | 2 + test/jasmine/tests/pie_test.js | 44 ++++++++++++ 6 files changed, 141 insertions(+), 53 deletions(-) diff --git a/src/plots/plots.js b/src/plots/plots.js index 6243ca422fe..04429525565 100644 --- a/src/plots/plots.js +++ b/src/plots/plots.js @@ -2435,8 +2435,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: @@ -2505,6 +2503,8 @@ plots.doCalcdata = function(gd, traces) { // clear stuff that should recomputed in 'regular' loop if(hasCalcTransform) clearAxesCalc(axList); + var calcInteractionsFuncs = []; + function calci(i, isContainer) { trace = fullData[i]; _module = trace._module; @@ -2530,6 +2530,12 @@ plots.doCalcdata = function(gd, traces) { if(_module && _module.calc) { cd = _module.calc(gd, trace); + + // Some modules need to update traces' calcdata after + // *all* traces have been through calc - so later traces can + // impact earlier traces. + var calcInteractions = _module.calcInteractions; + if(calcInteractions) Lib.pushUnique(calcInteractionsFuncs, calcInteractions); } } @@ -2555,6 +2561,8 @@ 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); + for(i = 0; i < calcInteractionsFuncs.length; i++) calcInteractionsFuncs[i](gd, calcdata); + Registry.getComponentMethod('fx', 'calc')(gd); }; diff --git a/src/traces/pie/calc.js b/src/traces/pie/calc.js index 665c6758540..4727df7cfe3 100644 --- a/src/traces/pie/calc.js +++ b/src/traces/pie/calc.js @@ -15,14 +15,13 @@ 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; @@ -30,10 +29,6 @@ module.exports = function calc(gd, trace) { 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++) { @@ -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 @@ -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; @@ -151,34 +123,65 @@ 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.calcInteractions = function(gd, calcdata) { + var fullLayout = gd._fullLayout; + 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; diff --git a/src/traces/pie/index.js b/src/traces/pie/index.js index aa1b9283d73..d790aa81273 100644 --- a/src/traces/pie/index.js +++ b/src/traces/pie/index.js @@ -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.calcInteractions = calcModule.calcInteractions; + Pie.plot = require('./plot'); Pie.style = require('./style'); Pie.styleOne = require('./style_one'); diff --git a/src/traces/pie/layout_attributes.js b/src/traces/pie/layout_attributes.js index b05e5ccbfe4..9ee781624f3 100644 --- a/src/traces/pie/layout_attributes.js +++ b/src/traces/pie/layout_attributes.js @@ -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(' ') } }; diff --git a/src/traces/pie/layout_defaults.js b/src/traces/pie/layout_defaults.js index 40589fcdd08..8da651ffa58 100644 --- a/src/traces/pie/layout_defaults.js +++ b/src/traces/pie/layout_defaults.js @@ -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'); }; diff --git a/test/jasmine/tests/pie_test.js b/test/jasmine/tests/pie_test.js index d56dc6a2ee9..49e451620cb 100644 --- a/test/jasmine/tests/pie_test.js +++ b/test/jasmine/tests/pie_test.js @@ -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) { + 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() { From ded6e945250b910261101ccd92acd20c662abe30 Mon Sep 17 00:00:00 2001 From: alexcjohnson Date: Fri, 3 Aug 2018 20:11:14 -0400 Subject: [PATCH 2/3] update from calcInteractions to crossTraceCalc and simplify by moving more stuff into doCalcdata --- src/plot_api/plot_api.js | 6 ----- src/plots/plots.js | 32 ++++++++++++-------------- src/traces/pie/calc.js | 3 ++- src/traces/pie/index.js | 2 +- test/jasmine/tests/annotations_test.js | 2 +- test/jasmine/tests/bar_test.js | 8 ------- 6 files changed, 19 insertions(+), 34 deletions(-) diff --git a/src/plot_api/plot_api.js b/src/plot_api/plot_api.js index 023edda5c54..d81255aa7f2 100644 --- a/src/plot_api/plot_api.js +++ b/src/plot_api/plot_api.js @@ -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([ @@ -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); diff --git a/src/plots/plots.js b/src/plots/plots.js index 04429525565..f19ecbe9963 100644 --- a/src/plots/plots.js +++ b/src/plots/plots.js @@ -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(); } @@ -2503,8 +2501,6 @@ plots.doCalcdata = function(gd, traces) { // clear stuff that should recomputed in 'regular' loop if(hasCalcTransform) clearAxesCalc(axList); - var calcInteractionsFuncs = []; - function calci(i, isContainer) { trace = fullData[i]; _module = trace._module; @@ -2530,12 +2526,6 @@ plots.doCalcdata = function(gd, traces) { if(_module && _module.calc) { cd = _module.calc(gd, trace); - - // Some modules need to update traces' calcdata after - // *all* traces have been through calc - so later traces can - // impact earlier traces. - var calcInteractions = _module.calcInteractions; - if(calcInteractions) Lib.pushUnique(calcInteractionsFuncs, calcInteractions); } } @@ -2561,9 +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); - for(i = 0; i < calcInteractionsFuncs.length; i++) calcInteractionsFuncs[i](gd, calcdata); + plots.doCrossTraceCalc(gd); Registry.getComponentMethod('fx', 'calc')(gd); + Registry.getComponentMethod('errorbars', 'calc')(gd); }; function clearAxesCalc(axList) { @@ -2599,14 +2590,21 @@ 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); } } } diff --git a/src/traces/pie/calc.js b/src/traces/pie/calc.js index 4727df7cfe3..bf3e892e056 100644 --- a/src/traces/pie/calc.js +++ b/src/traces/pie/calc.js @@ -130,8 +130,9 @@ exports.calc = function calc(gd, trace) { * This is done after sorting, so we pick defaults * in the order slices will be displayed */ -exports.calcInteractions = function(gd, calcdata) { +exports.crossTraceCalc = function(gd) { var fullLayout = gd._fullLayout; + var calcdata = gd.calcdata; var pieColorWay = fullLayout.piecolorway; var colorMap = fullLayout._piecolormap; diff --git a/src/traces/pie/index.js b/src/traces/pie/index.js index d790aa81273..581a1f34d74 100644 --- a/src/traces/pie/index.js +++ b/src/traces/pie/index.js @@ -17,7 +17,7 @@ Pie.layoutAttributes = require('./layout_attributes'); var calcModule = require('./calc'); Pie.calc = calcModule.calc; -Pie.calcInteractions = calcModule.calcInteractions; +Pie.crossTraceCalc = calcModule.crossTraceCalc; Pie.plot = require('./plot'); Pie.style = require('./style'); diff --git a/test/jasmine/tests/annotations_test.js b/test/jasmine/tests/annotations_test.js index a4331307cfe..2b863f862c8 100644 --- a/test/jasmine/tests/annotations_test.js +++ b/test/jasmine/tests/annotations_test.js @@ -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]); return Plotly.relayout(gd, 'annotations[0].text', 'a'); }) .then(function() { diff --git a/test/jasmine/tests/bar_test.js b/test/jasmine/tests/bar_test.js index b6f30ceedd0..e7b3f395a1e 100644 --- a/test/jasmine/tests/bar_test.js +++ b/test/jasmine/tests/bar_test.js @@ -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); - return gd; } From 0bf82c862047304d4c1d1c6f205f773b6ceec044 Mon Sep 17 00:00:00 2001 From: alexcjohnson Date: Mon, 6 Aug 2018 11:13:09 -0400 Subject: [PATCH 3/3] stop exporting doCrossTraceCalc --- src/plots/plots.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/plots/plots.js b/src/plots/plots.js index f19ecbe9963..861493eea07 100644 --- a/src/plots/plots.js +++ b/src/plots/plots.js @@ -2551,7 +2551,7 @@ 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); - plots.doCrossTraceCalc(gd); + doCrossTraceCalc(gd); Registry.getComponentMethod('fx', 'calc')(gd); Registry.getComponentMethod('errorbars', 'calc')(gd); @@ -2563,7 +2563,7 @@ function clearAxesCalc(axList) { } } -plots.doCrossTraceCalc = function(gd) { +function doCrossTraceCalc(gd) { var fullLayout = gd._fullLayout; var modules = fullLayout._visibleModules; var hash = {}; @@ -2608,7 +2608,7 @@ plots.doCrossTraceCalc = function(gd) { } } } -}; +} plots.rehover = function(gd) { if(gd._fullLayout._rehover) {