Skip to content

initial implementation of layout.colorscale #3274

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.

Sign up for GitHub

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 15 commits into from
Nov 23, 2018
Merged

initial implementation of layout.colorscale #3274

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
6b1b940
initial implementation of `layout.colorscale`
antoinerg Nov 20, 2018
0e98c5b
fix editType
antoinerg Nov 20, 2018
6e61f3b
fix parcoords, fix default colorscale
antoinerg Nov 20, 2018
f5bcb71
do not coerce layout.colorscale if not specified
antoinerg Nov 20, 2018
a299ce0
move layout colorscale attributes into its own file
antoinerg Nov 21, 2018
2a20414
update description for layout.colorscale attributes
antoinerg Nov 21, 2018
2460fc3
fix lint
antoinerg Nov 21, 2018
0c13592
change colorscaleCalc's signature, cleanup code
antoinerg Nov 21, 2018
93ce241
update colorscale tests to provide a layout object to calcColorscale
antoinerg Nov 21, 2018
d263a7f
fix lint
antoinerg Nov 21, 2018
84f0ea8
revert parcoords_tests.js to master
antoinerg Nov 21, 2018
ee222d5
test layout.colorscale inheritance
antoinerg Nov 22, 2018
75e8910
improve code readability
antoinerg Nov 22, 2018
e50a6aa
add a mock using template and layout.colorscale
antoinerg Nov 22, 2018
dca4350
set autocolorscale: true in template to properly test mock
antoinerg Nov 23, 2018
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: 3 additions & 3 deletions src/components/colorscale/calc.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,9 +84,9 @@ module.exports = function calc(trace, vals, containerStr, cLetter) {
doUpdate(autoAttr, (auto !== false || (min === undefined && max === undefined)));

if(container.autocolorscale) {
if(min * max < 0) scl = scales.RdBu;
else if(min >= 0) scl = scales.Reds;
else scl = scales.Blues;
if(min * max < 0) scl = container.diverging || scales.RdBu;
else if(min >= 0) scl = container.sequential || scales.Reds;
else scl = container.sequentialminus || scales.Blues;

// reversescale is handled at the containerOut level
doUpdate('colorscale', scl, container.reversescale ? flipScale(scl) : scl);
Expand Down
11 changes: 9 additions & 2 deletions src/components/colorscale/defaults.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ var colorbarDefaults = require('../colorbar/defaults');
var isValidScale = require('./is_valid_scale');
var flipScale = require('./flip_scale');


module.exports = function colorScaleDefaults(traceIn, traceOut, layout, coerce, opts) {
var prefix = opts.prefix,
cLetter = opts.cLetter,
Expand All @@ -43,7 +42,15 @@ module.exports = function colorScaleDefaults(traceIn, traceOut, layout, coerce,
var autoColorscaleDflt;
if(sclIn !== undefined) autoColorscaleDflt = !isValidScale(sclIn);
coerce(prefix + 'autocolorscale', autoColorscaleDflt);
var sclOut = coerce(prefix + 'colorscale');

var layoutColorscale = layout.colorscale || {};
containerOut.diverging = layoutColorscale.diverging;
containerOut.sequential = layoutColorscale.sequential;
containerOut.sequentialminus = layoutColorscale.sequentialminus;
var dfltScl = containerOut.diverging;
var sclOut;
if(dfltScl) sclOut = coerce(prefix + 'colorscale', dfltScl);
Copy link
Contributor

Choose a reason for hiding this comment

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

Ha I see. We wouldn't need this extra if-else block if you made the layout.colorscale.*.dflt undefined (or removed them entirely) instead of false. The relevant line in coerce.js is:

plotly.js/src/lib/coerce.js

Line 384 in 695f311

if(dflt === undefined) dflt = opts.dflt;

Copy link
Contributor

Choose a reason for hiding this comment

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

... But looking at this again, do we really need logic in colorscale/defaults.js and in colorscale/calc.js?

Ideally,

  • I would leave colorscale/defaults.js untouched
  • make the layout.colorscale.*.dflt match scales.RdBu, scales.Reds and scales.Blues, making those container.diverging || scales.RdBu; in colorscale/calc.js unnecessary.
  • handle the autocolorscale logic in colorscale/calc.js

Now, I'm just thinking out-loud. @antoinerg let me know if my proposal has any flaws.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this makes a lot of sense. Let me rework this PR!

else sclOut = coerce(prefix + 'colorscale');

// reversescale is handled at the containerOut level
var reverseScale = coerce(prefix + 'reversescale');
Expand Down
24 changes: 24 additions & 0 deletions src/plots/layout_attributes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,30 @@ module.exports = {
editType: 'calc',
description: 'Sets the default trace colors.'
},
colorscale: {
Copy link
Contributor

@etpinard etpinard Nov 21, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Let's put these new attributes and default logic in the Colorscale component module.

Create two new files:

  • src/components/colorscale/layout_attributes.js
  • src/components/colorscale/layout_defaults.js

and link their exports in src/components/colorscale/index.js as layoutAttributes and supplyLayoutDefaults. The latter will get automatically picked up in Plotly.supplyLayoutModuleDefaults here:

plotly.js/src/plots/plots.js

Lines 1512 to 1518 in 695f311

for(component in componentsRegistry) {
_module = componentsRegistry[component];
if(_module.supplyLayoutDefaults) {
_module.supplyLayoutDefaults(layoutIn, layoutOut, fullData);
}
}

editType: 'calc',
sequential: {
valType: 'colorscale',
dflt: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

What would happen if we set dflt: scales.RdBu ?

Copy link
Contributor Author

@antoinerg antoinerg Nov 21, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

That's another way of doing it and I think it's much better than the current approach. We'd have the defaults clearly defined in the attributes where they should be 🎉

role: 'style',
editType: 'calc',
description: 'Sets the default sequential colorscale for positive values.'
},
sequentialminus: {
valType: 'colorscale',
dflt: false,
role: 'style',
editType: 'calc',
description: 'Sets the default sequential colorscale for negative values.'
},
diverging: {
valType: 'colorscale',
dflt: false,
role: 'style',
editType: 'calc',
description: 'Sets the default diverging colorscale.'
}
},
datarevision: {
valType: 'any',
role: 'info',
Expand Down
5 changes: 5 additions & 0 deletions src/plots/plots.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1362,6 +1362,11 @@ plots.supplyLayoutGlobalDefaults = function(layoutIn, layoutOut, formatObj) {
coerce('hidesources');

coerce('colorway');
if(layoutIn.colorscale) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need this?

Copy link
Contributor Author

@antoinerg antoinerg Nov 21, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Coercing valType: colorScale will always give it a default colorscale. I initially wanted it to be falsey if not set by the user. There are other ways to do that.

if(layoutIn.colorscale.sequential) coerce('colorscale.sequential');
if(layoutIn.colorscale.sequentialminus) coerce('colorscale.sequentialminus');
if(layoutIn.colorscale.diverging) coerce('colorscale.diverging');
}

coerce('datarevision');

Expand Down
73 changes: 73 additions & 0 deletions test/jasmine/tests/colorscale_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -296,6 +296,41 @@ describe('Test colorscale:', function() {
handleDefaults(traceIn, traceOut, layout, coerce, opts);
expect(traceOut.showscale).toBe(false);
});

it('should use global colorscale.diverging if no colorscale is specified', function() {
traceIn = {
zmin: -10,
zmax: 10
};
var divergingScale = [[0, 'rgb(0,0,0)'], [1, 'rgb(255,255,255)']];
var layout2 = {
colorscale: {
diverging: divergingScale
},
_dfltTitle: {colorbar: 'cb'}
};
handleDefaults(traceIn, traceOut, layout2, coerce, opts);
expect(traceOut.colorscale).toBe(divergingScale);
});

it('should not use layout colorscale.diverging if colorscale is specified', function() {
var divergingScale = [[0, 'rgb(0,0,0)'], [1, 'rgb(255,255,255)']];
var traceColorscale = [[0, 'rgb(128,128,128)'], [1, 'rgb(255,255,255)']];
traceIn = {
zmin: -10,
zmax: 10,
colorscale: traceColorscale
};

var layout2 = {
colorscale: {
diverging: divergingScale
},
_dfltTitle: {colorbar: 'cb'}
};
handleDefaults(traceIn, traceOut, layout2, coerce, opts);
expect(traceOut.colorscale).toBe(traceColorscale);
});
});

describe('handleDefaults (scatter-like version)', function() {
Expand Down Expand Up @@ -348,6 +383,41 @@ describe('Test colorscale:', function() {
handleDefaults(traceIn, traceOut, layout, coerce, opts);
expect(traceOut.marker.showscale).toBe(true);
});

it('should use layout colorscale.diverging if no colorscale is specified', function() {
traceIn = {
zmin: -10,
zmax: 10
};
var divergingScale = [[0, 'rgb(0,0,0)'], [1, 'rgb(255,255,255)']];
var layout2 = {
colorscale: {
diverging: divergingScale
},
_dfltTitle: {colorbar: 'cb'}
};
handleDefaults(traceIn, traceOut, layout2, coerce, opts);
expect(traceOut.marker.colorscale).toBe(divergingScale);
});

it('should not use layout colorscale.diverging if colorscale is specified', function() {
var divergingScale = [[0, 'rgb(0,0,0)'], [1, 'rgb(255,255,255)']];
var traceColorscale = [[0, 'rgb(128,128,128)'], [1, 'rgb(255,255,255)']];
traceIn = {
marker: {
colorscale: traceColorscale
}
};

var layout2 = {
colorscale: {
diverging: divergingScale
},
_dfltTitle: {colorbar: 'cb'}
};
handleDefaults(traceIn, traceOut, layout2, coerce, opts);
expect(traceOut.marker.colorscale).toBe(traceColorscale);
});
});

describe('calc', function() {
Expand All @@ -366,6 +436,7 @@ describe('Test colorscale:', function() {
autocolorscale: true,
_input: {autocolorscale: true}
};

z = [[0, -1.5], [-2, -10]];
calcColorscale(trace, z, '', 'z');
expect(trace.autocolorscale).toBe(true);
Expand All @@ -376,6 +447,8 @@ describe('Test colorscale:', function() {
trace = {
type: 'heatmap',
z: [[0, -1.5], [-2, -10]],
zmin: -10,
zmax: 0,
autocolorscale: true,
_input: {}
};
Expand Down
8 changes: 7 additions & 1 deletion test/jasmine/tests/parcoords_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,10 @@ describe('parcoords initialization tests', function() {
cauto: true,
autocolorscale: false,
reversescale: false,
showscale: false
showscale: false,
diverging: undefined,
sequential: undefined,
sequentialminus: undefined
});
});

Expand Down Expand Up @@ -278,6 +281,9 @@ describe('parcoords initialization tests', function() {
]
}));

delete(fullTrace.line.diverging);
delete(fullTrace.line.sequential);
delete(fullTrace.line.sequentialminus);
expect(fullTrace.line).toEqual({
color: [35, 63, 21, 42],
colorscale: attributes.line.colorscale.dflt,
Expand Down