Add support for additional Sankey diagram orientations

[jayaddison]

Adds orientation-specific sankey generator constructors: sankeyTop (top-to-bottom), sankeyRight (left-to-right), sankeyBottom (bottom-to-top) and sankeyLeft (right-to-left).

A new function linkShape is exposed on the sankey generator which allows the caller to retrieve the 'correct' D3 link shape matching the orientation of the constructor.

This was developed against a working copy of https://github.com/LonnyGomes/sankey-diagram-poc, with one styling modification applied for vertical mode: updating text element positioning logic to improve readability.

Bottom-to-top

Top-to-bottom

Right-to-left

Left-to-right

Resolves #55

[jayaddison]

@mbostock I'm fairly new to modifying any d3-related code, but was keen to introduce this feature for some work I have in progress. Does this seem like a reasonable approach towards introducing vertical orientation support for sankey digrams? Glad to incorporate any feedback/suggestions.

[curran]

Haven't read every line but generally looks good to me!

[jayaddison]

Thanks @curran! - I decided to make one further code change here which is to remove the use of ES6 destructuring during variable assignments (just in case that helps ease any migration/compatibility concerns). And realized I forgot to add a little bit of documentation, which is now in place too.

[curran]

Generally looks great! Very minor nit on indentation.

[mbostock]

Related d3/d3-hierarchy#63.

I was considering for the hierarchical layouts to have separate constructors for each orientation, as we do with d3-axis, rather than having a getter-setter property. So for example you’d call d3.sankeyLeft() instead of d3.sankey() to get a left-to-right oriented Sankey diagram. But, I’m not totally wed to that idea. (It made sense for an axis because there’s not an appropriate default orientation, and also d3-axis doesn’t support transitioning between orientations.)

Another consideration here is that we’re already using d3.sankeyLeft, d3.sankeyRight, d3.sankeyCenter and d3.sankeyJustify for the sankey.nodeAlign property, and we have to be careful about conflicting with those names. We definitely can’t use the names d3.orientLeft, d3.orientRight etc. because it will not be clear that these symbols are only for use by d3-sankey and not in other contexts.

One option would be to rename the current methods to d3.sankeyAlignLeft, d3.sankeyAlignRight, etc. Then we could use d3.sankeyOrientLeft, d3.sankeyOrientRight, etc. for sankey.orient. (This is one argument for favoring string names "left", "right", etc. since you don’t need an explicit symbol or worry about conflict…) Or we could break backwards-compatibility and have d3.sankeyLeft, d3.sankeyRight, etc. be constructors.

Do you have a preference? I’d like to be consistent with d3-hierarchy, but I could probably be convinced to break consistency with d3-axis.

[jayaddison]

Thanks for the feedback - I'm swayed by the idea of consistency for both situations: when using additional components from the set available, it seems best for developer experience if both the API and the naming are similar (even if the implementations are different).

If that sounds reasonable I'd plan to make the following changes next:

• Refactor the code so that an orientation is selected based on the constructor invoked by the caller
• Rename the existing alignment options so that these become d3.sankeyAlignLeft, ...
• Introduce consistency with Layouts should have orientations. d3-hierarchy#63 by exporting orientation names such as d3.sankeyLeft, ...
• Perform a major version bump to 0.13 so that existing clients do not accidentally run into breaking changes

[curran]

Looks good to me! Handing off to @mbostock or @Fil for final review.

[curran]

@jayaddison I did not verify the build locally or run it on any examples. Please double check that it runs after all the recent changes, if you haven't already. Also, how would one go about testing it locally? I'm not sure. Maybe an example for reference is somewhere? Thanks!

[jayaddison]

Thanks @curran! Yep, I've been using https://github.com/LonnyGomes/sankey-diagram-poc throughout development to verify diagram rendering - not on every commit to be honest, but during previous development and after your latest round of review.

It would be nice to have more in-library test coverage; npm test does continue to pass but I haven't expanded the test coverage so far.

[curran]

Ah I see! It might be a good move to add test coverage for the new orientations. At least a little bit.

[jayaddison]

Yep, good idea. That's led to #86 and #87 as follow-up tasks; if we could merge those (and perhaps #80 as well?) then I'll incorporate their changes into this branch before adding additional test coverage.

[curran]

LGTM!

[Crustquake]

When will this pull request be merged approximately? I really need it for my task. Thanks.

[jayaddison]

@curran @mbostock ping

[jayaddison]

I'll also note https://github.com/jayaddison/d3-sankey/issues/1, that has been worrying me a little bit because I don't completely understand the cause yet.

I don't think that issue implies much potential for risk for this pull request, but in some ways I would be happier with a fresh implementation (again after re-enabling unit tests for this repository and updating the dependencies as per #86 and #87).

[curran]

I'd merge it if I could ¯_(ツ)_/¯

I can share this PR out to the wider D3 developer community and see if anyone might be able to take on maintenance of this repo. I think it's an issue of limited developer bandwidth and/or interest to dedicate to this repo.

[jayaddison]

@curran Sure thing, that seems like a good assessment and I'd appreciate that. There's no urgency to it; I'd prefer we take our time and get it reviewed thoroughly. Perhaps with that in mind it could be one good way for a new maintainer to get involved.

[Crustquake]

Is only @mbostock can merge? Maybe we should contact him somehow. He is active on github, the account is not abandoned.

[curran]

Yes only he can merge here, I believe.

[jayaddison]

@tomshanley Fairly random ping here - I was searching around to try to find what level of community interest there is for vertical D3 sankey diagrams and that led me to this ObservableHQ post of yours.

It looks like you use a canvas transform in that case to switch between horizontal and vertical - that makes a lot of sense and is quite dynamic.

I was wondering if you have any recommendations based on industry experience about whether visualization practitioners would find alignment support in-library for d3-sankey useful, or whether canvas operations are preferred (or becoming preferred)? I could imagine situations in which the ability to transform a diagram at either the component level or the canvas level could be confusing.

(speak of confusing: I'm not really very involved in datavizualization, so please forgive probably slightly vague and inaccurate use of terminology)

[tomshanley]

@tomshanley Fairly random ping here - I was searching around to try to find what level of community interest there is for vertical D3 sankey diagrams and that led me to this ObservableHQ post of yours.

It looks like you use a canvas transform in that case to switch between horizontal and vertical - that makes a lot of sense and is quite dynamic.

I was wondering if you have any recommendations based on industry experience about whether visualization practitioners would find alignment support in-library for d3-sankey useful, or whether canvas operations are preferred (or becoming preferred)? I could imagine situations in which the ability to transform a diagram at either the component level or the canvas level could be confusing.

(speak of confusing: I'm not really very involved in datavizualization, so please forgive probably slightly vague and inaccurate use of terminology)

Absolutely - This PR would be very useful to me. The transform I use in that Observable Notebook is OK, but I don't think its very elegant in the long term

[jayaddison]

Thanks, @tomshanley.

@mbostock When you have time, do you have any suggestions about ways to progress this pull request? Glad to make further adjustments and changes.

[jayaddison]

Ping @mbostock

[jayaddison]

Sorry @mbostock - one more ping to attempt to see whether these changes are acceptable; I think I'll close this pull request soon otherwise.

[curran]

@jayaddison Have you considered releasing a fork with this on NPM? I think these changes are great!

[simPod]

I asked on D3 slack and there seems to be no incentive to maintain this repo anymore

https://d3js.slack.com/archives/C0ACH1TUY/p1657486050771439?thread_ts=1655915282.245839&cid=C0ACH1TUY

[jayaddison]

Yep, those are fair reasons to be cautious about accepting changes. In general, maintenance and development burdens seem likely to increase more rapidly as code quality diminishes.

We're at an impasse in this case, though: I'm not keen to maintain a fork of this package, because I don't understand it particularly deeply (meaning that I can't effectively estimate the quality of my own contributions, letalone explain, support and provide future direction for the package - so I don't think I'd serve the community well by trying to).

(I think this can be a common problem with open source software: for any given ecosystem, there may be a limited number of people who really "grok" the problem space and are able to guide improvements -- that's not me in this case, and I recognize that. I made some changes to fit my use-case, and based on that effort investment, am happy to invest further to reach D3's acceptance criteria, but I'm not maintainer material)