-
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
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
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 |
Good question. I would be curious for @joekaufmanexpensify 's thoughts here, but I think the main information we want to communicate is:
Does that sound right? Or do we not need to include all of that information? Let's first figure out the requirements and then we can figure out the best way to display it. cc @Expensify/design too for viz |
I don't have much to add here but I definitely agree with this approach. |
IMO the non-negotiables are card assignee and last 4. As those are what you definitively need to identify it. Everything else feels like a nice to have, but if we can fit it all, that sounds great. If we needed to cut something, I'd vote for type of card program (because that is also clear with the card icon), or card nickname. Between those two, I'd lean towards cutting type of card program, also because it is already represented with the card icon. Card nickname only really becomes useful when one employee has multiple cards, which I am not convinced will be the mainline case. But in that case it will be really helpful. |
Looks good to me. I assume we'd just omit the card type at the end for third party cards? |
That makes sense to me, yup! |
I've changed UI to match the new design: Screen.Recording.2025-01-09.at.17.51.39.mov@rayane-djouah I think we've finished this design discussion, so the PR is ready for your another review. I'll fix eslint tomorrow as I've ran out of time today. |
That is odd. Third party cards do have a "nickname", it is Third party card names are a bit wonky dating back to their original implementation in OldDot, and we plan to refactor them, which could also be part of the issue. |
So maybe we don't use it then, and only add |
Yep, that's fine with me. Card name for custom feeds will get cleaned up here, so yeah just not showing them for now works. |
Final version: Bumping @rayane-djouah 🙏 |
Looks good to me! |
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