Shrink VirtualCFO horizontal chart's left-axis padding to fit label width#95995
Conversation
…idth Employee-name labels on the left axis of Concierge-generated horizontal bar charts reserve a fixed 150px regardless of content, wasting bar space for short names. The configured padding.left now acts as a max that shrinks to the actual measured label width when smaller.
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.
|
…_EDGE_BUFFER to LEFT_AXIS_LABEL_PADDING
The XML's tickLabels.padding can set an arbitrarily large gap between the label and the chart; clamp it so labels never sit further than 16px from the bars they label.
melvin-bot
Bot
requested review from
lakchote and
trjExpensify
and removed request for
a team
![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: 386bbed577
ℹ️ 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".
When tickValues is longer than tickFormat, the unformatted ticks render via String(label) fallback but were excluded from the width measurement, letting padding.left shrink below what a wide fallback label needs.
| domain: CartesianChartProps['domain']; | ||
| domainPadding: CartesianChartProps['domainPadding']; | ||
| padding: CartesianChartProps['padding']; | ||
| /** Pixel width the left y-axis actually needs for its widest label, used to shrink `padding.left` when it's larger than the content requires. */ |
There was a problem hiding this comment.
NAB: I don't think a block style comment is needed here
| /** Pixel width the left y-axis actually needs for its widest label, used to shrink `padding.left` when it's larger than the content requires. */ | |
| // Pixel width the left y-axis actually needs for its widest label, used to shrink `padding.left` when it's larger than the content requires. |
|
Screenshot above seems good to me 👍 |
|
Though I bet you could still reduce this gap between the label and the chart just slightly, by like 4-8px: |
|
@shawnborton reduced the gap |
|
Bun tests failing |
|
@situchan fixed |
|
Thanks! |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a2f0c471ce
ℹ️ 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".
| if (childResult.leftAxisLabelPadding !== undefined) { | ||
| leftAxisLabelPadding = childResult.leftAxisLabelPadding; | ||
| } |
There was a problem hiding this comment.
When a horizontal chart supplies multiple left-side y-axis configurations, this assignment overwrites the earlier measurement with the last axis encountered. yAxis is intentionally accumulated as an array in the same traversal, so a later axis with shorter labels can make resolvePadding() shrink padding.left below the width required by an earlier axis, clipping that axis's labels. Retain the maximum measured width instead of the last value.
Useful? React with 👍 / 👎.
ReviewOverall this looks clean and well-scoped — the measure-and-shrink approach is a solid replacement for the hardcoded Suggestions1. Add a unit test for 2. Constant naming collision (readability). 3. Numeric Things I checked that look correctDetails
Not a substitute for a human review — please still get eyes from a reviewer before merging. |
Regression scanNo confirmed regressions. The change is correctly scoped to horizontal bar charts with a left y-axis, and the unchanged golden images prove nothing else moved. Two edge cases are worth a look before merge (both low-likelihood). What I checked (and why it's safe)
Edge cases to sanity-check1. Offset clamp is unconditional, but the padding shrink is not — they can disagree. 2. No golden coverage for other horizontal charts. Only Neither is a blocker — flagging for a look, not a fix demand. This is an automated scan, not a substitute for human review. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid.movAndroid: mWeb ChromeiOS: HybridAppios.moviOS: mWeb SafariMacOS: Chrome / Safari
|
|
It would be good to have some space at the left edge. Not blocker as happening on production. 
production: |
|
Totally agree |
|
@situchan I don't get that issue anymore (don't mind the messed up bars, I'm fixing that in a separate PR) |
|
🚧 luacmartins has triggered a test Expensify/App build. You can view the workflow run here. |
|
@luacmartins please test with this data. Still reproducible on main (after this PR merged) |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
@situchan this is the chart I get from the BE using the same long name. I think there might be something wrong with the data above. This is the data I get: 
|
Because backend is sending already ellipsed name |
|
Ah yea, the BE truncates the names to 16 characters |
7 participants
Explanation of Change
Horizontal bar charts (e.g. "Top employees by spend") hardcode
padding.leftto150px for the category-axis labels.This PR measures the widest rendered label via Skia glyph widths and adjusts the label area accoridngly.
Fixed Issues
$ #95434
Tests
Pre-condition: an admin account with expenses on the previous month
VirtualCFO report
https://www.expensify.com.dev/_support/agentZeroSandboxHeadless rendering
cd server/victory-chart-renderer && bun test tests/render.test.tsand confirm all golden-PNG fixtures pass, including the two updated (top-employees-by-spend,top-employees-by-spend-truncated-labels) and the unrelated vertical-chart fixture (monthly-spend), which is byte-identical to its unchanged golden.bun run scripts/build.ts --target --outfile /tmp/vcr) and rendertests/fixtures/top-employees-by-spend.xmlwith short 2-3 character placeholder names.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) — verified the vertical-chart (dependent-axis) path is unaffected via its unchanged golden render.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.server/victory-chart-renderer/tests/__golden__/.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
N/A
Android: mWeb Chrome
N/A
iOS: Native
N/A
iOS: mWeb Safari
N/A
MacOS: Chrome / Safari
N/A