-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Unique colors for expanded traces #1830
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
Changes from 4 commits
476c8b7
4398bd7
808e35b
0877f48
5f346e4
c8a2607
2ceac06
25c2951
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -659,6 +659,41 @@ plots.linkSubplots = function(newFullData, newFullLayout, oldFullData, oldFullLa | |
} | ||
}; | ||
|
||
// This function clears defaults between the first and second pass of | ||
// supplyDefaults. It exists because otherwise null attributes are | ||
// supplyDefault'd and inherited as *colors* instead of an actual null | ||
// attribute which needs to be supplydefaulted by the individual | ||
// expanded traces. | ||
plots.clearExpandedTraceDefaultColors = function(expandedTraces) { | ||
var colorAttrs, path, trace, i, j; | ||
|
||
// A better check *might* be to explicitly check for a groupby transform | ||
if(expandedTraces.length <= 1) return; | ||
|
||
function locateExpandedTraceAttrs(attr, attrName, attrs, level) { | ||
path[level] = attrName; | ||
path.length = level + 1; | ||
if(attr.useExpandedTraceDefaultColor) { | ||
colorAttrs.push(path.join('.')); | ||
} | ||
} | ||
|
||
for(i = 0; i < expandedTraces.length; i++) { | ||
trace = expandedTraces[i]; | ||
colorAttrs = []; | ||
path = []; | ||
|
||
if(!trace || !trace._module) continue; | ||
|
||
PlotSchema.crawl(trace._module.attributes, locateExpandedTraceAttrs); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. would it be worthwhile to cache There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good idea. Will do. |
||
|
||
for(j = 0; j < colorAttrs.length; j++) { | ||
Lib.nestedProperty(trace, colorAttrs[j]).set(null); | ||
} | ||
} | ||
}; | ||
|
||
|
||
plots.supplyDataDefaults = function(dataIn, dataOut, layout, fullLayout) { | ||
var i, fullTrace, trace; | ||
var modules = fullLayout._modules = [], | ||
|
@@ -693,6 +728,8 @@ plots.supplyDataDefaults = function(dataIn, dataOut, layout, fullLayout) { | |
if(fullTrace.transforms && fullTrace.transforms.length) { | ||
var expandedTraces = applyTransforms(fullTrace, dataOut, layout, fullLayout); | ||
|
||
plots.clearExpandedTraceDefaultColors(expandedTraces); | ||
|
||
for(var j = 0; j < expandedTraces.length; j++) { | ||
var expandedTrace = expandedTraces[j], | ||
fullExpandedTrace = plots.supplyTraceDefaults(expandedTrace, cnt, fullLayout, i); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
{ | ||
"data": [{ | ||
"x": [2, 3, 4, 5, 6, 7], | ||
"y": [3, 2, 4, 3, 5, 4], | ||
"transforms": [{ | ||
"type": "groupby", | ||
"groups": ["c", "b", "d", "c", "e", "d"] | ||
}] | ||
}, { | ||
"x": [2, 3, 4, 5, 6, 7], | ||
"y": [9, 8, 10, 9, 11, 10], | ||
"marker": { | ||
"symbol": "square" | ||
}, | ||
"transforms": [{ | ||
"type": "groupby", | ||
"groups": ["c", "b", "d", "c", "e", "d"] | ||
}] | ||
}], | ||
"layout": { | ||
"title": "Groupby expanded trace coloring" | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,30 @@ | ||
var Plots = require('@src/plots/plots'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No need for a new file here. Can you move this to either |
||
|
||
describe('groupby', function() { | ||
it('varies the color for each expanded trace', function() { | ||
var uniqueColors = {}; | ||
var dataOut = []; | ||
var dataIn = [{ | ||
y: [1, 2, 3], | ||
transforms: [ | ||
{type: 'filter', operation: '<', value: 4}, | ||
{type: 'groupby', groups: ['a', 'b', 'c']} | ||
] | ||
}, { | ||
y: [4, 5, 6], | ||
transforms: [ | ||
{type: 'filter', operation: '<', value: 4}, | ||
{type: 'groupby', groups: ['a', 'b', 'b']} | ||
] | ||
}]; | ||
|
||
Plots.supplyDataDefaults(dataIn, dataOut, {}, {}); | ||
|
||
for(var i = 0; i < dataOut.length; i++) { | ||
uniqueColors[dataOut[i].marker.color] = true; | ||
} | ||
|
||
// Confirm that five total colors exist: | ||
expect(Object.keys(uniqueColors).length).toEqual(5); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would be better for sure, but I'm ok with this for now, as this will never get called for transform-less traces.
Maybe we start thinking about adding transform module categories (similar to our trace module categories) to help us out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a hypothetical for now, since
groupby
is currently the only one that can yield more than one trace... but my rationale for suggesting this was that it's precisely this condition that breaks default colors. We'll see what happens if/when we add more such transforms, if this is insufficient then a category system is a great idea!