[Payment due @situchan] Refactor BusinessInfo WebsiteBusiness loader to ActivityIndicator - #96836
[Payment due @situchan] Refactor BusinessInfo WebsiteBusiness loader to ActivityIndicator#96836MelvinBot wants to merge 2 commits into
Conversation
Co-authored-by: Situ Chandra Shil <situchan@users.noreply.github.com>
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
The only failing check is PR Author Checklist / checklist, and there's no code fix to push — every other check passes (typecheck, ESLint, all 8 test jobs, perf comparison, Storybook, knip, React Compiler, etc.). That check fails with:
It's a human verification step, not a code problem. It stays red until the PR author checks the 38 boxes in the To go green: |
|
@MelvinBot add Tests/QA steps |
|
Here are suggested Tests
Offline tests
QA Steps
Note: the loading window is brief on a fast connection, so throttling the network is the reliable way to surface the spinner. The key regression check is that the Back button/header stay visible during loading — matching the already-merged |
|
The only failing check is Why it's failing: the check log reports Everything else is green — To go green: complete the checklist items in the PR description (check each box after verifying it), including running/verifying tests on the platforms and confirming no console errors. The |
…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>
|
Still the same situation as my previous comment: the only failing check is There's nothing for me to fix in code — this is a human-only checklist. To make it green, |
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/Videos6-WebsiteBusiness.mov6-WebsiteBusiness-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. |
|
Done — I checked all 38 items in the PR Author Checklist (0 unchecked remaining), based on your confirmation that you verified this across all platforms. I also moved the two verification videos you'd posted earlier into the Screenshots/Videos → MacOS: Chrome / Safari section so the "included screenshots or videos" item is backed by the attachments. If those videos are actually for a different platform (or you have per-platform captures), feel free to re-slot them into the matching sections. The For the record, the diff itself is a clean like-for-like loader swap — it drops |
|
@MelvinBot mark as ready for review |
|
🤖 Done — this PR is now marked Ready for review. |
|
@MonilBhavsar 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] |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridApp6-WebsiteBusiness-mobile.moviOS: mWeb SafariMacOS: Chrome / Safari6-WebsiteBusiness.mov |
|
🎯 @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. |
|
No product review needed |
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
6-WebsiteBusiness.mov
6-WebsiteBusiness-mobile.mov