SankeyChart: Add chart for displaying dataflow#11919
SankeyChart: Add chart for displaying dataflow#11919danielchalmers merged 13 commits intoMudBlazor:devfrom
Conversation
mudbot
bot
added
the
new component
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new Sankey chart component, which is a fantastic addition to the library. The implementation is comprehensive, including the component logic, data models, documentation with examples, and unit tests. I've identified a few areas for improvement. There's a critical bug in color handling that could cause a crash, a logical error in how node values are calculated, and missing input validation on chart options. Additionally, I've provided suggestions to enhance performance, improve the documentation's accuracy, and make the examples more robust. Overall, this is a very impressive first contribution, and addressing these points will make it even stronger.
src/MudBlazor.Docs/Pages/Components/Charts/SankeyChartPage.razor
Outdated
Show resolved
Hide resolved
src/MudBlazor.Docs/Pages/Components/Charts/Examples/SankeyExample2.razor
Show resolved
Hide resolved
src/MudBlazor.Docs/Pages/Components/Charts/Examples/SankeyExample3.razor
Show resolved
Hide resolved
Anu6is
left a comment
Anu6is
left a comment
There was a problem hiding this comment.
Overall it looks pretty solid.
versile2
left a comment
versile2
left a comment
There was a problem hiding this comment.
Nice to see a new chart and it looks pretty good. I too want to see and understand the Chart Tooltip Changes, in particular how it affects other charts. Please include that in the opening of the PR. The only visual thing I noticed was adjusting the sizing did not move the labels so you could easily adjust sizing and have the labels just show up in odd spots. Is it possible to adjust the labels when these are resized?
|
@versile2 Update the PR-description (hope that was what you ment). Also made the labels adjust their position while changing vertical spacing (this also worked previously but only if the vertical spacing was set at the initial chart render). |
|
Love it! |
danielchalmers
changed the title
|
Thank you! |
6 participants
Often used in finances or life sciences, a sankey chart visualizes data flow between multiple states, allowing flows to di- and converge:
Overlapping edges:
Due to the nature of the sankey chart it may be required to display many chart tool tips at once. Therefore I had to implement a way to adjust the font size (which was hard coded).
The dynamic changing of the font size then resulted in alignment issues (as the pixel values of the background where also hardcoded) which required reworking the ChartTooltip component.
There are no visual differences for the existing charts as far as i can tell besides a smaller min-width (it was hardcoded before to hide the overlapping triangle at the bottom):
This is my very first PR so feedback would be greatly appreciated.