Skip to content

Conversation

@antoinerg
Copy link

In order to fix this issue plotly/plotly.js#3140, we override nodePadding if it's too large.

@antoinerg
Copy link
Author

antoinerg commented Oct 23, 2018

I added the built files to commit 6f19390 to enable easy inclusion in plotly.js' package.json file. The reason I had to do this is probably because plotly.js is ES5 whereas this module uses ES6!

src/sankey.js Outdated
var L = max(nodesByBreadth, function(nodes) {
return nodes.length;
});
var maxNodePadding = 2/3 * size[1] / (L - 1);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I like the 2/3 - looks good. Lets define it up top though by the properties block:

d3-sankey/src/sankey.js

Lines 7 to 11 in 6f19390

nodeWidth = 24,
nodePadding = 8,
size = [1, 1],
nodes = [],
links = [];

not that I think we need to make it a property now, though we may want to do that later. For now I just want it to be easier to find, in case someone's wondering why they get our warning (or why the padding refuses to get bigger)

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for pointing this out! It should now be resolved in commit 1a79abb!

@alexcjohnson
Copy link
Collaborator

💃 Very nice and clean!

@antoinerg antoinerg merged commit e08cc70 into master Oct 24, 2018
@antoinerg antoinerg deleted the fix-large-padding branch October 24, 2018 19:24
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.

3 participants