-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Fix: Replace FullScreenLoadingIndicator with ActivityIndicator in AddressSearch Component
#76156
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers! |
|
@Krishna2323 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] |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
Julesssss
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @PiyushChandra17, could you please add further test cases to verify other uses are also working as expected, thanks:
- Settings > Profile > Personal Details > Address
- Settings > Wallet > Cards > Digital Details > Update Address
- Workspace > Bank account setup > Bank Account Details
- Workspace > Settings > Overview > Address
- Create distance expense > Add/edit waypoint
- Settings > Subscription > Add payment card
|
@Julesssss I think i had already added 2 test cases, i think that's enough. It's pretty self-explanatory that other test cases uses will also work perfectly the same. Let me know wdyt? Also do you want me to update screenshots/recordings of all remaining test cases mentioned ^. Thanks! |
@Julesssss Yeah it does make sense to add all test cases to verify the loader is working as expected in other parts aswell. Will do that tomm. morning first time at the desk .. Thanks! |
| </View> | ||
| </ScrollView> | ||
| {isFetchingCurrentLocation && <FullScreenLoadingIndicator />} | ||
| {isFetchingCurrentLocation && <ActivityIndicator size="large" />} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use CONST.ACTIVITY_INDICATOR_SIZE.LARGE.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Krishna2323 exactly i was thinking the same, thanks
|
@PiyushChandra17 the UI interactivity is not the same as before, now I can interact with the input and the loader position is also not fixed. Also, the background opacity/color is also changed. FullScreenLoadingIndicatorMonosnap.screencast.2025-11-28.13-43-22.mp4ActivityIndicatorMonosnap.screencast.2025-11-28.13-44-05.mp4 |
@Krishna2323 I think we need apply |
@PiyushChandra17 please make sure that it looks the same as before. |
|
🚧 @Julesssss 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! 🧪🧪
|
|
@PiyushChandra17 it still doesn't look the same: FullScreenLoadingIndicatorMonosnap.screencast.2025-11-30.12-19-37.mp4ActivityIndicatorMonosnap.screencast.2025-11-30.12-19-14.mp4 |
@Krishna2323 Can you please check once again Current FullScreenLoadingIndicator current-search-address01-fullscreenloadingindicator.movLatest-styling-change-ActivityIndicator up-to-date-search-address-activity-indicator.mov |
|
@PiyushChandra17 the bg is still not transparent. |
|
@Krishna2323 What do you mean by background still not being transparent? |
|
@PiyushChandra17 in the current |
Explanation of Change
Replace
FullScreenLoadingIndicatorwithActivityIndicatorinAddressSearchComponentFixed Issues
$ #75535
PROPOSAL: #75535(Comment)
Tests
Test case 1:
Loadingis correctTest case 2:
PaymentCardFormusesAddressSearchcomponent > Verify the display ofLoadingis correctTest case 3:
Loadingis correctTest case 4:
Loadingis correctTest case 5:
Loadingis correctLoadingis correctOffline tests
Same as tests
QA Steps
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, 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.ScrollViewcomponent to make it scrollable when more elements are added to the page.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
android-mweb-chrome-td-ai.mov
android-mweb-chrome-activity-indicator.mov
android-mweb-chrome-personaladdress-activity-indicator.mov
android-mweb-chrome-worspace-overview-ai.mov
android-mweb-chrome-wallet-ba-ai.mov
iOS: Native
iOS: mWeb Safari
ios-mweb-safari-td-ai.mov
ios-mweb-safari-activity-indicator.mov
ios-mweb-safari-personaladdress-activity-indicator.mov
ios-mweb-safari-workspaceoverview-ai.mov
ios-mweb-safari-wallet-ba-ai.mov
MacOS: Chrome / Safari
macos-chrome-td-ai.mov
macos-chrome-activity-indicator.mov
macos-chrome-personaladress-activity-indicator.mov
macos-chrome-workspace-overview-ai.mov
macos-chrome-wallet-ba-ai.mov
MacOS: Desktop
macos-desktop-td01-ai.mov
macos-desktop-activity-indicator.mov
macos-desktop-personaladdress-activity-indicator.mov
macos-desktop-workspace-overview-ai.mov
macos-desktop-wallet-ba-ai.mov