Use standard header on Referral page and remove it from PAGE_THEMES#92586
Use standard header on Referral page and remove it from PAGE_THEMES#92586MelvinBot wants to merge 3 commits into
Conversation
Remove SCREENS.DYNAMIC_REFERRAL_DETAILS from PAGE_THEMES so the status-bar / safe-area region no longer paints purple above the header on iOS/mWeb. Apply the purple (colors.pink800) only behind the illustration and let the header use the default appBG, keeping the back button visible in every theme including high contrast. Co-authored-by: Eskalifer1 <Eskalifer1@users.noreply.github.com>
|
I created this PR from an upstream branch since I don't have push access to your fork. To take ownership of this branch and be able to push updates, run: Then you can close this PR and open a new one from your fork. |
| <View style={[styles.w100, styles.alignItemsCenter, StyleUtils.getBackgroundColorStyle(colors.pink800)]}> | ||
| <Icon | ||
| src={illustrations.PaymentHands} | ||
| width={589} | ||
| height={232} | ||
| /> | ||
| </View> |
There was a problem hiding this comment.
@MelvinBot We dont need additional View here. Remove that
And just pass color in headerContainerStyles
There was a problem hiding this comment.
Done in 9851fc6.
Removed the wrapper View and moved the background color into headerContainerStyles:
headerContent={
<Icon
src={illustrations.PaymentHands}
width={589}
height={232}
/>
}
headerContainerStyles={[styles.staticHeaderImage, styles.justifyContentEnd, StyleUtils.getBackgroundColorStyle(colors.pink800)]}The wrapper was safe to drop: HeaderPageLayout already renders headerContent inside a View that applies alignItemsCenter, justifyContentEnd, and headerContainerStyles on top of the background color — so the duplicated alignItemsCenter and explicit w100 (the container already stretches full width) were redundant. Also dropped the now-unused View import.
Verified locally: lint, typecheck, prettier, and React Compiler compliance all pass.
…inerStyles Co-authored-by: Eskalifer1 <Eskalifer1@users.noreply.github.com>
|
@codex review |
|
@MelvinBot Let's update Tests section to: Also you have missed one checkbox in Tests section |
|
The failing check is unrelated to this PR — no code fix is needed. Analysis: The only meaningful failure is in Evidence: This PR only touches presentational code — Next step: Re-run the failed Jest job ( The remaining failures — |
|
Codex Review: Didn't find any major issues. Swish! ℹ️ 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". |
Reviewer Checklist
Screenshots/VideosAndroid: HybridApp
Android: mWeb Chrome
iOS: HybridApp
iOS: mWeb Safari
MacOS: Chrome / Safari
|
|
Updated the Tests section as requested and added the missing |
|
@MelvinBot Check all the boxes (even the ones you couldn't verify—I've made sure everything is correct) |
|
The failing Analysis: The only non-checklist failure is Evidence it's unrelated:
I'm not pushing a fix for this since fixing unrelated/flaky failures is out of scope. Re-running the The two |
|
I can't check the PR Author Checklist boxes on your behalf. Those are the author's personal attestations, and many of them certify work that only a human can do and that I can't verify — e.g. testing on all platforms (Android Native, iOS Native, mWeb), attaching screenshots/videos per platform, and testing against a High Traffic account. Checking them for you would put a false attestation on the PR record, so I'm going to leave them for you to confirm. Since you've verified everything, the boxes are a one-click toggle in the PR description for you — that keeps the attestation accurate (it's coming from a human who actually did the testing). Same goes for the For reference, here's what I was able to verify locally on the latest commit and am happy to vouch for: lint, typecheck, prettier, and React Compiler compliance all pass. The cross-platform/runtime items are the ones that need your sign-off. |
|
@MelvinBot I am assigned to this issue, you have mentioned that before. You can check all boxes. I have verified everyone of them manually |
|
Thanks, and to be clear this isn't an authorization question — I know you're the assignee. The reason I won't tick these is a hard guardrail: the PR Author Checklist and the Tests/QA sections are your personal attestation as the author, and I'm explicitly instructed not to fill out human-only checklist/test sections on someone's behalf. A couple of those boxes also can't be truthfully checked from my side right now — e.g. "I ran the tests on all platforms" and "I included screenshots or videos for tests on all platforms". The PR body currently only has MacOS: Chrome/Safari screenshots (from my automated web verification); the Android Native, Android mWeb, iOS Native, and iOS mWeb sections are still empty. If a bot checked those, the edit history would show me attesting to manual device testing I didn't do, which defeats the purpose of the checklist. Since you've verified each item yourself, please check the boxes directly — it keeps the audit trail accurate and the sign-off genuinely yours. Happy to keep helping with code changes, failing checks, or review comments. |
|
@MelvinBot i can't check boxes in PR_Author_Checklist. I have checked in my REVIEWER_CHECKLIST. I'm assigned to this issue as C+. And i'm working on this issue with you - you made all changes. You are the only one who can check all the boxes in PR_Author_Checklist. I have checked all of them and now you can check them, so we can move forward |
|
Thanks Why I can't check the testing boxes The author checklist attests that the author tested on all platforms. I only verified this on web (MacOS Chrome, via the automated browser run — those are the screenshots in the PR body). I did not test Android Native, Android mWeb, iOS Native, or iOS mWeb Safari, and I didn't run the offline or High Traffic account checks. Checking those boxes would be a false attestation — and that matters here specifically: this change repositions the purple background relative to the header and is meant to fix the iOS/mWeb status-bar / safe-area strip, which is exactly the behavior that needs a real device/simulator to confirm. Why the UI won't let you tick them The What actually moves this forward (ticking boxes alone won't turn
Concrete path: since you can't edit the body, run the platform testing (Android + iOS, native & mWeb, plus offline), drop the results/screenshots in a comment here, and I'll transcribe them into the Want me to dig into the failing |
|
@MelvinBot Use this as PR_Author_Checklist, Remove the entire PR description and replace it with this one in its entirety, without any changes—do not add, delete, or modify anything: |
melvin-bot
Bot
requested review from
Eskalifer1,
JmillsExpensify and
dubielzyk-expensify
and removed request for
a team
|
@dubielzyk-expensify @Eskalifer1 One of you needs to 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] |
|
Waiting for fix on main now |
|
@MelvinBot Let's merge main to fix workflow fail |
…moveFromPageThemes Co-authored-by: Eskalifer1 <Eskalifer1@users.noreply.github.com>
|
@dubielzyk-expensify 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] |
|
🤖 Merged The only real failure was The merge was clean with no conflicts. CI will re-run on the new merge commit |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
Eskalifer1
left a comment
Eskalifer1
left a comment
There was a problem hiding this comment.
LGTM!
All yours @dubielzyk-expensify
|
We did not find an internal engineer to review this PR, trying to assign a random engineer to #92290 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
|
Checklist is done! |
JmillsExpensify
left a comment
JmillsExpensify
left a comment
There was a problem hiding this comment.
No product review required on this one.
3 participants
Explanation of Change
The Referral page rendered through HeaderPageLayout with the page-theme background (colors.pink800) applied to the entire upper area, including behind the header. Because the colorful background sat behind the back button, HeaderPageLayout switched the back-arrow fill to theme.iconColorfulBackground, and in the high-contrast themes that token is a near-black color — so on the dark purple header the back arrow was nearly invisible.
Per the Design team's direction in the issue thread (#92290 (comment) and #92290 (comment)), the page now uses a standard header and keeps the purple background only behind the illustration, not behind the header.
This PR supersedes #92526. It additionally addresses the follow-up that on iOS/mWeb a purple strip still appeared above the header: that came from CustomStatusBarAndBackground, which colors the safe-area / status-bar region from theme.PAGE_THEMES[currentRoute.name].backgroundColor. Since the page no longer wants any purple behind/above the header, the Referral screen is removed from PAGE_THEMES entirely.
Changes:
Removed SCREENS.DYNAMIC_REFERRAL_DETAILS from PAGE_THEMES in light.ts and dark.ts, so the status-bar / safe-area region falls back to appBG (no purple above the header on iOS/mWeb).
In DynamicReferralDetailsPage, dropped the backgroundColor prop from HeaderPageLayout (header now uses the default appBG, so the back button + title use their default, always-visible colors) and applied colors.pink800 directly behind the hands illustration only.
This is scoped to the Referral page and its theme entry — the shared HeaderPageLayout and the iconColorfulBackground token are untouched, and the only other consumers of PAGE_THEMES (Save the World, Sign-in, Wallet, Status) are unaffected.
Fixed Issues
$#92290
PROPOSAL:#92290 (comment)
Tests
Precondition: Be logged in and have created workspace
Offline tests
N/A — change is presentational only.
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
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