[No QA] [BYOC] [Bulk Card Assignments] Release 1: Workspace Company Cards Table#77701
Conversation
|
Hey, I noticed you changed If you want to automatically generate translations for other locales, an Expensify employee will have to:
Alternatively, if you are an external contributor, you can run the translation script locally with your own OpenAI API key. To learn more, try running: npx ts-node ./scripts/generateTranslations.ts --helpTypically, you'd want to translate only what you changed by running |
| if (assignedCard) { | ||
| if (!assignedCard?.accountID || !assignedCard?.fundID) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
❌ PERF-2 (docs)
This expensive function call (getCardFeedIcon()) is executed unconditionally inside renderItem for every list item, but it's only used when the card is unassigned (!assignedCard). This wastes computation for all assigned cards.
Suggested fix: Move this call inside a conditional check or compute it lazily only when needed:
const cardFeedIcon = !assignedCard ? getCardFeedIcon(selectedFeed as CompanyCardFeed, illustrations, companyCardFeedIcons) : undefined;Or better, check if assignedCard exists first and only call getCardFeedIcon in the unassigned branch where it's actually used.
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
| if (assignedCard) { | ||
| if (!assignedCard?.accountID || !assignedCard?.fundID) { | ||
| return; | ||
| } | ||
|
|
||
| return Navigation.navigate( | ||
| ROUTES.WORKSPACE_COMPANY_CARD_DETAILS.getRoute( | ||
| policyID, | ||
| assignedCard.cardID.toString(), | ||
| getCompanyCardFeedWithDomainID(assignedCard?.bank as CompanyCardFeed, assignedCard.fundID), | ||
| ), | ||
| ); |
There was a problem hiding this comment.
Would it be better to put this logic in the parent inside the onAssignCard() callback?
There was a problem hiding this comment.
No, that just makes the logic more complex, since we need the assignedCard value which we get within the list component
Co-authored-by: Tim Golen <tgolen@gmail.com>
|
Looks like quite a few of the checks are failing, but the code looks good overall |
|
No C+ or product person necessary for this PR. We'll have you review the final PRs that go to |
|
@tgolen The TS checks are failing because we need to run the translations script as in: #77701 (comment) Removed the manual memoization reported by React Compiler Compliance Check, but the check is faulty anyway atm (We're updating it here atm) |
|
@tgolen i think we can tackle Code coverage at the end of the release/sprint PR, right? I think that might smarter |
ac82086
into
Expensify:byoc-bulk-card-assign-r1
|
🚀 Deployed to staging by https://github.com/tgolen in version: 9.2.86-0 🚀
|
|
🚀 Deployed to production by https://github.com/marcaaron in version: 9.2.86-4 🚀
|
@tgolen
Explanation of Change
Changes the company cards list to show both assigned and unassigned cards and updates the layout to list to be a three-column table (not yet sortable/filterable/searchable), based on the according design doc
Fixed Issues
$ #77702
PROPOSAL:
Tests
None needed.
Offline tests
None needed.
QA Steps
None needed.
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