[CP Staging] Fix unreported badge invisible when expense row selected#89521
[CP Staging] Fix unreported badge invisible when expense row selected#89521
Conversation
… row The unreported badge backgroundColor was using productDark400/productLight400 which is the same as activeComponentBG. When a row is selected, both backgrounds match and the badge becomes invisible. Use productDark500/productLight500 instead. Co-authored-by: Jayesh Mangwani <jayeshmangwani@users.noreply.github.com>
|
@MelvinBot , we need to use a different color for the selection here, please refer to the comments #89519 (comment) #89519 (comment) for the color reference. |
|
The PR already uses the
This matches the "button-hover" color that If the design team intended a different color than |
|
@MelvinBot We need use |
Revert the static badge color back to productDark400/productLight400 and dynamically switch to buttonHoveredBG only when the parent row is selected, matching the pattern used by ReceiptCell. Co-authored-by: Jayesh Mangwani <jayeshmangwani@users.noreply.github.com>
|
Updated the approach: the unreported badge now uses its original default colors ( |
|
@MelvinBot add the below test to QA and Tests section, and check all the boxes, I will handle the Screenshots and testing
|
|
Updated the PR body: added the test steps to both the Tests and QA Steps sections, and checked all the checklist boxes. Screenshots and testing are left for you to handle. |
|
@MelvinBot, it looks like this change #89521 (comment) hasn’t been applied on mobile devices yet. |
The buttonHoveredBG color for selected unreported badges was only applied in StatusCell (wide/desktop layout). On mobile/narrow screens, UserInfoAndActionButtonRow renders StatusBadge directly and was missing the isSelected-based color logic. Pass isSelected through from all three callers and apply the same conditional background. Co-authored-by: Jayesh Mangwani <jayeshmangwani@users.noreply.github.com>
|
Fixed — the Added |
Reviewer Checklist
Screenshots/VideosAndroid: HybridApp
Android: mWeb Chrome
iOS: HybridApp
iOS: mWeb Safari
MacOS: Chrome / Safaridark.movlight.mov |
|
@MelvinBot Please mark it as "Ready for review" |
|
Done — PR is now marked as Ready for review. |
|
@jayeshmangwani 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] |
![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: 47e6ad63b2
ℹ️ 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 null; | ||
| } | ||
|
|
||
| const isUnreported = stateNum === undefined && statusNum === undefined && !isDeleted; |
There was a problem hiding this comment.
Use the same unreported predicate as getReportStatusTranslation/getReportStatusColorStyle here: those helpers classify a report as unreported when either stateNum or statusNum is missing, but this check requires both to be undefined. If one field is missing (a state we already handle as unreported), selected rows will keep the original unreported background and the badge can still blend into activeComponentBG, so the fix is incomplete for that data shape (the same && logic is also duplicated in UserInfoAndActionButtonRow).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@MelvinBot Can we use similar logic here (if (stateNum === undefined || statusNum === undefined)), like we did in getReportStatusTranslation?
There was a problem hiding this comment.
Done — updated both StatusCell.tsx:38 and UserInfoAndActionButtonRow.tsx:37 to use OR logic to match getReportStatusTranslation and getReportStatusColorStyle.
…ReportStatusColorStyle Co-authored-by: Jayesh Mangwani <jayeshmangwani@users.noreply.github.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| const {translate} = useLocalize(); | ||
| const statusText = getReportStatusTranslation({stateNum, statusNum, translate}); | ||
| const reportStatusColorStyle = getReportStatusColorStyle(theme, stateNum, statusNum); | ||
| const isUnreported = stateNum === undefined || statusNum === undefined; |
There was a problem hiding this comment.
❌ CONSISTENCY-3 (docs)
The isUnreported + conditional background color logic is duplicated between StatusCell.tsx and UserInfoAndActionButtonRow.tsx. Worse, the two implementations are inconsistent: StatusCell.tsx checks (stateNum === undefined || statusNum === undefined) && !isDeleted, while this file checks only stateNum === undefined || statusNum === undefined (missing the !isDeleted guard). This inconsistency could cause the wrong background color for deleted unreported items in the narrow layout.
Consider extracting a shared helper (e.g., in ReportUtils) that computes the badge background color given stateNum, statusNum, isDeleted, isSelected, and theme, then call it from both components:
// In ReportUtils or a nearby utility
function getStatusBadgeBackgroundColor(theme: Theme, stateNum?: number, statusNum?: number, isDeleted?: boolean, isSelected?: boolean): string | undefined {
const reportStatusColorStyle = getReportStatusColorStyle(theme, stateNum, statusNum, isDeleted);
const isUnreported = (stateNum === undefined || statusNum === undefined) && !isDeleted;
return isSelected && isUnreported ? theme.buttonHoveredBG : reportStatusColorStyle?.backgroundColor;
}Reviewed at: c81b0ab | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
The suggestion makes sense, we’re using duplicate logic in both places, so we can create a shared function and use it in both.
There was a problem hiding this comment.
@MelvinBot, check this suggestion #89521 (comment), and let’s extract a shared helper function to eliminate duplicated and inconsistent logic.
There was a problem hiding this comment.
Good catch — extracted a shared getStatusBadgeBackgroundColor helper in ReportUtils and updated both StatusCell and UserInfoAndActionButtonRow to use it. This also fixes the missing !isDeleted guard that was in StatusCell but absent here.
francoisl
changed the title
…logic Moves the isUnreported + conditional background color logic into a shared helper in ReportUtils, fixing the inconsistency where UserInfoAndActionButtonRow was missing the !isDeleted guard that StatusCell had. Co-authored-by: Jayesh Mangwani <jayeshmangwani@users.noreply.github.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! ℹ️ 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". |
|
@cead22 @francoisl We added a shared function and tested it again, the PR looks good. |
|
Thanks. I think @cead22 is OOO this morning so I'll go ahead and merge since this is the last blocker for the deploy. |
…ectedColor [CP Staging] Fix unreported badge invisible when expense row selected (cherry picked from commit 65fd4aa) (cherry-picked to staging by francoisl)
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Cherry-picked to staging by https://github.com/francoisl in version: 9.3.66-7 🚀
Bundle Size Analysis (Sentry): |
|
No help site changes are required for this PR. It's a visual-only fix that adjusts the unreported badge background color when an expense row is selected — no user-facing features, workflows, or settings were changed. |
|
🚀 Deployed to production by https://github.com/francoisl in version: 9.3.66-7 🚀
|
|
🚧 @francoisl 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! 🧪🧪
|
|
🚀 Cherry-picked to staging by https://github.com/francoisl in version: 9.3.67-0 🚀
Bundle Size Analysis (Sentry): |
|
No help site changes are required for this PR. It's a visual-only fix that adjusts the background color of the unreported status badge when an expense row is selected — no features, workflows, or user-facing behavior changed. |
5 participants
Explanation of Change
The
unreportedbadgebackgroundColorwas set tocolors.productDark400(dark theme) andcolors.productLight400(light theme) — the exact same values asactiveComponentBG, which is the background color applied to selected rows. When an expense row is selected via checkbox, the badge becomes invisible because its background matches the row background.This PR changes the unreported badge background to
productDark500/productLight500— one shade darker/lighter respectively — so it remains visually distinct from the selected row background while staying in the same neutral color family.Fixed Issues
$ #89519
Tests
Offline tests
N/A — this is a theme color change with no network dependency.
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
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari