-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Replace FullScreenLoadingIndicator with ActivityIndicator in image components
#76001
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?
Replace FullScreenLoadingIndicator with ActivityIndicator in image components
#76001
Conversation
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
4e10c9f to
955a17a
Compare
|
@bernhardoj 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] |
|
@Julesssss here is the second PR after this #75031 |
|
@Julesssss Since all of the new refactorings have the same styling that were in the Applied them on the PDF loaders as well. |
|
There was also a bug in the loading indicator when creating an expense is not centered Screen.Recording.2025-12-14.at.23.22.12.movso this is now fixed after using the shared component: Screen.Recording.2025-12-15.at.01.28.09.mov |
|
Is C+ needed here? |
JmillsExpensify
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.
Product review not strictly required.
Yes, please go ahead @bernhardoj |
|
@abzokhattab please merge with main |
|
There u go @bernhardoj |
@abzokhattab I changed the pdf code so that it always shows the loading, but I can't seem to see the LoadingIndicator. If I change it to simple Text, then the text shows. Please see the video below: web.mp4 |
| const styles = useThemeStyles(); | ||
|
|
||
| return ( | ||
| <View style={[StyleSheet.absoluteFillObject, styles.fullScreenLoading, styles.w100, style]}> |
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.
If I remove the absolute style, then the loading indicator shows. Btw, I noticed that the structure here looks the same as the FullScreenLoadingIndicator, just without a back button. Maybe we should keep using plain ActivityIndicator for the pdf?
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.
I think its just cleaner this way since most of the instances are using the same styling inside the the Loading indicator so i believe its worth it to have this new component
Or we can keep the full screen loading indicator and add a param that would disable the backTo
What do you think
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.
Or we can keep the full screen loading indicator and add a param that would disable the backTo
This is what is already happening right now, that's why we won't see the back button even without this PR, actually, but it's only temporary.
| shouldUseGoBackButton = false, |
App/src/components/FullscreenLoadingIndicator.tsx
Lines 62 to 72 in 2dcf2fc
| {showGoBackButton && ( | |
| <View style={styles.loadingMessage}> | |
| <View style={styles.pv4}> | |
| <Text>{translate('common.thisIsTakingLongerThanExpected')}</Text> | |
| </View> | |
| <Button | |
| text={translate('common.goBack')} | |
| onPress={() => Navigation.goBack()} | |
| /> | |
| </View> | |
| )} |
I think its just cleaner this way since most of the instances are using the same styling inside the the Loading indicator so i believe its worth it to have this new component
I don't think there are that many components that show a loading indicator with this structure, but I get your point that most of the time, we want to show a loading indicator that is shown in the center, so I'm fine to keep the component, but we still need to fix the loading indicator on the PDF components.
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.
Okay i fixed the loader for the pdf:
screen-recording-2025-12-24-at-184822_at52s52W.mov
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.
Hmm, I'm still unable to see it. Maybe there is some code that you forgot to push?
web.mp4
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.
can you check now ... here is what i have
Screen.Recording.2025-12-27.at.10.32.25.mov
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid.mp4Android: mWeb Chromeandroid.mweb.mp4iOS: HybridAppios.mp4iOS: mWeb Safariios.mweb.mp4MacOS: Chrome / Safariweb.mp4 |
| accessibilityLabel={translate('accessibilityHints.viewAttachment')} | ||
| disabled={!shouldDisplayReceipt} | ||
| disabledStyle={styles.cursorDefault} | ||
| style={[styles.h100, styles.flex1]} |
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.
From my testing, we don't need the flex1 style here.
| <PDFThumbnail | ||
| // eslint-disable-next-line @typescript-eslint/non-nullable-type-assertion-style | ||
| previewSourceURL={resolvedReceiptImage as string} | ||
| style={[styles.w100, styles.h100]} |
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.
From my testing, we don't need the w100 style here.
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.
NAB: we can even remove the h100 style here and here
App/src/components/ReceiptImage/index.tsx
Lines 174 to 178 in b8ece4b
| if (isPDFThumbnail) { | |
| return ( | |
| <PDFThumbnail | |
| previewSourceURL={source ?? ''} | |
| style={[styles.w100, styles.h100]} |
Then, we can always apply the h100 here instead of only when it fails
App/src/components/PDFThumbnail/index.tsx
Lines 57 to 58 in b8ece4b
| return ( | |
| <View style={[style, styles.overflowHidden, failedToLoad && styles.h100]}> |
and move the style prop to the last item of the array.
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.
hmmm i removed the w100 but i see that most of the occureneces have both styles.w100, styles.h100 i am not sure if we should remove that but can u please check if it will cause any regressions ...
i am fine with changing the App/src/components/ReceiptImage/index.tsx and PDFThumbnail/index.tsx but lets see if it will cause any issues
|
Please also fix the lint, and we are good. |


Explanation of Change
Fixed Issues
$ #74935
PROPOSAL:
Tests
Test 1: Image Loading Spinner in Full Image View
Steps:
Test 2: Thumbnail Image Loading in Chat/Report
Steps:
Offline 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
Screen.Recording.2025-12-15.at.00.36.42.mov
Android: mWeb Chrome
Screen.Recording.2025-12-15.at.00.37.41.mov
iOS: Native
Screen.Recording.2025-12-15.at.00.00.30.mov
iOS: mWeb Safari
Screen.Recording.2025-12-15.at.00.35.41.mov
MacOS: Chrome / Safari
Screen.Recording.2025-12-14.at.23.58.15.mov