Use lazy loading for company cards#75279
Conversation
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@ShridharGoel Could you take a look at the errors here? Thank you. |
|
@brunovjk Updated. |
|
Great! Reviewing ASAP. |
|
@ShridharGoel Could you resolve the conflict? Thank you! |
|
Updated. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridApp75279_android_nattivv.movAndroid: mWeb Chrome75279_android_web.moviOS: HybridApp75279_ios_web.moviOS: mWeb Safari75279_ios_web.movMacOS: Chrome / Safari75279_web_chrome.mov |
src/hooks/useCompanyCardIcons.ts
Outdated
| import {COMPANY_CARD_BANK_ICON_NAMES, COMPANY_CARD_FEED_ICON_NAMES} from '@libs/CardUtils'; | ||
| import {useMemoizedLazyIllustrations} from './useLazyAsset'; | ||
|
|
||
| function useCompanyCardFeedIcons() { |
There was a problem hiding this comment.
@ShridharGoel should we add explicit return types? Thanks.
|
We have some falling checks here @ShridharGoel, could you fix them for us? Thank you. |
|
@brunovjk Updated. |
|
Reviewing ASAP |
JmillsExpensify
left a comment
There was a problem hiding this comment.
No review required from a product perspective. Unsubscribing.
|
@ShridharGoel Could you resolve the conflict? Thank you! |
|
I'll review it now, @ShridharGoel, we still have some conflicts. Thank you. |
|
@brunovjk Actually this keeps getting conflicts because it seems these files are being frequently changed. I'll update to fix them. |
|
@brunovjk Updated. |
|
Sorry for the delay @ShridharGoel, I've been busy. I'll review it here today. Could you resolve the conflicts? Thank you very much. |
|
@brunovjk Updated |
|
@ShridharGoel Do we need to provide steps to test this PR? I tested the app's overall functionality and it seems fine. |
|
We just need to check that the various icons are being rendered properly. Is it fine to mention it as NoQA ? |
|
Good question. @mollfpr can you help us confirm if we need QA here? Thank you. |
|
Let's not add NoQA here since it only focuses on the company card assets, and list out the assets that migrated so the QA team will be aware of the migrated assets. |
|
Great, thanks for the clarification. @ShridharGoel please update the PR and the Test Steps accordingly. Let me know if you need anything. Thanks. |
|
Updated |
|
@mollfpr do you think the "Test Steps" here are sufficient? Thank you. cc: @ShridharGoel |
|
@ShridharGoel, could you give a few example steps to verify the asset? |
|
Added |
|
🚀 Deployed to staging by https://github.com/mollfpr in version: 9.2.75-0 🚀
|
|
🚀 Deployed to staging by https://github.com/mollfpr in version: 9.2.75-0 🚀
|
|
🚀 Deployed to staging by https://github.com/mollfpr in version: 9.2.77-0 🚀
|
|
🚀 Deployed to production by https://github.com/yuwenmemon in version: 9.2.77-1 🚀
|
Explanation of Change
Use lazy loading for company cards
List of migrated assets:
Fixed Issues
$ #75247
PROPOSAL: #75247 (comment)
Tests
Offline tests
QA Steps
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
MacOS: Desktop