Skip to content

Expose the node alignment property for Sankey plots#6800

Merged
archmoj merged 6 commits intoplotly:masterfrom
adamreeve:sankey-align
Dec 12, 2023
Merged

Expose the node alignment property for Sankey plots#6800
archmoj merged 6 commits intoplotly:masterfrom
adamreeve:sankey-align

Conversation

@adamreeve
Copy link
Copy Markdown
Contributor

@adamreeve adamreeve commented Nov 28, 2023

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 archmoj added feature something new community community contribution status: reviewable labels Dec 5, 2023
@archmoj
Copy link
Copy Markdown
Contributor

archmoj commented Dec 5, 2023

🏆
LGTM.
Over to @alexcjohnson and @antoinerg

@alexcjohnson
Copy link
Copy Markdown
Collaborator

alexcjohnson commented Dec 5, 2023

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
Copy Markdown
Contributor Author

adamreeve commented Dec 5, 2023

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
Copy Markdown
Contributor Author

adamreeve 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.

@archmoj
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

adamreeve commented Dec 6, 2023

Thanks, looks like that fixed it!

alexcjohnson
alexcjohnson approved these changes Dec 12, 2023
View reviewed changes
Copy link
Copy Markdown
Collaborator

@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
@adamreeve adamreeve deleted the sankey-align branch February 22, 2024 20:11
@FlorentinTh FlorentinTh mentioned this pull request May 24, 2024
Open
@FlorentinTh FlorentinTh mentioned this pull request Jul 17, 2024
Open
@GregBrimble GregBrimble mentioned this pull request Aug 4, 2024
Open
@FlorentinTh FlorentinTh mentioned this pull request Aug 9, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alexcjohnson alexcjohnson alexcjohnson approved these changes

@antoinerg antoinerg Awaiting requested review from antoinerg

Assignees

No one assigned

Labels

community community contribution feature something new

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Sankey nodeAlign?

3 participants

@adamreeve @archmoj @alexcjohnson