Fix overlapping category labels in VirtualCFO pie charts#95835
Open
luacmartins wants to merge 18 commits into
Open
Fix overlapping category labels in VirtualCFO pie charts#95835luacmartins wants to merge 18 commits into
luacmartins wants to merge 18 commits into
Conversation
Small adjacent slices in the "Top categories" donut (VirtualCFO/AnalyzeExpense) crowded and overlapped their labels because each label was placed independently along its own slice's angle, only alternating radius by index parity. Labels now split into two fixed columns (left/right of the ring) and stack top-to-bottom with collision-aware spacing, with leader lines bent once to the resolved position. Also fixes a pre-existing bug found while verifying the new layout: polar chart containers clip their bottom ~10% (POLAR_CONTAINER_HEIGHT_RATIO), which could silently cut off labels near 6 o'clock in the live app even though the headless golden-PNG renderer's layout path never exercises that clipping.
melvin-bot
Bot
requested review from
arosiclair and
flaviadefaria
and removed request for
a team
|
@arosiclair Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
Contributor
Author
|
Not ready for review yet |
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
The leader line's second segment went straight from a slice's natural elbow point to its (possibly collision-pushed) final position. Once a label was pushed any real distance from its natural angle, that diagonal could double back vertically relative to the first (radial) segment, producing a visible zigzag. Replaced with a horizontal run (at the slice's natural height) followed by a vertical bend into the resolved row, so the line only ever moves toward its target and never reverses direction. This also let a chunk of now-dead labelRadius/labelIndicatorOuterOffset plumbing be removed from the pie label components.
The previous elbow ran horizontal near the ring and vertical into the label, which left the line meeting the text at an arbitrary height. Swap the order: a short diagonal near the ring absorbs both the x and y offset toward the resolved row, then a flat run carries the line the rest of the way into the label at that row's own height. labelIndicatorOuterOffset (previously removed as dead after the last change) is reinstated with a new meaning: it now sizes the diagonal segment's horizontal reach near the ring, instead of the old radial elbow-point offset.
A 100%-value slice's start and end angle are the same point, so the generic (start + end) / 2 "midpoint" used for every other slice lands diametrically opposite the ring's seam — the bottom, given this codebase's 12-o'clock start angle — instead of a sensible spot. Anchor a lone slice's label to a fixed 3-o'clock position instead. The indicator line previously recomputed its own midAngle straight from slice.startAngle/endAngle, disagreeing with the label's (now overridden) angle and producing a line that still ran to the bottom before jumping to the relocated label. Thread the resolved midAngle through instead, so both the label and its line always agree.
ResolvedPieLabel was declared identically in both VictoryChartPieLabel.tsx and VictoryChartPieLabelIndicator.tsx; moved it into the shared types.ts module (alongside TextAnchor, which it already depends on) so both import one definition instead of keeping two in sync by hand. Also adds the missing header comment to computePieLabelLayout.ts describing its purpose.
Neither type is imported anywhere outside this file — they're only used internally by computePieLabelLayout itself, so exporting them was dead surface area.
The title/subtitle are left-aligned at the same x the left column's labels end at, so a label stacked right at the shared top clearance read as cramped against them even without literally overlapping — the right column doesn't share that horizontal space and didn't have the problem. plotBounds is now computed per side instead of shared, with the left column's top pushed down by an extra fixed buffer.
The diagonal segment's horizontal reach was a small fixed offset regardless of how far a label had been pushed vertically. Giving the left column extra top clearance increased how far labels near 12 o'clock get pushed down, so that fixed-width diagonal turned into a near-vertical stroke hugging the ring's x position for a long vertical run — reading as a line cutting through the chart instead of a side connector. The diagonal's horizontal reach now grows with the vertical distance it covers (never steeper than 45°), falling back to the original fixed offset when the push is small enough not to need it.
The elbow-radius approach from the last commit fixed the ring-overlap for one case but introduced two new problems: extending along a slice's own angle to a fixed radius overshoots well past the resolved row for slices near the top/bottom seam (producing a stray peak above the ring), and can still land inside the ring when there isn't enough room left before the column for slices whose natural angle is already close to their column. Replaced with a direct solve: since the final segment must be purely horizontal at the resolved row's height (so the line meets the label text at that height), the bend point's Y is fixed to resolvedLabel.y already — so compute the X offset needed at that exact Y to clear the ring, clamped between the ring touchpoint and the label itself. No separate elbow point, no overshoot, and no possibility of landing inside the ring's radius.
The previous fix only guaranteed the bend point itself sat at a safe radius from center. A straight line between two points that are each individually outside the ring can still dip inside it if they sit at sufficiently different angles — the chord bows toward center. This showed up as a leader line cutting across a neighboring slice whenever several tiny slices sat close together with very different resolved rows (the ring touchpoint stays near the slice's own angle, but the bend point can land at a very different effective angle once collision resolution has moved the row). Now finds the closest point on the ring-to-bend segment to the center and, if it would dip inside the safe radius, projects that point outward onto the safe circle instead of connecting the two points directly.
…bend The chord-dip check compared against safeRadius (slice.radius + outer offset), but the ring attachment point itself always sits at the smaller innerRadius by construction. Since innerRadius < safeRadius always, the "closest point" on almost every line was the attachment point itself, which the check then flagged as unsafe and pushed outward — adding an unnecessary extra bend to lines that were never actually at risk of crossing the ring (visible as a double-elbow zigzag on well-separated labels like large slices sharing a column with a small cluster). Check against innerRadius instead — the radius the attachment point is actually guaranteed to sit at — so the correction only fires when a line would truly dip inside the ring.
Contributor
|
Can you please share test chart data? |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 08bb0b5df7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Contributor
Author
|
@situchan this is one of the charts from the screenshot |
Contributor
|
Screenshots are looking good to me 👍 |
Contributor
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
Contributor
I am not seeing amount with this data. 
There's no padding at the edge |
The bottom and left/right edge-padding fix changes rendered pixel output for these fixtures, so their golden regression snapshots need regenerating to match.
Contributor
Author
Oh weird since I think that's the data that I used to generate the chart in the OP. It seems like you figured it out though.
@situchan should be fixed now |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Explanation of Change
"Top categories" pie chart labels overlapped when several small slices sat next to each other, since each label was placed by its own slice angle with only an even/odd radius alternation.
Replaced with a two-column layout: labels split left/right by ring side, stack top-to-bottom with collision-aware spacing, and connect via a single-bend leader line.
Fixed Issues
$ #94613
Tests
Top categorieschart labels look correct.server/victory-chart-renderer:bun test tests/render.test.ts— all golden PNGs pass, including newtop-categories-crowded-slicesfixture.top-categories-*.xmlfixture (standalone renderer or in-app) and confirm: two clean label columns, no overlaps, single-bend leader lines, no regression on well-separated charts.Offline tests
N/A
QA Steps
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari