Repository navigation
[No QA] Expand integrationIcons render-array into explicit JSX in ExportedIconCell - #100262
Conversation
…nCell Co-authored-by: Manan <mananjadhav@users.noreply.github.com>
melvin-bot
Bot
added
Melvin-Test-Android
Melvin-Test-Web
|
🤖 Android test steps verification results: This PR has no filled-in
What this run does not prove: the icons this PR touches were never rendered on screen, for two independent reasons — the narrow phone layout picks |
|
No code fix is needed — every automated check on this PR passes. The single failing check is a process gate, not a build or test failure.
Full check statusPassing: Failing: I did not push a commit, since there is no code defect to correct. |
|
🤖 Web test steps verification results: Tested on dev NewDot at 1440x900 (wide layout, so the
What this run does not prove: no row rendered an actual exported-integration icon, so the icons this PR touches were never drawn on screen. |
mananjadhav
left a comment
mananjadhav
left a comment
There was a problem hiding this comment.
The code changes are fine.
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppNA. Only for desktop. Android: mWeb ChromeNA. Only for desktop. iOS: HybridAppNA. Only for desktop. iOS: mWeb SafariNA. Only for desktop. MacOS: Chrome / Safari
|
mananjadhav
left a comment
mananjadhav
left a comment
There was a problem hiding this comment.
I don't think we'll need QA on this one.
roryabraham
changed the title
|
@mananjadhav 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] |
mananjadhav
left a comment
mananjadhav
left a comment
There was a problem hiding this comment.
Approving again. Checklist here.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| /> | ||
| )} | ||
| {integrationIcons.map(({name, src}) => ( | ||
| {isExportedToNetsuite && ( |
There was a problem hiding this comment.
❌ CONSISTENCY-3 (docs)
This change replaces a single, data-driven icon list with ten near-identical <Icon> JSX blocks that differ only in the guarding boolean and the src value. Every block repeats the same size={CONST.ICON_SIZE.SMALL} and additionalStyles={[integrationIconStyle]} structure, so adding, removing, or restyling an integration icon now requires editing every copy, raising maintenance overhead and bug risk (DRY violation).
Consolidate the repetition by iterating over a small config that maps a condition to its icon source, keeping a single <Icon> definition:
const integrationIcons = [
{show: isExportedToNetsuite, name: 'NetSuiteSquare', src: icons.NetSuiteSquare},
{show: isExportedToXero, name: 'XeroSquare', src: icons.XeroSquare},
{show: isExportedToIntacct, name: 'IntacctSquare', src: icons.IntacctSquare},
{show: isExportedToQuickbooksOnline || isExportedToQuickbooksDesktop, name: 'QBOSquare', src: icons.QBOSquare},
{show: isExportedToIntuitEnterpriseSuite, name: 'IntuitSquare', src: icons.IntuitSquare},
{show: isExportedToCertinia, name: 'CertiniaSquare', src: icons.CertiniaSquare},
{show: isExportedToRillet, name: 'RilletSquare', src: icons.RilletSquare},
{show: isExportedToDualEntry, name: 'DualEntrySquare', src: icons.DualEntrySquare},
{show: isExportedToBillCom, name: 'BillComSquare', src: icons.BillComSquare},
{show: isExportedToZenefits, name: 'ZenefitsSquare', src: icons.ZenefitsSquare},
];
// ...
{integrationIcons
.filter(({show}) => show)
.map(({name, src}) => (
<Icon
key={name}
src={src}
size={CONST.ICON_SIZE.SMALL}
additionalStyles={[integrationIconStyle]}
/>
))}This keeps a single source of truth for the <Icon> markup while still avoiding the removed inline object churn.
Reviewed at: 5398ced | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 roryabraham has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/roryabraham in version: 9.4.74-0 🚀
|
|
🤖 No help site changes are required for this PR, so I did not create a draft docs PR. This PR is a pure render refactor of one component. It replaces the Nothing user-facing changes:
I checked Since no docs PR was needed, there is no linked help site PR to review. If you'd like one anyway — for example to document the exported-integration indicator itself — reply with what you want covered and I'll draft it. |
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.4.74-2 🚀
|
Explanation of Change
Follow-up to the review comment on #98031 (comment).
ExportedIconCellbuilt anintegrationIconsarray of{name, src}objects gated on booleans, filtered the falsy entries with a type predicate, then rendered it with.map. The two icons immediately above it (TableandTablePencil) already rendered as plain conditional JSX, so the same component used two patterns for the same thing. The array form also invites future authors to add configuration props to the mapped element, and React Compiler memoizes the whole.mapcallback as one unit instead of memoizing each icon node.This PR:
integrationIconsarray and the.map, replacing them with ten conditional<Icon>blocks — one per integration, in the same order — matching theTable/TablePencilblocks above them. The QuickBooks block keeps itsisExportedToQuickbooksOnline || isExportedToQuickbooksDesktopcondition.StyleUtils.getAvatarBorderStyle(CONST.AVATAR_SIZE.XXX_SMALL, CONST.AVATAR_SHAPE.CIRCLE)call into a localintegrationIconStyleconst instead of repeating the call in every block.keyprops, which are no longer needed now that the children are static.IconAssettype import, which was only used by the filter's type predicate.No behavior change: the same icons render, in the same order, at the same size, with the same circular border. The loop that computes the
isExportedTo*booleans is untouched.AI Tests
Run locally on this branch by MelvinBot:
npm run lint-changed— passednpm run typecheck— passednpm run react-compiler-compliance-check check src/components/Search/SearchList/ListItem/ExportedIconCell.tsx— passed (no compile failure and no memoization divergence under either the Babel or OXC compiler)npm run spell-changed— passed (0 issues)npm test -- tests/unit/TransactionGroupListItemTest.tsx tests/unit/Search/ExpenseFlatSearchViewTest.tsx tests/unit/Search/ExpenseReportSearchViewTest.tsx— passed (29 tests). No test file referencesExportedIconCelldirectly; these cover the Search list rows that render it.Not run:
npm run prettier(this repo has noprettiernpm script; formatting is enforced through ESLint, which passed). A browser smoke check of the Search list could not be completed — the web session in this run never rendered past a blank page, which is a test-infra failure unrelated to this diff.Fixed Issues
$ #99462
PROPOSAL: #99462 (comment)
Tests
see @mananjadhav's review checklist
Offline tests
None.
QA Steps
None.
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