[charts-pro] Sankey should respect node order #20065
Conversation
JCQuintas
commented
Oct 22, 2025
JCQuintas
commented
- I have followed (at least) the PR section of the contributing guide.
|
Deploy preview: https://deploy-preview-20065--material-ui-x.netlify.app/ Updated pages: Bundle size report
|
CodSpeed Performance ReportMerging #20065 will not alter performanceComparing Summary
Benchmarks breakdown
Footnotes |
| if (nodeSort) { | ||
| sankeyGenerator = sankeyGenerator.nodeSort(nodeSort); | ||
| } | ||
| sankeyGenerator = sankeyGenerator.nodeSort(defaultNodeSort); |
There was a problem hiding this comment.
Not sure you need to get back the returned value.
| sankeyGenerator = sankeyGenerator.nodeSort(defaultNodeSort); | |
| sankeyGenerator.nodeSort(defaultNodeSort); |
I gave a look at the D3 code, and it look like nodeSort is just a getter if no arguments, and a setter if args are provided.
sankey.nodeSort = function(_) {
return arguments.length ? (sort = _, sankey) : sort;
};By the way do wee need that. According to the docs, you should be able to do sankeyGenerator.nodeSort(null); for the same result.
If sort is null, the order is fixed by the input. Otherwise, the specified sort function determines the order;
https://github.com/d3/d3-sankey/tree/master?tab=readme-ov-file#sankey_nodeSort
The sort function you set with sankey.nodeSort appears 3 time in the code
// in relaxLeftToRight()
if (sort === undefined) column.sort(ascendingBreadth);
// in relaxRightToLeft()
if (sort === undefined) column.sort(ascendingBreadth);
// in computeNodeLayers
if (sort) for (const column of columns) {
column.sort(sort);
}
From what I understand, if not defined, the sankey tries to move nodes the best they can with ascendingBreadth
If sort is not undefined, they sort columns according to the sort function.
And so if the sort is null (or false) it's not equal to undefined and it's falsy so the scritp never touch to the column node order :)
Should we have a boolean parameter to keep the ascendingBreadth behavior? For exampel the node sort function coudl get values 'auto' or 'fix' corresponding respectively to undefined and null in D3
Becasue the result of node placement automatisation is not that bad when looking at argos
There was a problem hiding this comment.
By the way do wee need that. According to the docs, you should be able to do sankeyGenerator.nodeSort(null); for the same result.
True, just the types don't accept that.
My initial intention (current behaviour) was to always use null, but then I was doing if nodeSort check, which would always ignore the null value 😆
I don't think the "auto" is any better than the null and it more often than not is produces exactly the same result.
In the end if the user wants a specific behaviour they can set it themselves.
Now the code should always be null or function, and should fix the ordering issues.
There was a problem hiding this comment.
Nvm... I skipped the null previously because if both are null then the linkSort doesn't usually work. 😆
There was a problem hiding this comment.
@JCQuintas for types: DefinitelyTyped/DefinitelyTyped#73953
There was a problem hiding this comment.
In the end if the user wants a specific behaviour they can set it themselves.
Except if the sankey data come from a database that does not care about the layout. When looking at argos screen shot before looks better than after this PR
There was a problem hiding this comment.
When looking at argos screen shot before looks better than after this PR
Yeah cause:
Nvm... I skipped the null previously because if both are null then the linkSort doesn't usually work. 😆
There was a problem hiding this comment.
I'll find a better solution 😆
This reverts commit ff366b5.
github-actions
bot
added
the
PR: out-of-date
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
3 participants
