92969: Add "expand" button overlay to charts for full-screen mode - #95249
Conversation
|
Hey, I noticed you changed If you want to automatically generate translations for other locales, an Expensify employee will have to:
Alternatively, if you are an external contributor, you can run the translation script locally with your own OpenAI API key. To learn more, try running: npx ts-node ./scripts/generateTranslations.ts --helpTypically, you'd want to translate only what you changed by running |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bb9bb9f384
ℹ️ 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".
|
cc @Expensify/design this button doesn't look right to me at all. Can we use one of our standard icon buttons here? (fully circle, smaller icon, etc.) |
|
Totally agree. Please use our standard icon buttons, use a small 28x28 icon and it should probably be like 12px from the top and 12px from the right? |
|
@situchan I fixed above AI feedbacks. |
|
Much better. @shawnborton do you think we should only show that icon on hover? Basically exactly what we do for receipts? CleanShot.2026-07-06.at.11.28.54.mp4 |
|
Yup, that makes sense to me! Or we always show a three dots icon in the top right but I don't think that will look good based on how these things scale on mobile devices. Also, does clicking anywhere on the chart also expand it? Or just this button does? |
|
@abbasifaizan70 the flicker is not fixed yet. Clearly noticeable in dark mode
Screen.Recording.2026-07-20.at.6.47.54.PM.mov |
|
@situchan let me retry to check again. |
… removing Skia canvas during close animation
Screen.Recording.2026-07-21.at.2.23.28.AM.mov@situchan Fixed flicker issues. Can you check if you can see it now as well? Thanks |
|
In theory that's exactly how image attachments work. For the resolution, are we not showing the svg version at the large size? |
|
@luacmartins Yes, the vertical fill is intentional — the modal scales the chart uniformly to fit the available area (preserving aspect ratio), the same way image attachments do, so height usually becomes the binding dimension. @shawnborton There's no SVG version to swap in — charts are drawn by Skia onto a raster canvas at their design size, and the expanded view upscales that raster uniformly. That upscaling is what keeps the HTML-provided label/legend coordinates (which are in design space) perfectly aligned, but it's also why very large screens look soft. Rendering the canvas natively at the target size would fix sharpness but requires re-mapping the label coordinates — the same machinery pinch-zoom needs. I'd suggest handling sharp large-size rendering together with the zoom follow-up PR . |
ReviewSolid, well-documented feature — the state-lives-below-the-renderer design (so hover doesn't re-render the chart subtree) and the "render at design size, then uniform transform-scale" technique are the right calls, and they keep the expanded chart aligned with the inline one. CI is fully green. A few things worth addressing before merge: 1. Duplicated scaling/clipping logic (main concern)
The only thing the modal needs that the container doesn't do is scale up ( 2. No tests for the new componentsThe checklist marks unit tests as added and Codecov flagged a coverage drop, but the only test change is a mock update in 3. Modal title is "Details"The header uses 4. Always-visible button occludes the chart on touch / no-hover devicesBecause Minor
None of these are correctness blockers (the approach and edge-case handling for polar clipping, close-animation flashes, and touch-vs-hover are all thoughtfully covered). (1) and (2) are the ones I'd want resolved. |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
Regression scanMost of the diff is additive (new components, a new 1. Every inline chart now gets a new wrapping
|
|
@luacmartins Can you please take a look? Thanks |
|
@abbasifaizan70 please merge main |
@luacmartins is OOO until Aug 5. Maybe @JS00001 can help? |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 JS00001 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/JS00001 in version: 9.4.46-0 🚀
|
|
🤖 Help site review: no docs changes required. This PR adds an expand button (and a full-screen modal) to the inline charts Concierge renders in chat — a self-contained UI affordance. It introduces no new workflow, setting, or user-facing capability that a help article would describe. I reviewed the relevant help content against these changes:
None of these articles document Concierge's in-chat chart output, and help articles describe features and workflows rather than individual buttons. An expand-to-full-screen control doesn't meet the bar for its own documentation, so I did not create a draft help site PR. If the team wants the broader "Concierge can render charts/graphs in chat" capability documented (that capability predates this PR), that would be a separate docs effort — let me know and I can scope it. |
|
🚀 Deployed to production by https://github.com/marcaaron in version: 9.4.46-10 🚀
Bundle Size Analysis (Sentry): |
luacmartins
left a comment
There was a problem hiding this comment.
Thanks for getting this through the finish line!


Explanation of Change
Adds an expand button overlaid on the top-right corner of inline charts, which opens the chart in a centered full-screen modal. The modal reuses the already-parsed chart data and renders it scaled to the viewport, so the expanded chart matches the inline one exactly, just larger.
Fixed Issues
$ #92969
PROPOSAL: #92969 (comment)
Tests
Offline tests
same as tests.
QA Steps
same as tests.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)Avatar, 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
Screen.Recording.2026-07-07.at.2.33.29.AM.mov
Android: mWeb Chrome
iOS: Native
Screen.Recording.2026-07-07.at.2.36.43.AM.mov
iOS: mWeb Safari
Screen.Recording.2026-07-07.at.2.37.58.AM.mov
MacOS: Chrome / Safari
Screen.Recording.2026-07-07.at.2.29.33.AM.mov