I'm going to change the target branch from master to our upcoming major version branch, v4.0. We've made some changes in the Sankey code and I'd like to add your changes against that. If you have time, it would be great to get this in for that version. I'll leave you a review tomorrow.

Thanks for having a look (I have rebased on v4.0 for the moment)

This is a great addition and pretty simple to add. Could you please separate out the direction config into a second attribute? I added a suggestion. This will require changes to your code elsewhere, but I don't think it will be too much work. Let me know what you think

Good idea, I changed it accordingly.

Ah I see. I have changed where node labels are put in vertical layouts.

Old: on node

New: same as horizontal (behind node flow-wise, last nodes before node)

What's your preference? I have added the horizontal baselines and plot-schema, for vertical baselines I will wait till we discussed label positioning.

Let's stick with the existing location. I'll ask for opinions internally. It could be the case that this should be treated as a bug. If that's the case, it can be addressed in a separate PR.

✔

Could you also take a look at sankey/select.js? I think you'll need to make an update there that also handles the reverse direction.

✔

Lastly, link sort and node sort just landed in the v4 branch (in #7873). How do you think that will affect this PR? I think you'll need to merge those changes to make sure things are working.

Is already in my v4.0, based on the mocks there is the question how you would like the sorting in reverse mode (see mocks in last commit). Currently links and nodes get sorted as follows: in horizontal and horizontal reverse mode sorted top to bottom while flow goes horizontally; and in vertical and vertical reverse mode sorted left to right while flow goes vertically. Is this ok, or do you want the sorting to always be in direction of flow (i.e. if looking in flow direction, nodes/links should be sorted from left to right)?

Status Quo:

I just noticed that it looks like you committed the diff image files for some of the new mocks. Could you remove those and add the non-diff images? It might be as simple as renaming the diff files.

Apparently my earlier comment never got posted. I played around with the sorting and I think that it's working correctly right now. @adamreeve might have an opinion on this. Additionally, I've got two requests:

• Could you change the enum value from "reverse" to "reversed"? I talked about it with a colleague and this would more closely match some other uses in the schema.
• Could you update the mock titles to include "reversed" for mocks that use the reversed direction?

Ah sorry, working too early leads to errors :(

I have changed to reversed, added it to mock titles and tried to clean up the baseline images. Reversed baselines will fail, as the title changed. I will re-commit them once the checks have given me new baselines.

This looks good to me thanks @wf-r.

You might want to update the PR title and description to reflect the changed approach.

Currently links and nodes get sorted as follows: in horizontal and horizontal reverse mode sorted top to bottom while flow goes horizontally; and in vertical and vertical reverse mode sorted left to right while flow goes vertically

This behaviour seems most intuitive to me. And it's easy enough to reverse the order of the nodes if someone wants the opposite.

Done. I hope tests will run successfully now (baselines should be fixed, jasmine as well; yet there are some pixel deviations on my windows machine). Let's see what CI says.

I have to fix one test, will do it this evening (European time).

Edit: Done