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

Optional node.fixedValue to override size of node #40

Merged
merged 6 commits into from
Sep 1, 2019

Conversation

sillitoe
Copy link
Contributor

We have a use case where it is useful to allow the size of each node to be larger than the sum of the link.value (overriding the default case).

e.g.

image

I can't figure out a clean way of implementing this in the current API, so I've added a local workaround that allows an optional node.value to be added to the existing Math.max calculation.

Is this (or a better implementation) something that might be useful to others?

Thanks,

Ian

@elliotchi
Copy link

+1 on this. Can we get this merged?

@dandelany
Copy link

+1, this would be really valuable for my use-case as well! Would be great if this were supported.

@craigmcginley
Copy link

+1

@bvoisin
Copy link

bvoisin commented May 13, 2018

+1, can we help to get it merged ?

@geekplux
Copy link
Contributor

geekplux commented Jun 1, 2018

+1, this would be really useful

@sillitoe
Copy link
Contributor Author

sillitoe commented Jun 1, 2018

@mbostock ?

@mbostock
Copy link
Member

mbostock commented Mar 3, 2019

The issue I see with this design is that you can’t run the Sankey layout again on the same input: the node.value set by the previous run of the Sankey layout becomes the new minimum value of the node. So, this is not strictly backwards-compatible.

We tackled a similar issue in d3/d3-hierarchy#9 by wrapping the input data, so that the layout isn’t overwriting the same value it reads from. I think we’ll probably want to do that with the Sankey input, too—to treat the input data as immutable, and to wrap it with whatever the layout generates.

A super-simple way of avoiding this problem would be to read from node.minValue instead.

@sillitoe
Copy link
Contributor Author

sillitoe commented Mar 5, 2019

Thanks, yes that seems a much better design - added in c03678f.

@sillitoe sillitoe changed the title Optional node.value to allow for larger nodes. Optional node.minValue to allow for larger nodes. Mar 5, 2019
@mbostock
Copy link
Member

mbostock commented Mar 5, 2019

Thanks. Would it be better specify a node.fixedValue, rather than a node.minValue? Then you would say:

node.value = node.fixedValue === undefined
    ? Math.max(sum(node.sourceLinks, value), sum(node.targetLinks, value))
    : node.fixedValue;

@sillitoe
Copy link
Contributor Author

sillitoe commented Mar 5, 2019

I figured this might cause a rendering problem if the nodes were smaller than the incoming links?

@mbostock
Copy link
Member

mbostock commented Mar 5, 2019

Yep, but isn’t that more of a data problem than a rendering problem? It seems better to do that than to ignore the input value.

@sillitoe
Copy link
Contributor Author

sillitoe commented Mar 5, 2019

Yes, makes sense. Looks good to me.

@sillitoe sillitoe changed the title Optional node.minValue to allow for larger nodes. Optional node.fixedValue to override size of node Mar 5, 2019
@vdubr
Copy link

vdubr commented Aug 30, 2019

Hi, what is the state of this pull request? This function would be very useful for us. Can I help?

@mbostock mbostock merged commit 2197f17 into d3:master Sep 1, 2019
@vdubr
Copy link

vdubr commented Sep 2, 2019

Thanks

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

Successfully merging this pull request may close these issues.

8 participants