-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Fix uber infinite loading button #79997
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
Conversation
|
@jayeshmangwani 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❌ 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.
|
joekaufmanexpensify
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.
Makes sense from a product perspective 👍
|
@iwiznia, does this PR require a C+ review? |
|
Yes please |
|
@DylanDylann 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: HybridAppScreen.Recording.2026-01-26.at.14.50.21.movAndroid: mWeb ChromeScreen.Recording.2026-01-26.at.14.48.54.moviOS: HybridAppScreen.Recording.2026-01-26.at.14.48.06.moviOS: mWeb SafariScreen.Recording.2026-01-26.at.14.47.23.movMacOS: Chrome / SafariScreen.Recording.2026-01-26.at.14.46.16.mov |
|
@iwiznia The PR is working well. However, I think we should define a dedicated loading state instead of using |
|
Why add a new piece of data if we can do the same with the data we have? |
|
@iwiznia I think using a loading flag would be more correct. We can't guarantee that policy?.receiptPartners?.uber will always return a value, the API could fail for various reasons, and in that case, the loading state would stay infinite. The loading indicator should only show while the API/network request is in progress. Once the API completes, whether it returns no data or fails, we should stop displaying the loading. |
|
Ah true. Updated |
DylanDylann
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.
LGTM
|
@chuckdries 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] |
| style={styles.justifyContentCenter} | ||
| small | ||
| isLoading={!policy?.receiptPartners?.uber} | ||
| isLoading={!isOffline && !!policy?.isLoadingReceiptPartners} |
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.
Because openPolicyReceiptPartnersPage already been triggered when going to the Receipt partners page. So we only need to display the loading indicator the first time when policy?.receiptPartners?.uber haven't loaded before
| isLoading={!isOffline && !!policy?.isLoadingReceiptPartners} | |
| isLoading={policy?.receiptPartners?.uber ? false : (!isOffline && !!policy?.isLoadingReceiptPartners)} |
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.
Ah you are right. Updated
chuckdries
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.
Code looks good. Lint is failing because we're over our threshold for the max number of warnings we can have lol. They bumped it in main - merge main and it should fix it
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/iwiznia in version: 9.3.11-0 🚀
|
Explanation of Change
Fixed Issues
$#79870
Tests
Screen.Recording.2026-01-20.at.12.02.17.PM.mov
Offline tests
See above.
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
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
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari