[Payment due @situchan] Refactor BusinessInfo AddressBusiness loader to ActivityIndicator - #96829
Conversation
Co-authored-by: Situ Chandra Shil <situchan@users.noreply.github.com>
|
The only failing check here is PR Author Checklist /
Every other check is passing or still running; none are red for a code reason. This isn't something I can fix by pushing to the branch — the author checklist is meant to be completed by the human co-author, and the PR body still has the Let me know if you spot a different check failing and I'll take another look. |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@MelvinBot add Tests/QA steps |
|
Added They walk a reviewer/QA through reaching the Business info → Company address substep of the USD Connect-bank-account (VBBA) flow and, while the reimbursement account data is loading, verifying the new centered spinner keeps the step header and Back button visible (instead of the old full-screen overlay that covered it), that Back still works mid-load, and that the address form renders and submits normally afterward with no console errors. The steps note using DevTools throttling / a High Traffic account to make the brief loading state observable. I left every checkbox unchecked — the human co-author still needs to actually run these and capture the platform screenshots before marking the PR ready for review. |
|
The only failing check is It fails only because the checklist boxes aren't ticked:
There's no fix for me to push here — the PR Author Checklist is a human-only step, and I won't check those boxes (or fill the Full check statusFailing:
All other checks pass or are skipped (tests jobs 1–8, |
| }; | ||
| return <FullScreenLoadingIndicator reasonAttributes={reasonAttributes} />; | ||
| return ( | ||
| <View style={[styles.flex1, styles.alignItemsCenter, styles.justifyContentCenter]}> |
There was a problem hiding this comment.
@MelvinBot use styles.fullScreenLoading and remove styles.alignItemsCenter, styles.justifyContentCenter
There was a problem hiding this comment.
Done in 897f0c8 — switched to AddressBusiness.tsx:58:
<View style={[styles.flex1, styles.fullScreenLoading]}>styles.fullScreenLoading already sets justifyContent: 'center' and alignItems: 'center', so alignItemsCenter/justifyContentCenter were redundant and are removed. typecheck-tsgo, lint, and the React Compiler check pass on the file.
There was a problem hiding this comment.
@MelvinBot apply same to all other 11 PRs in #96093 (comment)
There was a problem hiding this comment.
@MelvinBot apply same fix to all other 11 PRs in #96093 (comment)
|
Both currently-failing checks are manual, human-only checklist steps — there's still no code problem and nothing for me to push:
Every functional check (tests, |
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>
|
After your new commit ( That check fails only because the 38 author-checklist boxes aren't ticked ( The |
|
Applied the same fix —
Each change is byte-identical to the one you verified here on #96829 ( |
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/Videos8-AddressBusiness.mov8-AddressBusiness-mobile.mov@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/Videos8-AddressBusiness.mov8-AddressBusiness-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 left the rest of the body untouched, and I didn't flip the PR out of draft since you only asked me to complete the checklist — let me know if you'd also like it marked ready for review. |
|
@MelvinBot mark as ready for review |
|
🤖 Done — this PR is now marked Ready for review. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridApp8-AddressBusiness-mobile.moviOS: mWeb SafariMacOS: Chrome / Safari8-AddressBusiness.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. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 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! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/mollfpr in version: 9.4.50-0 🚀
|
|
🤖 I reviewed this PR against Expensify's help site articles under Why: This is a purely internal UI refactor — it swaps a Help articles document how to use features (e.g. connecting a business bank account and entering company info), not the internal loading-indicator implementation, so there's nothing in @situchan, if you believe a documented behavior actually changed here, let me know and I'll take another look. |
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
AddressBusiness).<) 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