Skip to content
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

Add midpoint attr to color scales #3549

Merged
merged 7 commits into from
Feb 19, 2019
Merged
Show file tree
Hide file tree
Changes from 6 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
17 changes: 17 additions & 0 deletions src/components/colorscale/attributes.js
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,8 @@ module.exports = function colorScaleAttrs(context, opts) {
var auto = cLetter + 'auto';
var min = cLetter + 'min';
var max = cLetter + 'max';
var mid = cLetter + 'mid';
var autoFull = code(contextHead + auto);
var minFull = code(contextHead + min);
var maxFull = code(contextHead + max);
var minmaxFull = minFull + ' and ' + maxFull;
Expand Down Expand Up @@ -160,6 +162,21 @@ module.exports = function colorScaleAttrs(context, opts) {
].join('')
};

attrs[mid] = {
valType: 'number',
role: 'info',
dflt: null,
editType: 'calc',
impliedEdits: autoImpliedEdits,
description: [
'Sets the mid-point of the color domain by scaling ', minFull,
' and/or ', maxFull, ' to be equidistant to this point.',
effectDesc,
' Value should have the same units as ', colorAttrFull, '. ',
'Has no effect when ', autoFull, ' is `false`.'
].join('')
};

attrs.colorscale = {
valType: 'colorscale',
role: 'style',
Expand Down
11 changes: 11 additions & 0 deletions src/components/colorscale/calc.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,11 @@ module.exports = function calc(gd, trace, opts) {
var autoAttr = cLetter + 'auto';
var minAttr = cLetter + 'min';
var maxAttr = cLetter + 'max';
var midAttr = cLetter + 'mid';
var auto = container[autoAttr];
var min = container[minAttr];
var max = container[maxAttr];
var mid = container[midAttr];
var scl = container.colorscale;

if(auto !== false || min === undefined) {
Expand All @@ -36,6 +38,15 @@ module.exports = function calc(gd, trace, opts) {
max = Lib.aggNums(Math.max, null, vals);
}

if(auto !== false && mid !== undefined) {
if(max - mid > mid - min) {
min = mid - (max - mid);
}
else if(max - mid < mid - min) {
max = mid + (mid - min);
}
}

if(min === max) {
min -= 0.5;
max += 0.5;
Expand Down
11 changes: 8 additions & 3 deletions src/components/colorscale/defaults.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,14 @@ module.exports = function colorScaleDefaults(traceIn, traceOut, layout, coerce,
var minIn = containerIn[cLetter + 'min'];
var maxIn = containerIn[cLetter + 'max'];
var validMinMax = isNumeric(minIn) && isNumeric(maxIn) && (minIn < maxIn);
coerce(prefix + cLetter + 'auto', !validMinMax);
coerce(prefix + cLetter + 'min');
coerce(prefix + cLetter + 'max');
var auto = coerce(prefix + cLetter + 'auto', !validMinMax);

if(auto) {
coerce(prefix + cLetter + 'mid');
} else {
coerce(prefix + cLetter + 'min');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this line and the next used to always run, but now only conditionally... might this break anything? I'm not sure about the implication of not calling coerce().

Copy link
Contributor

Choose a reason for hiding this comment

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

No tests are failing because of it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is a test failure that mentions coerce:

Test surface supplyDefaults should coerce 'c' attributes with 'c' values regardless of `'z' if 'c' is present FAILED

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah good eye.

Well cmin and cmax have no effect

if(auto !== false || min === undefined) {
min = Lib.aggNums(Math.min, null, vals);
}
if(auto !== false || max === undefined) {
max = Lib.aggNums(Math.max, null, vals);
}

when cauto is true.

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'm easy. Like I said, I don't know the impact of calling coerce and ignoring the attr vs not calling coerce :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Calling the behaviour correct and adapting the test in ff67812

coerce(prefix + cLetter + 'max');
}

// handles both the trace case (autocolorscale is false by default) and
// the marker and marker.line case (autocolorscale is true by default)
Expand Down
1 change: 1 addition & 0 deletions src/traces/scattermapbox/attributes.js
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ module.exports = overrideAll({
cauto: markerAttrs.cauto,
cmax: markerAttrs.cmax,
cmin: markerAttrs.cmin,
cmid: markerAttrs.cmid,
autocolorscale: markerAttrs.autocolorscale,
reversescale: markerAttrs.reversescale,
showscale: markerAttrs.showscale,
Expand Down
Binary file added test/image/baselines/cmid-zmid.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
43 changes: 43 additions & 0 deletions test/image/mocks/cmid-zmid.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
{
"data": [{
"y": [1, 2, 1],
"mode": "markers",
"marker": {
"size": 20,
"color": [-1, 1, 10],
"cmid": "0",
"colorbar": {
"len": 0.3,
"y": "1",
"yanchor": "top",
"title": {"text": "marker.cmid=0", "side": "right"}
}
}
}, {
"type": "heatmap",
"z": [[1,2], [2, 3]],
"zmid": -1,
"colorbar": {
"len": 0.3,
"y": "0.5",
"yanchor": "middle",
"title": {"text": "zmid=-1", "side": "right"}
}
}, {
"type": "bar",
"y": [0.5, 1, 0.5],
"name": "marker.line.cmid=2",
"hoverlabel": {"namelength": -1},
"marker": {
"line": {
"width": 2,
"color": [-1, -1, -10],
"cmid": 2
}
}
}],
"layout": {
"title": {"text": "Traces with set <b>cmid</b> / <b>zmid</b>"},
"showlegend": false
}
}
71 changes: 71 additions & 0 deletions test/jasmine/tests/plot_api_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1195,6 +1195,77 @@ describe('Test plot api', function() {
.then(done);
});

it('turns on cauto when cmid is edited', function(done) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is an odd description... cmid only has effect if cauto is true, but doesn't turn on cauto per se, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

but doesn't turn on cauto per se, right?

yes, it has to. If not, restyle(gd, 'marker.cmid', 0) wouldn't work.

Copy link
Contributor

@etpinard etpinard Feb 19, 2019

Choose a reason for hiding this comment

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

by "not work" here I mean it wouldn't apply that new cmid value when cmin and cmax are set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK I see what you mean.

function _assert(msg, exp) {
return function() {
var mk = gd._fullData[0].marker;
for(var k in exp) {
expect(mk[k]).toBe(exp[k], [msg, k].join(' - '));
}
};
}

function _restyle(arg) {
return function() { return Plotly.restyle(gd, arg); };
}

Plotly.plot(gd, [{
mode: 'markers',
y: [1, 2, 1],
marker: { color: [1, -1, 4] }
}])
.then(_assert('base', {
cauto: true,
cmid: undefined,
cmin: -1,
cmax: 4
}))
.then(_restyle({'marker.cmid': 0}))
.then(_assert('set cmid=0', {
cauto: true,
cmid: 0,
cmin: -4,
cmax: 4
}))
.then(_restyle({'marker.cmid': -2}))
.then(_assert('set cmid=-2', {
cauto: true,
cmid: -2,
cmin: -8,
cmax: 4
}))
.then(_restyle({'marker.cmid': 2}))
.then(_assert('set cmid=2', {
cauto: true,
cmid: 2,
cmin: -1,
cmax: 5
}))
.then(_restyle({'marker.cmin': 0}))
.then(_assert('set cmin=0', {
cauto: false,
cmid: undefined,
cmin: 0,
cmax: 5
}))
.then(_restyle({'marker.cmax': 10}))
.then(_assert('set cmin=0 + cmax=10', {
cauto: false,
cmid: undefined,
cmin: 0,
cmax: 10
}))
.then(_restyle({'marker.cauto': true, 'marker.cmid': null}))
.then(_assert('back to cauto=true', {
cauto: true,
cmid: undefined,
cmin: -1,
cmax: 4
}))
.catch(failTest)
.then(done);
});

it('turns off autobin when you edit bin specs', function(done) {
// test retained (modified) for backward compat with new autobin logic
var start0 = 0.2;
Expand Down
2 changes: 1 addition & 1 deletion test/jasmine/tests/scattergeo_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ describe('Test scattergeo defaults', function() {
describe('Test scattergeo calc', function() {

function _calc(opts) {
var base = { type: 'scattermapbox' };
var base = { type: 'scattergeo' };
var trace = Lib.extendFlat({}, base, opts);
var gd = { data: [trace] };

Expand Down