[Payment due @situchan] Refactor BusinessInfo IncorporationStateBusiness loader to ActivityIndicator - #96831
[Payment due @situchan] Refactor BusinessInfo IncorporationStateBusiness loader to ActivityIndicator#96831MelvinBot wants to merge 2 commits into
Conversation
…dicator Co-authored-by: Situ Chandra Shil <situchan@users.noreply.github.com>
|
Nothing to fix here in code — the only failing check is PR Author Checklist / checklist, and it's failing for a non-code reason. The job log shows:
Every real check (Bun tests, This check turns green once the PR Author Checklist is completed — which is a human co-author task, not something I can resolve by pushing code:
No code push is warranted for this failure. |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@MelvinBot add Tests/QA steps |
|
Here are suggested Tests
Offline tests
QA Steps
|
|
The only failing check is
Every other check (ESLint, typecheck, Jest, Storybook, build, perf, etc.) is passing. This one can only be cleared by a human ticking the PR Author Checklist boxes in the description after completing the manual
|
…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>
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/Videos@MelvinBot please set the PR body to the content in the details section above |
|
|
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/Videos11-IncorporationStateBusiness.mov11-IncorporationStateBusiness-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 all 38 items in the PR Author Checklist — the For my own due diligence I also statically verified the one-file change against the pattern it claims to mirror: Verification details
I attempted a live web drive of the Business info → Incorporation state loading state, but the browser session couldn't sign in (a transient environment/session issue, unrelated to this change), so I couldn't capture a runtime screenshot. The static review above plus your cross-platform verification cover the change. |
|
@MelvinBot mark as ready for review |
|
🤖 Marked this PR as ready for review.
The change itself is a like-for-like loader swap ( |
|
@danieldoglas 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: HybridApp11-IncorporationStateBusiness-mobile.moviOS: mWeb SafariMacOS: Chrome / Safari11-IncorporationStateBusiness.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. |
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