Skip to content

adding layout.colorway#2

Open
cixzhang wants to merge 9 commits into
masterfrom
layout-colorway
Open

adding layout.colorway#2
cixzhang wants to merge 9 commits into
masterfrom
layout-colorway

Conversation

@cixzhang

@cixzhang cixzhang commented Nov 2, 2017

Copy link
Copy Markdown
Owner

For plotly#2136

This PR adds the layout.colorway attribute which takes a list of colors to use as the default set of colors for all traces. By default, it should use the existing Color.defaults.

Comment thread src/plots/plots.js Outdated
newFullLayout._initialAutoSizeIsDone = true;

// pass along trace colors
newFullLayout._colorway = newLayout.colorway;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

That works, but we can do better. Ideally, we should use Lib.coerce to validate and map colorway from input layout to full layout. layout.colorway is a bit of a special case: we don't have a valType that corresponds to the desired behavior. That said, we should at the very least make sure that colorway is an array and (manually) validate every color in the colorway.

Comment thread src/plots/layout_attributes.js Outdated
description: 'Determines whether or not a legend is drawn.'
}
},
colorway: colorAttrs.defaults,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This should be:

colorway: {
  valType: 'any', // for now ;)
  dflt: colorAttrs.defaults,
  role: 'style',
  editType: 'plot' // (maybe 'calc')
  description: 'describe this thing!'
}

Comment thread src/plot_api/helpers.js Outdated
// Make a few changes to the data right away
// before it gets used for anything
exports.cleanData = function(data, existingData) {
exports.cleanData = function(data, existingData, colorway) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

cleanData is intended for legacy code only, so maybe we don't have to propagate colorway here.

@etpinard

etpinard commented Nov 2, 2017

Copy link
Copy Markdown

@cixzhang looking great! Thanks very much for doing this! Let me know if you have any questions.

@cixzhang

cixzhang commented Nov 3, 2017

Copy link
Copy Markdown
Owner Author

@etpinard Thanks so much for the suggestions! I've implemented your comments. Let me know if there's anything more I should do!

@cixzhang

cixzhang commented Nov 3, 2017

Copy link
Copy Markdown
Owner Author

I noticed that my implementation of the colorway in pie/calc was causing every other pie chart to use the same colors, so I made some modifications to prevent that. I'm not too sure my approach was the best one. Let me know if you have any suggestions!

@etpinard

Copy link
Copy Markdown

Great @cixzhang - this is looking pretty nice.

Would you mind squashing your commits into one and making a PR to the main plotly.js repo?

@cixzhang

Copy link
Copy Markdown
Owner Author

Thanks for all your help! I made a new branch with the squashed changes to avoid force pushing: plotly#2156.

Since I added a mock, the CI test failed. I'm guessing I'm missing a step to register the new mock. Any advice?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants