tomshanley / d3-sankey-circular

A fork of the d3-sankey library to allow circular links.
MIT License
89 stars 41 forks source link

modular source code? #17

Open micahstubbs opened 6 years ago

micahstubbs commented 6 years ago

I want to break sankeyCircular.js up into may small files so that the interfaces between the functions it contains are more explicit to me.

this is an activity mainly aimed at getting myself familiar with d3-sankey-circular. when I'm happy with it, I'll send a PR to see if this code organization approach is useful to others. (if not, I'm perfectly happy doing this purely as a learning exercise :smile:)

I make an issue because old habits compel me to include an issue number in my branch names 😂

tomshanley commented 6 years ago

Hi Micah - thank you, it would definitely benefit from someone who knows more about modular code, and using bundle etc than me :)

An aside, i'm planning an update (still early stages) that would allow a drawn sankey to be updated with new values, without affecting the overall layout too much (ie the nodes and links would remain pretty much in their position, but with new heights/widths). so it would skip a lot of the initial positioning of nodes/links, update the heights and access the functions to generate the new circular path d. so making some parts more modular would be great help towards that.

micahstubbs commented 6 years ago

ah interesting. good hear that this generally fits with what you have in mind 😄

micahstubbs commented 6 years ago

my rough criteria for abstracting a function out into it's own file seem to be:

if the function is:

1) more than 5 lines of code long
2) is called by multiple other functions

this seems to be optimal for me to get a sense of the dependencies inside the library and easily read the code. this may or may not be optimal for other things that are important to this library.

tomshanley commented 6 years ago

sounds reasonable.. for the update function I mentioned earlier, I'm probably going to need to use addCircularPathData and its dependents (calcVerticalBuffer and createCircularPathString, while updating addCircularPathData to avoid any sorting functions), and probably adapt this adjustNodeHeight

peteruithoven commented 5 years ago

@micahstubbs any updates on this? It might be interesting to check how d3-sankey-diagram is broken up.

micahstubbs commented 5 years ago

@peteruithoven no update. thanks for the signal that it could be useful for you - I may take another look at this. cheers!