Reduce domain padding in line charts to minimum necessary for fitting labels#93405
Conversation
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.
|
|
🚧 @shawnborton has triggered a test Expensify/App build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
|
Love it! Crazy how removing that little bit of padding makes these somehow feel more legit? 😂 |
|
Big agree! |
|
Looking good to me as well :) |
|
awesome! I will clean up the code and make this ready for review soon |
mateuuszzzzz
left a comment
There was a problem hiding this comment.
Looks great, both code-wise and visually! Also, awesome that we dropped this outdated minimum space logic
borys3kk
left a comment
There was a problem hiding this comment.
code looks great, love the visual aspect as well, LGTM 🚀
|
@ZhenjaHorbach 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] |
Reviewer Checklist
Screenshots/VideosAndroid: HybridApp2026-06-15.12.10.32.movAndroid: mWeb Chrome2026-06-15.12.10.32.moviOS: HybridApp2026-06-15.12.03.51.moviOS: mWeb Safari2026-06-15.11.59.13.movMacOS: Chrome / Safari2026-06-15.11.54.31.mov |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5e35b30753
ℹ️ 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".
| return {...BASE_DOMAIN_PADDING, left: horizontalPadding, right: horizontalPadding}; | ||
| return { | ||
| ...BASE_DOMAIN_PADDING, | ||
| left: Math.max(0, measurements.firstLabelWidth / 2 - chartPaddingLeft), |
There was a problem hiding this comment.
Clamp left padding when labels rotate
When a line chart has dense or long labels (for example, expense results grouped by merchant/category with view:line), labelsExceedTickSpacing makes the right side reserve only a rotated-label overhang, but the left side still reserves half of the full first label width. A long first label can therefore add hundreds of pixels of empty space before the first point and squeeze the plot even though the label will be rotated/truncated by useChartLabelLayout; compute the left padding from the same rotated/truncated constraints used for the right side instead of the raw horizontal width.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
true, but we have limited information at the point of calculating and in this case we do indeed reserve too much space, but it's still the same or smaller than currently
There was a problem hiding this comment.
actually, in some cases it could reserve more space than before or not eniugh. I made it so that when labelsExceedTickSpacing is true (we likely rotate) we reserve the space necessary for 45 degrees rotated first label, not always half the width of the first label like before. (cc: @ZhenjaHorbach)
the rotated label does overhang to the right a little bit, even though it's right aligned. I did think about setting the padding to 0 in this case and letting this little overhang extend out of the plot area, but I thought that it would cause clipping. though I tried it briefly, and surprisingly it was not getting clipped, but I'm not sure why and it may be platform/library implementation dependent, so I decided to keep this little extra space, to be safe |
|
and currently the padding reserved on the right is the same for 45° and 90°, cause when we're deciding the padding we don't actually have the rotation information yet, because the rotation is determined using data that includes the padding, so there's a cycle. we could try to predict the rotation more thoroughly, but I think it would complicate the implementation and the different would be a couple pixels max and not always accurate anyway, so I'm not sure it's worth it |
It makes sense! |
|
Minor comment after the last changes |
do you maybe have a video of that happening? it's not reproducing for me, I think |
I wanted to share |
|
Although I managed to record a video 2026-06-15.13.48.51.mov |
|
hmm, I think it might be because of |
Definitely not critical😁 |
|
I think the diagonal labels above look pretty good IMO. |
|
Let's merge then! |
|
🚧 @puneetlath has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/puneetlath in version: 9.4.8-0 🚀
Bundle Size Analysis (Sentry): |
|
🤖 No help site changes required. I reviewed the changes in this PR against the help site files under This PR is a purely internal visual refinement to line chart rendering — it reduces the domain padding (the blank space between the Y axis / chart edges and the first/last data points) in the Spend and Home line charts, while ensuring axis labels aren't clipped or prematurely rotated. The only file touched is There are no changes to features, settings, tabs, button labels, workflows, or any user-facing functionality that the help site documents. Help articles describe what features do and how to use them — they don't document pixel-level chart spacing — so there is nothing to update and no draft PR is needed. If you believe a specific article should be updated, let me know which one and I'll take another look. |
|
🚀 Deployed to staging by https://github.com/puneetlath in version: 9.4.8-0 🚀
Bundle Size Analysis (Sentry): |
|
🤖 Help site review: no docs changes required. This PR is a purely visual rendering adjustment — it reduces the domain padding (whitespace) between the Y-axis/chart edges and the first/last points in Spend/Home line charts, and ensures labels aren't clipped or rotated prematurely. The only file touched is No user-facing feature, workflow, setting, label, or button changes here — just chart spacing polish. Help articles document what charts show and how features work, not pixel-level layout, so there's nothing to update under @mhawryluk, if you believe a help article does need updating as a result of this change, let me know which one and I'll draft it. |
|
🚀 Deployed to production by https://github.com/puneetlath in version: 9.4.8-3 🚀
|



Explanation of Change
Reduces the spacing in Spend/Home line charts between the Y axis and the first point, as well as between the chart edge and the last point to a reasonable minimum, while ensuring the labels don't get clipped or rotated prematurely because of it. It drops the idea of reserving minimum space for points so that they don't get clipped at the plot area boundaries, cause that's no longer an issue, as we've been rendering them using renderOutside prop for some time now. Also the padding symmetry is no longer enforced, the first point can make use of the space reserved for the Y axis labels more effectively this way.
Fixed Issues
$ #90646
PROPOSAL: N/A
Tests
Offline tests
N/A
QA Steps
Same as tests.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, 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.ScrollViewcomponent to make it scrollable when more elements are added to the page.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