feat: Allow fixing the vertical order of Sankey links, nodes - #7873
Conversation
Sorry, something went wrong.
camdecoster
left a comment
There was a problem hiding this comment.
This seems great. I've requested a few small changes. What do you think of adding support for link sort?
Sorry, something went wrong.
| right: d3Sankey.sankeyRight, | ||
| center: d3Sankey.sankeyCenter | ||
| }[trace.node.align]; | ||
| var input_sort = trace.node.sort === 'input'; |
There was a problem hiding this comment.
| var input_sort = trace.node.sort === 'input'; | |
| const inputNodeSort = trace.node.sort === 'input'; |
Sorry, something went wrong.
| if(circular && input_sort) { | ||
| Lib.error('Circular Sankey diagrams do not support the "input" node.sort mode; falling back to the default sort.'); | ||
| } | ||
|
|
There was a problem hiding this comment.
| if(circular && input_sort) { | |
| Lib.error('Circular Sankey diagrams do not support the "input" node.sort mode; falling back to the default sort.'); | |
| } |
Sorry, something went wrong.
| // d3-sankey-circular does not support the nodeSort method | ||
| if(!circular) { | ||
| sankey.nodeSort(input_sort ? null : undefined); | ||
| } |
There was a problem hiding this comment.
| } | |
| if (inputNodeSort) { | |
| if (circular) { | |
| Lib.warn('Circular Sankey diagrams do not support the "input" node.sort mode; falling back to the default sort.'); | |
| } else { | |
| // Passing null keeps nodes in their input order | |
| sankey.nodeSort(null); | |
| } | |
| } |
Sorry, something went wrong.
| 'If the value is `auto` (the default), the vertical order of nodes will be determined automatically', | ||
| 'by the layout.', | ||
| 'If the value is `input`, the vertical order is kept the same as the order in the input node array' |
There was a problem hiding this comment.
| 'If the value is `auto` (the default), the vertical order of nodes will be determined automatically', | |
| 'by the layout.', | |
| 'If the value is `input`, the vertical order is kept the same as the order in the input node array' | |
| 'For `auto` (the default), the vertical order of nodes will be determined automatically by the layout.', | |
| 'For `input`, the vertical order of nodes is kept the same as the order in the input array.' |
Sorry, something went wrong.
There was a problem hiding this comment.
I think this mock can be deleted since all of the other sankey mocks use the auto sort.
Sorry, something went wrong.
| } | ||
| ], | ||
| "layout": { | ||
| "title": { "text": "Sankey with fixed vertical node ordering" }, |
There was a problem hiding this comment.
| "title": { "text": "Sankey with fixed vertical node ordering" }, | |
| "title": { "text": "Sankey with vertical node ordering matching the input array" }, |
Sorry, something went wrong.
| var errors = []; | ||
| spyOn(Lib, 'error').and.callFake(function (msg) { | ||
| errors.push(msg); | ||
| }); |
There was a problem hiding this comment.
| var errors = []; | |
| spyOn(Lib, 'error').and.callFake(function (msg) { | |
| errors.push(msg); | |
| }); | |
| var warnings = []; | |
| spyOn(Lib, 'warn').and.callFake(function (msg) { | |
| warnings.push(msg); | |
| }); |
Sorry, something went wrong.
| expect(d3SelectAll('.sankey .node-rect').size()).toBeGreaterThan(0); | ||
|
|
||
| // An error is logged about the fallback | ||
| expect(errors.length).toBe(1); |
There was a problem hiding this comment.
| expect(errors.length).toBe(1); | |
| expect(warnings.length).toBe(1); |
Sorry, something went wrong.
|
FYI, we're tying to get an RC of v4.0 out this week, so I'm going to jump in and make some changes. I'd rather let you finish things up, but schedules, right? Your work has been very helpful and appreciated. |
Sorry, something went wrong.
|
OK thanks @camdecoster! Your changes all make sense to me but I've just been quite busy with work stuff lately. I'm not familiar with the link sort feature but am happy to take a look into that. I see you added the link sort here too, thanks! |
Sorry, something went wrong.
Fixes #4373
Follow up to #7830
This exposes the nodeSort method added to d3-sankey, to allow disabling the default ordering algorithm and enforcing that the vertical order should match the order of nodes in the input data.
Note that the nodeSort method in d3-sankey can be a method that compared two nodes (see the docs), but for many users I think just providing a way to pass null instead of the default undefined is sufficient. But maybe this could be future-proofed somehow to support using a comparison method in future? Although I'm not sure how feasible it is to make that work in plotly.
The nodeSort option is not supported by d3-sankey-circular, so is not used for circular Sankey diagrams (see #7689 (comment) for more context).
EDIT (by @camdecoster): I added the corresponding sort option for links as well. Everything that @adamreeve said above is true for this attribute (linkSort) as well.