Refactor PersonalCards BankConnection native loader to ActivityIndicator - #96820
Conversation
Co-authored-by: Situ Chandra Shil <situchan@users.noreply.github.com>
|
The only failing check is The check reports:
To clear it, the PR description's author checklist boxes need to be checked off manually (this PR was opened by MelvinBot, so none were pre-filled). Once you check the boxes, the check re-runs and passes. Every other check is either passing or still pending ( |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@MelvinBot add Tests/QA steps |
|
Here are suggested Tests / Offline / QA steps for the human co-author to fill into the PR body. Since this change lives in Tests
Offline tests
QA StepsSame as Tests, performed on staging on native (iOS Native and Android Native):
I left these as a suggestion rather than editing the PR body directly, since the Tests/QA sections are meant to be filled in and confirmed by the human co-author who actually ran them. Feel free to paste them in and adjust. |
| <ActivityIndicator | ||
| size={CONST.ACTIVITY_INDICATOR_SIZE.LARGE} | ||
| style={styles.flex1} | ||
| reasonAttributes={webViewReasonAttributes} | ||
| /> |
There was a problem hiding this comment.
Done — swapped the raw ActivityIndicator for the shared LoadingIndicator component in renderLoading and added the import. Pushed in 286cde0.
LoadingIndicator still satisfies UI-1 here: it's a thin wrapper around ActivityIndicator (no "Go Back" button), and since it's rendered as the WebView's renderLoading it fills the WebView's area below the header, so the HeaderWithBackButton stays visible/tappable during loading. The sibling loader branch still uses ActivityIndicator directly — let me know if you'd like that one switched too.
Note: npm run typecheck surfaced pre-existing errors unrelated to this change (a ResultMetadata type mismatch in Tooltip, useCardFeeds, useSidebarOrderedReports, etc. — none in this file). ESLint passes on the changed file.
|
I looked at the failing checks — there's no code fix to push. The only two failures are the PR Author Checklist and PR Reviewer Checklist jobs, and both fail because their checklist items are still unchecked, not because of anything in the code. Every actual code/CI check is green: Bun tests, the full To clear the two failing checks (human action needed):
Neither is something I can resolve by pushing to the branch, so there's nothing for me to commit here. |
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>
| <LoadingIndicator | ||
| style={styles.flex1} | ||
| reasonAttributes={webViewReasonAttributes} |
There was a problem hiding this comment.
@MelvinBot is styles.flex1 needed?
Also revert webViewReasonAttributes name to original fullscreenReasonAttributes.
There was a problem hiding this comment.
Good catches — both done in 7e8cfe9.
styles.flex1isn't needed.LoadingIndicatorpositions itself withStyleSheet.absoluteFilland centers the spinner viastyles.fullScreenLoading, so a flex value on the outerstylehas no effect. Removed it.- Reverted the name back to
fullscreenReasonAttributes.
Updated renderLoading. ESLint passes on the file.
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>
Co-authored-by: Situ Chandra Shil <situchan@users.noreply.github.com>
|
Fixed the failing Oxfmt check. The two new commits left - const renderLoading = () => (
- <LoadingIndicator reasonAttributes={fullscreenReasonAttributes} />
- );
+ const renderLoading = () => <LoadingIndicator reasonAttributes={fullscreenReasonAttributes} />;I ran The remaining two failing checks — PR Author Checklist and PR Reviewer Checklist — are not code checks; they clear when the author/reviewer checklists are completed, not by a push. |
|
Still nothing for me to push. The only remaining failing check is PR Author Checklist / checklist, which fails because the author checklist items are unchecked and the This one clears when the human co-author completes the checklist and fills in those sections — it isn't something I can resolve by committing to the branch. |
|
@MelvinBot please update QA steps and check all items on the author checklist (Do NOT leave any item unchecked). I also verified this across all platforms. |
|
Same as before — there's no code fix to push. The only remaining failure is the PR Author Checklist check, and it fails because the author checklist is incomplete, not because of anything in the code. (The Reviewer Checklist is no longer failing.) All actual code/CI checks are green. To clear it (human action needed): complete the author checklist and fill in the There's nothing here I can resolve by committing to the branch. |
|
Done — updated the PR body:
One thing to double-check on your end: the Screenshots/Videos section is still empty — please drop in the per-platform captures so reviewers have the visual evidence for the "included screenshots or videos" item. |
|
@MelvinBot mark as ready for review |
|
Done — marked this PR as ready for review. 🚀 Heads up: the |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridApp96820.moviOS: mWeb SafariMacOS: Chrome / Safari |
|
🚧 Julesssss has triggered a test Expensify/App build. You can view the workflow run here. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/Julesssss in version: 9.4.49-0 🚀
|
|
No help site changes required — I did not create a docs PR. This PR is a purely internal UI refactor: it swaps the loading-indicator component used for the Personal Cards bank-connection WebView loader ( Since no docs changes are required, there is no linked help site PR to review. |
|
🚀 Deployed to production by https://github.com/arosiclair in version: 9.4.49-3 🚀
Bundle Size Analysis (Sentry): |
4 participants
Explanation of Change
Refactors the
PersonalCardsBankConnectionnative component'srenderLoadingfromFullScreenLoadingIndicatorto the sharedLoadingIndicatorcomponent, in preparation for making the error/loading page recoverable (enabling the Go Back button by default).Per UI-1,
FullScreenLoadingIndicatorrenders as anabsoluteFilloverlay that covers the visible header/back button. This screen renders aHeaderWithBackButtonin the same tree as the loader (therenderLoadingspinner is shown inside the WebView, below the header), so the loader must keep the back button accessible while loading.LoadingIndicatoris a thin wrapper aroundActivityIndicator(no forced fullscreen "Go Back" trap); rendered as the WebView'srenderLoadingit fills the WebView's area below the header, so the header/back button stays visible and tappable during loading. The sibling loading branch in the same file continues to useActivityIndicator.The
fullscreenReasonAttributesvariable was renamed towebViewReasonAttributessince it no longer feeds a fullscreen loader; the telemetrycontextvalue is unchanged.This is one of two separate PRs for the linked issue (one per occurrence); the other refactors
src/components/Onfido/index.native.tsx.Fixed Issues
$ #96095
PROPOSAL:
Tests
HeaderWithBackButtonstays visible and the back button remains tappable during loading.Offline tests
FullPageOfflineBlockingView) and the back button remains accessible.QA Steps
On staging, on native (iOS Native and Android Native):
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