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

adding layout.colorway #2156

Merged
merged 4 commits into from
Nov 15, 2017
Merged

adding layout.colorway #2156

merged 4 commits into from
Nov 15, 2017

Conversation

cixzhang
Copy link
Contributor

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

@etpinard
Copy link
Contributor

To add the baseline corresponding to your new mock, see

https://github.com/plotly/plotly.js/blob/master/test/image/README.md#c-generate-or-update-existing-baseline-image

If you're having issues with our docker image-test container, let us know.

@etpinard etpinard added this to the v1.32.0 milestone Nov 13, 2017
valType: 'colorlist',
dflt: colorAttrs.defaults,
role: 'style',
editType: 'calc',
Copy link
Contributor

Choose a reason for hiding this comment

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

Comment for future development: note here that only pie traces require this to be editType: 'calc'. For most trace types, editType: 'plot' would have sufficed and even editType: 'style' might have been good enough for cartesian traces. Oh well, we might have to add-trace-type-specific editType flags for layout attributes down the road to improve performance.

Copy link
Contributor

@etpinard etpinard left a comment

Choose a reason for hiding this comment

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

@cixzhang looks great. I'm glad you were able to generate the baseline.

I noticed a few things while making what I thought would be a final review.

Sorry for not noticing them sooner 😏

"labels": ["a","b","c","c","c","a","d","e","f","f","g","h"],
"type": "pie",
"domain": {"x": [0, 0.4]},
"xaxis": "x2",
Copy link
Contributor

Choose a reason for hiding this comment

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

xaxis and yaxis don't do anything for pie traces. Would you mind 🔪 them?

"domain": [0.4, 1]
},
"yaxis": {
"anchor": "y2"
Copy link
Contributor

Choose a reason for hiding this comment

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

anchor for y-axes expects x-axis ids, so this thing here gets ignored. 🔪

@@ -141,6 +141,22 @@ exports.valObjectMeta = {
else propOut.set(dflt);
}
},
colorlist: {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would you mind adding a Lib.validate test case for the new colorlist valType?

@etpinard
Copy link
Contributor

A very lovely addition ❤️

Thanks very much @cixzhang 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature something new
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants