Migrate icons integration - status & indicators to lazy loading#76965
Conversation
|
Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers! |
|
@linhvovan29546 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!
|
| }, | ||
| ], | ||
| [icons.NewWindow, shouldShowEnterCredentials, shouldShowReinstallConnectorMenuItem, translate, isOffline, policy, connectedIntegration, startIntegrationFlow], | ||
| [ |
There was a problem hiding this comment.
Please fix this warning
| @@ -400,7 +418,7 @@ function PolicyAccountingPage({policy}: PolicyAccountingPageProps) { | |||
| ...(isEmptyObject(policy?.connections) || !isConnectionVerified ? [] : configurationOptions), | |||
| ]; | |||
| }, [ | |||
There was a problem hiding this comment.
Please fix this warning
There was a problem hiding this comment.
I am checking lint error and fixing. Could you wait me?
|
@annaweber830 Is this ready for review? |
Yes |
|
Next time, please only mark the PR as 'Ready' when it is actually ready. |
| return getMostRecentlyVisitedReport(reportsValues, allReportMetadata); | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
Why do we need to change these lines? This PR's scope is only icon migration. If that's triggering an ESLint failure, why is the lint error appearing only in this PR and not in others?
| : { | ||
| description: translate('workspace.intacct.entity'), | ||
| iconRight: Expensicons.ArrowRight, | ||
| iconRight: icons.ArrowRight, |
There was a problem hiding this comment.
Missing dependency icons
|
PR doesn’t need product input as a migration PR. Unassigning and unsubscribing myself. |
| * Whether the provided report has expenses | ||
| */ | ||
| function hasExpenses(reportID?: string, transactions?: Array<OnyxEntry<Transaction>>): boolean { | ||
| function hasExpenses(reportID?: string, transactions?: Transaction[] | Array<OnyxEntry<Transaction>>): boolean { |
There was a problem hiding this comment.
Same as #76965 (comment) Could you please re-check all changes in this file? These unnecessary modifications create noisy commits
There was a problem hiding this comment.
Could you please re-check all changes in this file?
👆
There was a problem hiding this comment.
There are still many unnecessary modification types. I didn't add a comment since it changes too much lines.
There was a problem hiding this comment.
Got it, I will look into it further.
|
Hi @linhvovan29546, should I go ahead and resolve these conflicts, or should I wait? |
Please resolve the conflict. Also, please don't forget this comment |
linhvovan29546
left a comment
There was a problem hiding this comment.
Could you please fix the ESLint failure by adding // eslint-disable-next-line @typescript-eslint/no-deprecated?
|
|
||
| const transaction = linkedTransaction ?? getLinkedTransaction(reportAction ?? undefined); | ||
|
|
||
| // In case the transaction is failed to be created, we should disable editing the money request |
There was a problem hiding this comment.
Please re-add this comment
There was a problem hiding this comment.
I re-added this comment and ready for review.
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2025-12-11.at.19.02.10.movAndroid: mWeb ChromeScreen.Recording.2025-12-11.at.18.59.43.moviOS: HybridAppScreen.Recording.2025-12-11.at.19.05.56.moviOS: mWeb SafariScreen.Recording.2025-12-11.at.19.03.39.movMacOS: Chrome / SafariScreen.Recording.2025-12-11.at.18.55.00.mov |
|
QA can test this, so we should have a few simple test step to verify the migrated assets is working. @annaweber830 Could you add the test step and you should have recording verifying the asset like in the reviewer checklist. |
Hi @mollfpr I added test step and recording my PR. |
|
✋ 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/mollfpr in version: 9.2.78-0 🚀
|
|
🚀 Deployed to production by https://github.com/AndrewGable in version: 9.2.78-8 🚀
|
Explanation of Change
Fixed Issues
$#75257
PROPOSAL:#75257 (comment)
Tests
Offline tests
Same as tests
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
Screen.Recording.2025-12-11.at.9.41.07.PM.mp4
Android: mWeb Chrome
Screen.Recording.2025-12-11.at.9.46.41.PM.mp4
iOS: Native
Screen.Recording.2025-12-11.at.7.07.48.PM.mp4
iOS: mWeb Safari
Screen.Recording.2025-12-11.at.7.15.11.PM.mp4
MacOS: Chrome / Safari
Screen.Recording.2025-12-11.at.1.37.36.PM.mp4