Repository navigation
Standardize Expense Report and Expense header with predictable button placement and carousel counter - #98611
Conversation
… placement and carousel counter
|
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 |
|
@ahmedGaber93 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] |
|
@codex review |
|
@ShridharGoel Will be reviewer of this PR |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
Codex Review: Didn't find any major issues. 🚀 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". |
JmillsExpensify
left a comment
There was a problem hiding this comment.
This PR is still good for me. We're mostly working through polish and deploy blockers.
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2026-07-14.at.11.14.54.PM.movAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / SafariScreen.Recording.2026-08-14.at.3.06.13.PM.mov |
|
@JmillsExpensify Do we need to show the carousel for grouped expenses as well? For example, when grouped by categories, and an expense is opened, then it doesn't show the carousel currently. |
|
Ah, that's a good question. I think we should, though I'm also fine tackling this separately given that getting this PR to production has been quite involved. |
|
@JS00001 Somehow it doesn't auto assign to you. Kindly help do final review. Thanks |
| const parentActions: Record<string, OnyxTypes.ReportAction> = {}; | ||
| if (snapshotData) { | ||
| for (const [key, reportActionsForReport] of Object.entries(snapshotData)) { | ||
| if (key.startsWith(ONYXKEYS.COLLECTION.REPORT_ACTIONS)) { |
There was a problem hiding this comment.
Can you please explain why we need to subscribe to the snapshot separately? We automatically use either onyx data, or snapshot data, based on the page the user is on. So do we need to subscribe to the snapshot and fetch all of the report actions? Or can we just subscribe to report actions (which will use the snapshot when needed)
There was a problem hiding this comment.
When the carousel is opened from a search (e.g. the Spend page), the sibling transactions may only exist in the search snapshot and not in the live collection yet. We use the snapshot
And also fixed this issue #90403 (comment)
|
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 bun ./scripts/generateTranslations.ts --helpTypically, you'd want to translate only what you changed by running |
|
Deploy Blocker #99612 was identified to be related to this PR. |
|
Deploy Blocker #99614 was identified to be related to this PR. |
|
Deploy Blocker #99616 was identified to be related to this PR. |
|
Deploy Blocker #99617 was identified to be related to this PR. |
|
Deploy Blocker #99619 was identified to be related to this PR. |
|
Deploy Blocker #99621 was identified to be related to this PR. |
|
Deploy Blocker #99627 was identified to be related to this PR. |
|
Deploy Blocker #99636 was identified to be related to this PR. |
|
Deploy Blocker #99641 was identified to be related to this PR. |
[cp staging] Revert "Merge pull request #98611 from thelullabyy/fix-03/90366"
|
🚀 Deployed to production by https://github.com/AndrewGable in version: 9.4.62-4 🚀
Bundle Size Analysis (Sentry): |
|
🚀 Deployed to staging by https://github.com/JS00001 in version: 9.4.63-0 🚀
|
|
🤖 Help site review: no changes required. I reviewed the changes in this PR against the help articles under Why: This PR is a UI layout standardization — it moves the report action buttons down to the report next-steps line, always places the carousel navigation in the top right, always shows the carousel for expenses opened from the Spend page, and standardizes the carousel counter to a The help articles describe what buttons do and how to accomplish tasks, not the exact vertical placement of action buttons or the presence/format of a navigation carousel counter — so nothing in them becomes inaccurate:
No draft PR was created since there is nothing to update. @thelullabyy, if you believe any help article should document the standardized header layout or carousel counter, let me know which article and behavior and I'll draft the update. |
|
🚀 Deployed to production by https://github.com/AndrewGable in version: 9.4.63-1 🚀
Bundle Size Analysis (Sentry): |
Explanation of Change
Fixed Issues
$ #90366
PROPOSAL: N/A
Tests
Log in with a test account that has access to a paid workspace with multiple expense reports and expenses
Have these in your test workspace:
Offline tests
QA Steps
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
Screen.Recording.2026-05-17.at.23.29.29.mov
Bug: #96563
Android: mWeb Chrome
Screen.Recording.2026-05-17.at.23.25.25.mov
iOS: Native
iOS: mWeb Safari
Screen.Recording.2026-05-17.at.23.21.16.mov
MacOS: Chrome / Safari
Screen.Recording.2026-05-17.at.23.15.10.mov
Bug: #96545
Screen.Recording.2026-07-27.at.20.19.00.mov
Bug: #96548
Bug: #96566
Screen.Recording.2026-07-27.at.21.06.31.mov
Bug: #96575