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

Expose the node alignment property for Sankey plots #6800

Merged
merged 6 commits into from Dec 12, 2023

Conversation

adamreeve
Copy link
Contributor

Fixes #1938

This is based on the changes linked in the comment at #1938 (comment), which I've updated to work with the current code.

@archmoj
Copy link
Contributor

archmoj commented Dec 5, 2023

🏆
LGTM.
Over to @alexcjohnson and @antoinerg

@alexcjohnson
Copy link
Contributor

Great! So this works with the @plotly/d3-sankey we already have, doesn't need upstream changes?

Has this been tested with circular links in the diagram? Can we maybe add one mock showing this?

@adamreeve
Copy link
Contributor Author

Yes the nodeAlign method is already in @plotly/d3-sankey so this doesn't need changes anywhere else.

This should also work with circular links, and actually the original changes I linked to above had a test mock using circular links so I will add that back in.

@adamreeve
Copy link
Contributor Author

I'm not sure why the baseline test is now failing for some mocks when it was passing previously, the failing mocks aren't ones I've added.

@archmoj
Copy link
Contributor

archmoj commented Dec 5, 2023

I'm not sure why the baseline test is now failing for some mocks when it was passing previously, the failing mocks aren't ones I've added.

There is a bug in our parallel image test now. To avoid that please rename your new mock and baseline to start with letter zz- to run in the end of process.
Thank you!

@adamreeve
Copy link
Contributor Author

Thanks, looks like that fixed it!

Copy link
Contributor

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

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

💃 Great! Very well done @adamreeve!

@archmoj archmoj merged commit 17f7013 into plotly:master Dec 12, 2023
1 check passed
@adamreeve adamreeve deleted the sankey-align branch February 22, 2024 20:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sankey nodeAlign?
3 participants