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

initial implementation of `layout.colorscale` #3274

Merged
merged 15 commits into from Nov 23, 2018

Conversation

Projects
None yet
2 participants
@antoinerg
Copy link
Collaborator

commented Nov 20, 2018

Attempt at closing #2925. As far as I can tell it works well but test coverage is pretty minimal at the moment.

In the process of making this PR, I uncovered issue #3273 .

@etpinard etpinard added this to the v1.43.0 milestone Nov 21, 2018

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

This comment has been minimized.

Copy link
@etpinard

etpinard Nov 21, 2018

Member

Why do we need this?

This comment has been minimized.

Copy link
@antoinerg

antoinerg Nov 21, 2018

Author Collaborator

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.

@@ -188,6 +188,29 @@ module.exports = {
editType: 'calc',
description: 'Sets the default trace colors.'
},
colorscale: {

This comment has been minimized.

Copy link
@etpinard

etpinard Nov 21, 2018

Member

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);
}
}

colorscale: {
sequential: {
valType: 'colorscale',
dflt: false,

This comment has been minimized.

Copy link
@etpinard

etpinard Nov 21, 2018

Member

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

This comment has been minimized.

Copy link
@antoinerg

antoinerg Nov 21, 2018

Author Collaborator

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 🎉

containerOut.sequentialminus = layoutColorscale.sequentialminus;
var dfltScl = containerOut.diverging;
var sclOut;
if(dfltScl) sclOut = coerce(prefix + 'colorscale', dfltScl);

This comment has been minimized.

Copy link
@etpinard

etpinard Nov 21, 2018

Member

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:

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

This comment has been minimized.

Copy link
@etpinard

etpinard Nov 21, 2018

Member

... 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.

This comment has been minimized.

Copy link
@antoinerg

antoinerg Nov 21, 2018

Author Collaborator

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

@antoinerg

This comment has been minimized.

Copy link
Collaborator Author

commented Nov 21, 2018

Commit 93ce241 refactors colorscale_test.js.

I still need to improve test coverage to make sure layout.colorscale.(sequential|sequentialminus|diverging) are used appropriately when autorange: true and no colorscale is provided by the user. It should already work but it should be 🔒 down!

antoinerg added some commits Nov 21, 2018

@antoinerg

This comment has been minimized.

Copy link
Collaborator Author

commented Nov 22, 2018

Commit ee222d5 improves the test coverage!

@@ -42,10 +42,18 @@ module.exports = function calc(gd, trace) {

// auto-z and autocolorscale if applicable
if(hasColorscale(trace, 'marker')) {
colorscaleCalc(trace, trace.marker.color, 'marker', 'c');
colorscaleCalc(gd, trace, {

This comment has been minimized.

Copy link
@etpinard

etpinard Nov 22, 2018

Member

Looking nice 💎

if(min * max < 0) scl = scales.RdBu;
else if(min >= 0) scl = scales.Reds;
else scl = scales.Blues;
if(min * max < 0) scl = gd._fullLayout.colorscale.diverging;

This comment has been minimized.

Copy link
@etpinard

etpinard Nov 22, 2018

Member

minor 🐄 , usually when a method uses fullLayout from gd we put a

var fullLayout = gd._fullLayout;

just below the function (

This comment has been minimized.

Copy link
@antoinerg

antoinerg Nov 22, 2018

Author Collaborator

Done in commit 75e8910

@etpinard

This comment has been minimized.

Copy link
Member

commented Nov 22, 2018

Nicely done @antoinerg

Since this PR will mostly help out template users, we should add a JSON mock that uses one of the new attribute in a template.

"template": {
"layout": {
"title": "heatmap template",
"colorscale": {

This comment has been minimized.

Copy link
@etpinard

etpinard Nov 23, 2018

Member

What if we put autocolorscale: true in the template container, does it still work?

This comment has been minimized.

Copy link
@antoinerg

antoinerg Nov 23, 2018

Author Collaborator

Good call! I modified the mock in dca4350

@etpinard

This comment has been minimized.

Copy link
Member

commented Nov 23, 2018

Nicely done 💃

@antoinerg antoinerg merged commit 922bad6 into master Nov 23, 2018

7 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: publish Your tests passed on CircleCI!
Details
ci/circleci: test-image Your tests passed on CircleCI!
Details
ci/circleci: test-image2 Your tests passed on CircleCI!
Details
ci/circleci: test-jasmine Your tests passed on CircleCI!
Details
ci/circleci: test-jasmine2 Your tests passed on CircleCI!
Details
ci/circleci: test-syntax Your tests passed on CircleCI!
Details

@antoinerg antoinerg deleted the 2925-default-colorbars branch Nov 23, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.