No unwarranted colorscale or related data on parcoords trace/fullTrace #1508
Conversation
… specified one (as interpreted by has_colorscale.js)
src/traces/parcoords/calc.js
Outdated
| var cs = !!trace.line.colorscale && Lib.isArray(trace.line.color); | ||
| var color = cs ? trace.line.color : Array.apply(0, Array(trace.dimensions.reduce(function(p, n) {return Math.max(p, n.values.length);}, 0))).map(function() {return 0.5;}); | ||
| var cscale = cs ? trace.line.colorscale : [[0, trace.line.color], [1, trace.line.color]]; | ||
|
|
There was a problem hiding this comment.
I just moved this block unchanged from here, where it'd be persisted, to parcoords.js below, as this calculation is now considered implementation detail, unexposed on data/fullData
src/traces/parcoords/calc.js
Outdated
| var cscale = cs ? trace.line.colorscale : [[0, trace.line.color], [1, trace.line.color]]; | ||
|
|
||
| trace.line.color = color; | ||
| trace.line.colorscale = cscale; | ||
|
|
There was a problem hiding this comment.
... so, no more persisting
src/traces/parcoords/parcoords.js
Outdated
| trace.line.color : | ||
| Array.apply(0, Array(trace.dimensions.reduce(function(p, n) {return Math.max(p, n.values.length);}, 0))).map(function() {return 0.5;}); | ||
| var lineColorScale = cs ? trace.line.colorscale : [[0, trace.line.color], [1, trace.line.color]]; | ||
|
|
There was a problem hiding this comment.
okay this is the moved logic block
There was a problem hiding this comment.
Hmm. I would prefer not. Per-datum non-pixel related computations should be in the calc step.
There was a problem hiding this comment.
In the original code, calc.js actually persisted the result of these calculations, see above, these lines:
- trace.line.color = color;
- trace.line.colorscale = cscale;
... so instead of this, I'd just return these two derived things (whereas now, calc.js returns {})
Is it how you'd prefer it? I'm swamped with preparation so I'd avoid dead-ends and you may already have a preferred way in mind (or see issues with it).
|
|
||
| if(hasColorscale(traceIn, 'line')) { | ||
| if(hasColorscale(traceIn, 'line') && Lib.isArray(traceIn.line.color)) { | ||
| coerce('line.colorscale'); | ||
| colorscaleDefaults(traceIn, traceOut, layout, coerce, {prefix: 'line.', cLetter: 'c'}); |
There was a problem hiding this comment.
There's no point in attempting to use a color scale if line.color is not an array but e.g. a plain color, because the line.color numerical array values are mapped to the colors on the color scale.
Also, now line.colorscale is only performed if it's warranted.
There was a problem hiding this comment.
I believe patching defaults.js is all you need to fix this bug.
- but store results in calcdata items, instead of mutating the corresponding fullData items.
Edits for no unwarranted colorscale or related data on parcoords trace/fullTrace
Avoid color scale persistence on trace / fullTrace if the user hasn't specified one (as interpreted by has_colorscale.js)
Before this PR,
colorscalerelated details populateddataandfullData, as seen by these test case changes.