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

Make sankey respect layout.colorway #2277

Merged
merged 2 commits into from
Jan 22, 2018
Merged

Make sankey respect layout.colorway #2277

merged 2 commits into from
Jan 22, 2018

Conversation

alexcjohnson
Copy link
Collaborator

fixes #2276

@monfera another quick one for your review. This slipped through during #2156

Copy link
Contributor

@monfera monfera left a comment

Choose a reason for hiding this comment

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

Nice, very compact PR!

@@ -31,7 +32,7 @@ describe('sankey tests', function() {
var traceOut = { visible: true },
defaultColor = '#444';

Sankey.supplyDefaults(traceIn, traceOut, defaultColor, layout);
Sankey.supplyDefaults(traceIn, traceOut, defaultColor, Lib.extendFlat({colorway: defaultColors}, layout));
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be an overly cautious idea, how about using non-default colors for colorwayhere, or just altering a sankey mock to lock in a visible difference.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good call. Actually, given that ('color/attributes').defaults no longer appears explicitly in the sankey code, seems to me that an end-to-end test of the full default-color case may be the most useful of all, so I just stripped explicit colors out of the dark mock -> f2ebc79

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, makes sense, and works great with a custom colorway too!

@monfera
Copy link
Contributor

monfera commented Jan 22, 2018

💃 thanks Alex, just tried such a thing locally as well, looks great!
image

@alexcjohnson alexcjohnson merged commit e701b5e into master Jan 22, 2018
@alexcjohnson alexcjohnson deleted the sankey-colorway branch January 22, 2018 16:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug something broken
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sankey plot doesn't use layout.colorway
2 participants