-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add ability to bulk select cards from the same bank in the Card filter #53389
base: main
Are you sure you want to change the base?
Add ability to bulk select cards from the same bank in the Card filter #53389
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of these stuff looks ok, although logic is pretty complex 😅
Left some small comments
src/pages/Search/SearchAdvancedFiltersPage/SearchFiltersCardPage.tsx
Outdated
Show resolved
Hide resolved
src/pages/Search/SearchAdvancedFiltersPage/SearchFiltersCardPage.tsx
Outdated
Show resolved
Hide resolved
src/pages/Search/SearchAdvancedFiltersPage/SearchFiltersCardPage.tsx
Outdated
Show resolved
Hide resolved
src/pages/Search/SearchAdvancedFiltersPage/SearchFiltersCardPage.tsx
Outdated
Show resolved
Hide resolved
src/pages/Search/SearchAdvancedFiltersPage/SearchFiltersCardPage.tsx
Outdated
Show resolved
Hide resolved
src/pages/Search/SearchAdvancedFiltersPage/SearchFiltersCardPage.tsx
Outdated
Show resolved
Hide resolved
src/pages/Search/SearchAdvancedFiltersPage/SearchFiltersCardPage.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dropped small comments
Reviewer Checklist
Screenshots/VideosAndroid: NativeAndroid: mWeb ChromeiOS: NativeiOS: mWeb SafariMacOS: Chrome / SafariMacOS: Desktop |
src/pages/Search/SearchAdvancedFiltersPage/SearchFiltersCardPage.tsx
Outdated
Show resolved
Hide resolved
src/pages/Search/SearchAdvancedFiltersPage/SearchFiltersCardPage.tsx
Outdated
Show resolved
Hide resolved
src/pages/Search/SearchAdvancedFiltersPage/SearchFiltersCardPage.tsx
Outdated
Show resolved
Hide resolved
src/pages/Search/SearchAdvancedFiltersPage/SearchFiltersCardPage.tsx
Outdated
Show resolved
Hide resolved
src/pages/Search/SearchAdvancedFiltersPage/SearchFiltersCardPage.tsx
Outdated
Show resolved
Hide resolved
src/pages/Search/SearchAdvancedFiltersPage/SearchFiltersCardPage.tsx
Outdated
Show resolved
Hide resolved
src/pages/Search/SearchAdvancedFiltersPage/SearchFiltersCardPage.tsx
Outdated
Show resolved
Hide resolved
src/pages/Search/SearchAdvancedFiltersPage/SearchFiltersCardPage.tsx
Outdated
Show resolved
Hide resolved
src/pages/Search/SearchAdvancedFiltersPage/SearchFiltersCardPage.tsx
Outdated
Show resolved
Hide resolved
src/pages/Search/SearchAdvancedFiltersPage/SearchFiltersCardPage.tsx
Outdated
Show resolved
Hide resolved
src/pages/Search/SearchAdvancedFiltersPage/SearchFiltersCardPage.tsx
Outdated
Show resolved
Hide resolved
src/pages/Search/SearchAdvancedFiltersPage/SearchFiltersCardPage.tsx
Outdated
Show resolved
Hide resolved
src/pages/Search/SearchAdvancedFiltersPage/SearchFiltersCardPage.tsx
Outdated
Show resolved
Hide resolved
src/pages/Search/SearchAdvancedFiltersPage/SearchFiltersCardPage.tsx
Outdated
Show resolved
Hide resolved
Fixed your comments @rayane-djouah |
bank, | ||
correspondingCardIDs, | ||
iconStyles, | ||
cardFeedLabel: isBankRepeating ? domainName : undefined, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will cause the card feed label to be displayed in expensify-policy${policyID}.exfy
default format for workspaces without a custom domain.
We need to display a human-readable name for Expensify policy domain names similar to what we already do here:
App/src/pages/settings/Wallet/PaymentMethodList.tsx
Lines 217 to 225 in 653a06b
const getDescriptionForPolicyDomainCard = (domainName: string): string => { | |
// A domain name containing a policyID indicates that this is a workspace feed | |
const policyID = domainName.match(CONST.REGEX.EXPENSIFY_POLICY_DOMAIN_NAME)?.[1]; | |
if (policyID) { | |
const policy = PolicyUtils.getPolicy(policyID.toUpperCase()); | |
return policy?.name ?? domainName; | |
} | |
return domainName; | |
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've applied your suggestion but have no domain cards to test it. When you test this Pr again please look at this part.
const text = card.bank === CONST.EXPENSIFY_CARD.BANK ? card.bank : cardName; | ||
|
||
return { | ||
lastFourPAN: card.lastFourPAN, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Physical cards that have not yet been issued lack a lastFourPAN
and are uniformly labeled as Expensify Card
:
I think we should consider either modifying how these cards are displayed or not showing the unissued cards (that do not have a lastFourPAN
) at all.
const cardToBeIssued = domainCards.find((card) => !card?.nameValuePairs?.isVirtual && card?.state === CONST.EXPENSIFY_CARD.STATE.STATE_NOT_ISSUED); |
@luacmartins, what are your thoughts on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose you cannot pay with a unissued card, so there's no point in searching report with them. We could just filter them out. WDYT @luacmartins
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. Let's filter out unissued cards. Nice catch!
// When user is admin of a workspace he sees all the cards of workspace under cards_ Onyx key | ||
const allWorkspaceCards: CardFilterItem[] = Object.values(workspaceCardFeeds) | ||
.filter((cardFeed) => !isEmptyObject(cardFeed)) | ||
.flatMap((cardFeed) => { | ||
return Object.values(cardFeed as Record<string, Card>) | ||
.filter((card) => card && CardUtils.isCard(card) && !userCardList?.[card.cardID]) | ||
.map((card) => createIndividualCardFilterItem(card, selectedCards, iconStyles)); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The admin of a workspace currently cannot differentiate between cards assigned to them and those assigned to other members:
Before | After |
---|---|
@Expensify/design, @luacmartins, should we consider modifying the card list item to address this issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooo I could definitely be into something like that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you link the whole figma for that design? Also does that mean that we should grup cards in Card Filter by user? @shawnborton
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My thinking is that this would work much like Workspace avatars do...
The workspace itself just uses a single avatar. In this case, the feed itself uses a single avatar and that's the big card logo.
But then individual users/cards within the feed get the stacked avatar + card treatment like I'm showing above.
Before we commit to that, let's see what @JmillsExpensify @trjExpensify @Expensify/design @joekaufmanexpensify thinks though. I think it could help solve this issue though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not really sure how the whole card filter should look like though. Would you give me the link to this Figma? Previously the desing didn't contain any groupings like your screenshot does. Or maybe this screenshot comes from another desing and I should only add these avatars to card filter? @shawnborton
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking it would look something like this:
So the main card feed is an option at the very top. Then below we have individual cards. That's just like you are doing above. But the only difference is how we display the avatar and card name. Thoughts? cc @JmillsExpensify @joekaufmanexpensify @Expensify/design
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally I'm super into that. We get ALL the info in there and it looks really nice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great to me! This change would apply to third party cards too, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great to me as well! Thanks for clarification
data: cards, | ||
shouldShow: cards.length > 0, | ||
title: translate('search.filters.card.individualCards'), | ||
data: individualCardsSectionData.filter((item) => item.text?.toLocaleLowerCase().includes(debouncedSearchTerm.toLocaleLowerCase())), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it necessary to include fields other than item.text
in the search, such as item.lastFourPAN
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be a good idea to allow for lastFourPAN
search too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BUG: When a feed list item is selected, the list automatically scrolls down to the individual cards. Conversely, selecting an individual card causes the list to automatically scroll up to the corresponding feed list item:
Screen.Recording.2024-12-19.at.9.28.06.PM.mov
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This behaviour is hard coded into SelectionList. Actually the way I handled selected items was not consistent with the rest. Now when I put selected items on top of SelectionList like we do everywhere else, this behaviour is correct and takes us to selected items section.
}, | ||
}; | ||
}); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SzymczakJ I'm testing with the mock data below. Is the data correct, or am I missing something?
Code
Onyx.merge('cardList', {
'21588678': {
accountID: 1,
bank: 'Expensify Card',
cardID: 21588678,
cardName: '455594XXXXXX1138',
domainName: 'expensify-005AA2EAC6D4E280.exfy',
lastFourPAN: '1138',
},
'21588684': {
accountID: 1,
bank: 'Expensify Card',
cardID: 21588684,
cardName: '',
domainName: 'expensify-FD17559D0E7ED371.exfy',
lastFourPAN: '',
},
'21589202': {
accountID: 1,
bank: 'Expensify Card',
cardID: 21589202,
cardName: '455594XXXXXX6232',
domainName: 'expensify-FD17559D0E7ED371.exfy',
lastFourPAN: '6232',
},
'21604933': {
accountID: 1,
bank: 'vcf',
cardID: 21604933,
cardName: '480801XXXXXX1601',
domainName: 'expensify-005AA2EAC6D4E280.exfy',
lastFourPAN: '1601',
},
'11111111': {
accountID: 1,
bank: 'Expensify Card',
cardID: 11111111,
cardName: '455594XXXXXX1139',
domainName: 'testDomain.com',
lastFourPAN: '1139',
},
'234523452345': {
cardID: 234523452345,
state: 3,
bank: 'Expensify Card',
availableSpend: 10000,
domainName: 'expensify.com',
lastFourPAN: '2345',
isVirtual: true,
cardholderFirstName: 'Test',
cardholderLastName: 'Test',
},
'234523452346': {
cardID: 234523452346,
state: 4,
bank: '1000',
availableSpend: 10000,
domainName: 'expensify.com',
lastFourPAN: '2346',
isVirtual: false,
},
});
Onyx.merge(`cards_18680694_vcf`, {
'21593492': {
accountID: 17822766,
bank: 'vcf',
cardID: 21593492,
cardName: '480801XXXXXX9411',
domainName: 'expensify-7BFA00039DEB0D94.exfy',
lastFourPAN: '9411',
},
'21604933': {
accountID: 17822766,
bank: 'vcf',
cardID: 21604933,
cardName: '480801XXXXXX1601',
domainName: 'expensify-FD17559D0E7ED371.exfy',
lastFourPAN: '1601',
},
'21638320': {
accountID: 17822777,
bank: 'vcf',
cardID: 21638320,
cardName: '480801XXXXXX2617',
domainName: 'expensify-7BFA00039DEB0D94.exfy',
lastFourPAN: '2617',
},
'21638598': {
accountID: 14826403,
bank: 'vcf',
cardID: 21638598,
cardName: '480801XXXXXX2111',
domainName: 'expensify-FD17559D0E7ED371.exfy',
lastFourPAN: '2111',
},
cardList: {
test: '231:1111111',
},
});
Onyx.merge('cards_18465979_Expensify Card', {
'21588678': {
accountID: 17861101,
bank: 'Expensify Card',
cardID: 21588678,
cardName: '455594XXXXXX1134',
domainName: 'expensify-005AA2EAC6D4E280.exfy',
lastFourPAN: '1134',
},
'21588684': {
accountID: 18118340,
bank: 'Expensify Card',
cardID: 21588684,
cardName: '',
domainName: 'expensify-005AA2EAC6D4E280.exfy',
lastFourPAN: '',
},
});
Onyx.merge('cards_18643109_Expensify Card', {
'21589168': {
accountID: 17822777,
bank: 'Expensify Card',
cardID: 21589168,
cardName: '455594XXXXXX4163',
domainName: 'expensify-7BFA00039DEB0D94.exfy',
lastFourPAN: '4163',
},
'21589182': {
accountID: 17822780,
bank: 'Expensify Card',
cardID: 21589182,
cardName: '',
domainName: 'expensify-7BFA00039DEB0D94.exfy',
lastFourPAN: '',
},
'21589202': {
accountID: 17822780,
bank: 'Expensify Card',
cardID: 21589202,
cardName: '455594XXXXXX6232',
domainName: 'expensify-7BFA00039DEB0D94.exfy',
lastFourPAN: '6232',
},
'21638322': {
accountID: 14826403,
bank: 'Expensify Card',
cardID: 21638322,
cardName: '',
domainName: 'expensify-7BFA00039DEB0D94.exfy',
lastFourPAN: '',
},
});
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I limited the mocks to have only what we need, Card objects are enormous and if I wanted to include all the data, that BE gives us, then These mocks would take 2.5x more code lines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work so far! We have a few pending items to address from the last review but looking good otherwise
There are merge conflicts that need to be resolved |
Let us know if there are new screenshots to review. |
Here is the current state(had to cut video in two because it's too big for GH): cards1480.movcards2480.movI would like to suggest one change to your previous screenshot @shawnborton. Instead of putting "bankName* - lastFourPan - Virtual(optional)" in subtitle of card item, we should replace bankName with cardName. This way we show card name to the user which in many times is meaningful. WDYT |
Explanation of Change
This PR adds Card feeds secion to Card filter on Search page, which is only visible to workspace admin. With this PR admin will be able to bulk select cards from the same feed.
PR also makes workspace admins see all the cards linked to workspace(taken from
cards_
Onyx key).Card feeds secion is built from workspace feeds(
cards_
Onyx key) and domain feeds which are separeted from cardList object(cardList
Onyx key), because right now New Dot doesn't fully support domain cards and it's the only Onyx object that contains it. Admin cannot see all domain cards yet.Fixed Issues
$ #53231
PROPOSAL: N/A
Tests
Offline tests
QA Steps
Same as tests. Please make sure domain feeds are tested well as I didn't have access to any domain feed.
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
android.mov
Android: mWeb Chrome
androidweb.mov
iOS: Native
ios.mov
iOS: mWeb Safari
ioweb.mov
MacOS: Chrome / Safari
web.mov
MacOS: Desktop
desktop.mov