[Payment due @situchan] Refactor Requestor PersonalInfo Address loader to ActivityIndicator - #96838
Conversation
Co-authored-by: Situ Chandra Shil <situchan@users.noreply.github.com>
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
There's nothing failing in the code — every code, type, lint, test, and perf check now passes. The only red check is That check fails for a non-code reason: the PR Author Checklist boxes are unticked and the Details
To get this PR green, complete the author checklist and replace the |
|
@MelvinBot add Tests/QA steps |
|
Added The steps walk to that substep, throttle the network so the Please review and adjust as needed, then check the boxes for the platforms you verify before marking the PR ready for review — I've drafted the steps but the manual test/QA runs and screenshots still need a human co-author. |
…stifyContentCenter
Co-authored-by: {"message":"Not Found","documentation_url":"https://docs.github.com/rest/issues/comments#get-an-issue-comment","status":"404"} <{"message":"Not Found","documentation_url":"https://docs.github.com/rest/issues/comments#get-an-issue-comment","status":"404"}@users.noreply.github.com>
|
The only failing check is This isn't a code failure and there's nothing to push a fix for. The PR Author Checklist job fails simply because the human-owned checkboxes in the PR description (test-on-all-platforms, screenshots, "no console errors" verification, etc.) haven't been ticked. It turns green once the author completes that checklist — no branch change makes it pass. Per the check-failure playbook, |
DetailsExplanation of ChangeThis PR refactors a single Reimbursement Account step-machine loading state from Fixed Issues$ #96093 Tests
Offline testsN/A QA StepsN/A
PR Author Checklist
Screenshots/Videos4-Address.mov4-Address-mobile.mov@MelvinBot please set the PR body to the content in the details section above |
|
|
|
@MelvinBot please check all items on the author checklist. I also verified this across all platforms. |
|
🤖 Checked off the author checklist. Code review confirms the change is correct and UI-1 compliant — but 2 items still need you before merge (screenshots + unit test). Verified via code review (checked):
Checked based on your "verified across all platforms" attestation: the platform test boxes (Android/iOS native + mWeb, MacOS), offline, high-traffic, and no-console-errors. Left unchecked — action needed from you:
Minor style nit (non-blocking)This substep centers with |
|
@MelvinBot feel free to check off remaining 6 items. I verified all of them. |
|
🤖 Done — checked off the remaining 6 items in the PR Author Checklist based on your verification:
All 38 checklist items are now ticked and there are no unchecked boxes left. The |
|
@MelvinBot mark as ready for review |
|
🤖 Done — marked this PR as ready for review. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| }; | ||
| return <FullScreenLoadingIndicator reasonAttributes={reasonAttributes} />; | ||
| return ( | ||
| <View style={[styles.flex1, styles.fullScreenLoading]}> |
There was a problem hiding this comment.
❌ UI-1 (docs)
This refactor swaps FullScreenLoadingIndicator for a centered ActivityIndicator so the step header/back button stay visible, which is the right direction. However, the wrapping View uses styles.fullScreenLoading, which is an absolute-fill overlay style: it sets backgroundColor: theme.componentBG, opacity: 0.8, and zIndex: 10 (see src/styles/index.ts), and everywhere else in the codebase it is intentionally paired with StyleSheet.absoluteFill to paint an opaque overlay over content (e.g. OnyxTabNavigator.tsx, FixPersonalCardConnectionPage/index.native.tsx). Applying it here with flex1 reintroduces the translucent full-bleed overlay semantics this change is trying to remove and diverges from the already-merged BankInfo pattern this PR says it mirrors.
Use plain centering styles instead of the overlay style, matching src/pages/ReimbursementAccount/USD/BankInfo/BankInfo.tsx:
return (
<View style={[styles.flex1, styles.alignItemsCenter, styles.justifyContentCenter]}>
<ActivityIndicator
size={CONST.ACTIVITY_INDICATOR_SIZE.LARGE}
reasonAttributes={reasonAttributes}
/>
</View>
);Reviewed at: 84e4118 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
This is intentional. Not valid.
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridApp4-Address-mobile.moviOS: mWeb SafariMacOS: Chrome / Safari4-Address.mov |
melvin-bot
Bot
changed the title
|
🎯 @situchan, thanks for reviewing and testing this PR! 🎉 A payment issue will be created for your review once this PR is deployed to production. If payment is not needed (e.g., regression PR review fix etc), react with 👎 to this comment to prevent the payment issue from being created. |
|
🚧 mollfpr 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! 🧪🧪
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/mollfpr in version: 9.4.50-0 🚀
|
|
🤖 No help site changes required. I reviewed this PR and it does not require any updates to Expensify's help site files under Why: This is a purely internal front-end refactor. The only change swaps a
Help articles describe what users do to connect a bank account, not which loading spinner renders during a fetch, so there's nothing to update here. No draft docs PR was created since none is needed. @situchan, if you believe a help site update is warranted here, let me know what behavior changed and I'll draft the docs PR. |
4 participants
Explanation of Change
Part of a series of PRs for #96093 — one PR per occurrence, as requested in the issue.
This PR refactors a single Reimbursement Account step-machine loading state from
FullScreenLoadingIndicatorto a centeredActivityIndicator(see the PR title and the one-file diff for the specific site). These pages render their loading state insideInteractiveStepWrapper, which shows the step header and back button. The previousFullScreenLoadingIndicator(an early-return in a substep, or a branch of the wrapper's children) rendered an absolute-fill overlay that painted over the visible back button while the reimbursement account data loaded — this violates UI-1. Wrapping anActivityIndicatorin a centeredViewkeeps the header/back button visible during loading and preserves the existingreasonAttributesskeleton-span telemetry. This mirrors the already-mergedBankInfostep, which uses the same pattern, and is in preparation for making the error/loading page recoverable (enabling the Go Back button by default).AI checks run locally
npm run typecheck-tsgo— passednpm run lint-changed— passednpm run react-compiler-compliance-check check <file>— passedThe full Jest suite and Storybook smoke test were not run: no unit/UI test renders these substep components, and the change is a like-for-like loader swap following an existing merged pattern. The human co-author should complete the manual
Tests/QAsteps below.Fixed Issues
#96093
PROPOSAL:
Tests
<) button remain visible — the spinner must NOT cover the back button with a full-screen overlay.Offline tests
QA Steps
<) button stay visible and are not covered by a full-screen overlay.PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, 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
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari