feat(sankey): Add inline label layout and fix auto-assigned node colo…#526
Conversation
a2df2f7 to
b8916ac
Compare
commit: |
Docs PreviewCommit: |
|
/bonk what do you think about this PR? |
There was a problem hiding this comment.
This is a solid PR! The implementation is clean and follows existing patterns.
What I like:
- The
nodeLabelLayoutprop is well-designed with clear semantics (stacked|inline) - The color fix correctly addresses the tooltip color mismatch by computing
nodeColorsupfront and storingcomputedColorin the data map - Good test coverage for both the new feature and the bug fix
- Proper changeset including both packages
- The demo provides a practical example showing when inline layout is useful
One minor observation:
The new describe("nodeLabelLayout") and describe("auto-assigned node colors") blocks are nested inside describe("security utilities"). While this doesn't affect functionality, these tests aren't security-related so they might be better as top-level describes. This is purely organizational and non-blocking.
Overall, this is ready to ship! 🚀
| }); | ||
| }); | ||
|
|
||
| describe("nodeLabelLayout", () => { |
There was a problem hiding this comment.
Nit: These new test blocks (nodeLabelLayout and auto-assigned node colors) are nested inside describe("security utilities") but they're not security-related. Consider moving them to top-level describes for better organization. Non-blocking.
| // Build a map of node name to original node data for tooltip access | ||
| const nodeDataMap = new Map(nodes.map((n) => [n.name, n])); | ||
|
|
||
| // Compute colors for each node (explicit color > default > categorical) |
There was a problem hiding this comment.
Good approach computing colors upfront and storing them in nodeDataMap with computedColor. This ensures consistent color usage across both nodes and tooltips.
| const formattedValue = escapeRichText( | ||
| formatValue(nodeData.value), | ||
| ); | ||
| // Inline: "name value" on single line; Stacked: value above name |
There was a problem hiding this comment.
Nice touch reversing the order for inline layout (name value vs value\nname). Reads more naturally on a single line.
Review Summary for PR #526This PR adds a useful new What's Good
Minor NitpickThe new test blocks for Verdict: LGTM! 🚀 This is ready to merge. |































































Changes
1. New
nodeLabelLayoutpropnodeLabelLayoutprop with two options:"stacked"(default): Value on top, name below (two lines)"inline": "Name Value" on a single line2. Bug fix: Auto-assigned node colors now appear in tooltips
colorproperties, the auto-assigned colors from ChartPalette.categorical() were not reflected in link tooltips (fell back to gray#666)nodeColorsarray upfront and storescomputedColorin the node data mapFiles Changed
nodeLabelLayoutprop, fixed color computationUsage