Feature/add assigned card tile and display expensify cards#26862
Feature/add assigned card tile and display expensify cards#26862grgia merged 2 commits intoExpensify:mainfrom pac-guerreiro:feature/add-assigned-card-tile-and-display-expensify-cards
Conversation
|
cc @grgia |
|
@ 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] |
There was a problem hiding this comment.
This component seems redundant, do we really need it?
There was a problem hiding this comment.
Not sure, it was Pedro's decision to add it, maybe let's wait until tomorrow, so he can share his opinion on that, wdyt?
There was a problem hiding this comment.
@thiagobrez
It was requested on the technical document
There was a problem hiding this comment.
Where is this being used in this PR?
|
@grgia Is there anything else missing in this PR? |
|
I think we should expand on the tests section for this PR. I can help you write tests but we should cover all cases in the doc |
Ok, could you provide those test scenarios then? 😄 I'm not aware of the whole context |
|
@pac-guerreiro please complete author checklist |
|
@pac-guerreiro it looks like a unrelated unit test is failing, could you also merge main again and fix lint errors? |
|
Assigning @thesahindia for C+ review from issue |
|
Looks like some docs were renamed here too @pac-guerreiro |
|
@pac-guerreiro let's merge main and fix the lint errors please! Let's get this PR ready for C+ review 😁🚀 cc @thesahindia |
|
Taking the review over from @thesahindia! |
|
@allroundexperts assigned |
src/components/Section.js
Outdated
There was a problem hiding this comment.
We don't use this pattern anymore. This causes console errors on native platforms because React will try to render Boolean(props.subtitle). Try using ? : null.
There was a problem hiding this comment.
@allroundexperts I haven't gotten any console error from using this
Reviewer Checklist
Screenshots/VideosMobile Web - ChromeScreen.Recording.2023-10-13.at.1.49.48.AM.movMobile Web - SafariScreen.Recording.2023-10-13.at.1.48.00.AM.movDesktopScreen.Recording.2023-10-13.at.1.46.45.AM.moviOSScreen.Recording.2023-10-13.at.1.49.19.AM.movAndroidScreen.Recording.2023-10-13.at.1.50.48.AM.mov |
There was a problem hiding this comment.
Let's try to stay consistent and not introduce a new pattern.
There was a problem hiding this comment.
This pattern was suggested by @roryabraham
There was a problem hiding this comment.
@allroundexperts @grgia This pattern was recommended to me by @roryabraham and it's been used for other screens.
In my opinion, this pattern prevents us from repeating hardcoded values throughout the app.
src/libs/Navigation/linkingConfig.js
Outdated
|
@pac-guerreiro @allroundexperts how can we move this forward? |
# Conflicts: # src/ROUTES.ts # src/languages/en.ts # src/libs/Navigation/AppNavigator/ModalStackNavigators.js # src/libs/Navigation/linkingConfig.js # src/pages/settings/Wallet/PaymentMethodList.js # src/pages/settings/Wallet/WalletPage/BaseWalletPage.js # src/pages/settings/Wallet/assignedCardPropTypes.js add missing translations # Conflicts: # src/languages/es.ts translate title, remove unnecessary useCallback modify comment # Conflicts: # src/pages/settings/Wallet/WalletEmptyState.js add missing type to ROUTES remove unnecessary dependency # Conflicts: # src/pages/settings/Wallet/PaymentMethodList.js remove unused prop chore(wallet): resolve merge conflicts # Conflicts: # src/libs/Navigation/linkingConfig.js # src/pages/settings/Wallet/PaymentMethodList.js # src/pages/settings/Wallet/WalletPage/BaseWalletPage.js # src/pages/settings/Wallet/WalletPage/WalletPage.js fix(wallet): clicking on an expensify card causing app to crash refactor(expensify card): rename card active states constant and import it where needed fix(expensify card): domain cards not getting properly setup because of different name cases fix(wallet): assigned cards section spacing
Reviewer Checklist
Screenshots/Videos |
| CLOSED: 6, | ||
| STATE_SUSPENDED: 7, | ||
| }, | ||
| ACTIVE_STATES: [2, 3, 4, 7], |
There was a problem hiding this comment.
STATE: {
OPEN: 3,
NOT_ACTIVATED: 4,
STATE_DEACTIVATED: 5,
CLOSED: 6,
STATE_SUSPENDED: 7,
},
What is state 2 in Active_States? it's not in the State object.
There was a problem hiding this comment.
Ah it's the Card Not Issued state. I think this is a typo from the design doc. Let's Remove 2 @pac-guerreiro.
But also I'd consider this as NAB for now considering the priority of this PR
|
Edit: it's also present on @pac-guerreiro bug: console error on native. Steps:
|
|
@rushatgabhane I had completed the checklist already 😄 |
|
I've created an issue for the State here - |
|
Also, there is another console error as well that needs confirmation from @pac-guerreiro! |
|
@allroundexperts I confirmed that was due to incorrect onyx merge data |
Gotcha. Then this is good to merge! |
|
@allroundexperts I just wanted to get more eyes on this because we need to merge this to unblock some issues |
|
@allroundexperts @rushatgabhane @pac-guerreiro Great work on this everyone! Thank you all so much for your help |
|
Congrats, that’s your 5th PR merged! 🎉 Do you know about the ContributorPlus role? It’s an opportunity to earn more in the Expensify Open Source community. Keep up the great work - thanks! |
|
✋ 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/grgia in version: 1.3.85-0 🚀
|
|
🚀 Deployed to production by https://github.com/francoisl in version: 1.3.85-4 🚀
|
|
🚀 Deployed to staging by https://github.com/grgia in version: 1.3.86-0 🚀
|
|
🚀 Deployed to production by https://github.com/francoisl in version: 1.3.86-5 🚀
|
| iconWidth={item.iconSize} | ||
| badgeText={shouldShowDefaultBadge(filteredPaymentMethods, item.isDefault) ? translate('paymentMethodList.defaultPaymentMethod') : null} | ||
| wrapperStyle={styles.paymentMethod} | ||
| shouldShowRightIcon={shouldShowAssignedCards} |
There was a problem hiding this comment.
Coming from #38643, this PaymentMethodList is also used in Transfer account screen, in that screen, we don't show the right icon by original design









Details
This PR adds assigned card tile with expensify card list to the wallet page.
Docs: https://docs.google.com/document/d/1rFxJ78vz5On6zSWzYa51B9v-tyLdC5pUsBeLOLig0t4/edit#bookmark=id.39u16xejrb8c
Fixed Issues
$ #22872
PROPOSAL:
Tests
getComponentforSCREENS.SETTINGS.WALLET, inSettingsModalStackNavigatorto import the component from../../../pages/settings/Wallet/WalletPage/WalletPagecardListwith mock data:Offline tests
NA
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)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
Web
Mobile Web - Chrome
Mobile Web - Safari
Desktop
iOS
Android