-
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 view transactions cta to Expensify card and company cards page #55095
Add view transactions cta to Expensify card and company cards page #55095
Conversation
<MenuItem | ||
icon={Expensicons.MoneySearch} | ||
title={translate('workspace.common.viewTransactions')} | ||
style={styles.mv1} |
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.
All of these menu items should not have a gap in between them, so I think we need to remove the vertical margin here. Can you make sure all of the menu items are 64px tall with no gaps between? Screenshots would be great, thanks! cc @Expensify/design for viz
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.
Doesn't quite look right. We want a gap of 12px below the last push row and above the block of optionRows. They should appear like two distinct sections. It's just that we want all of the option rows in the same block to not have any vertical margin.
For the option rows on your second screenshot, why do some have a green icon and others a gray icon? I think we want them all to be gray.
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 just noticed that the color of icons is different for update card cta. Normally these links will be green on hovering(active state). What should the color be here on default state?
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.
Done.
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.
Lovely, thanks!
@allgandalf 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] |
@@ -2652,6 +2652,7 @@ const translations = { | |||
planType: 'Tipo de plan', | |||
submitExpense: 'Envíe los gastos utilizando el chat de su espacio de trabajo:', | |||
defaultCategory: 'Categoría predeterminada', | |||
viewTransactions: 'Ver transacciones', |
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.
Did we confirm this translation ?
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.
Yes.
const queryString = policyID | ||
? `type:${type} status:${Array.isArray(status) ? status.join(',') : status} policyID:${policyID}` | ||
: `type:${type} status:${Array.isArray(status) ? status.join(',') : status}`; | ||
let queryString = `type:${type} status:${Array.isArray(status) ? status.join(',') : status}`; |
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.
did you try to mock some transaction data and see if we get the search results ?
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 is the same as before. I haven't tested this part. Did you notice any issues?
Also please fix failing tests |
Seems like there is still some feedback to address here |
I think the design feedback has been taken care of, so we're good there. We can also run a test build when we are close to merge to make sure it's looking good. |
I have made all requested changes and retested. |
🚧 @mountiny has triggered a test build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
almost done with testing, gonna mock some onyx data for card transactions and then approve this one |
One thing worth mentioning is that the card filter does not work for the cards that are owned by someone else. Even though I can see the card in my list when I the admin of a workspace but the search page does not show card filters for it. |
I don't feel strongly either but agree it would make sense. Can't think of a reason not to add it here other than it's more work. |
Yes I think we should |
Changes done. Its ready. |
Nice, let us know when we have updated screenshots/videos to review as well. We need them from Workspace Editor as well as Wallet > Cards |
@allgandalf This is ready for testing. |
I will share screenshots shortly. @allgandalf Can you please start testing this in the meantime? |
Closed mistakenly... |
🚧 @mountiny has triggered a test build. You can view the workflow run here. |
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪
|
Vids for web and native for wallet page. Others are attached to OP. cc: @shawnborton 22.01.2025_02.39.11_REC.mp422.01.2025_02.31.45_REC.mp4 |
Seems to test well for me! Screen.Recording.2025-01-21.at.23.38.29.mp4 |
@allgandalf how is it looking? |
Looks great! |
Reviewer Checklist
Screenshots/VideosAndroid: NativeScreen.Recording.2025-01-22.at.12.40.12.PM.movAndroid: mWeb ChromeScreen.Recording.2025-01-22.at.12.42.16.PM.moviOS: NativeScreen.Recording.2025-01-22.at.12.31.27.PM.moviOS: mWeb SafariScreen.Recording.2025-01-22.at.12.33.03.PM.movMacOS: Chrome / SafariScreen.Recording.2025-01-22.at.12.19.57.PM.movMacOS: DesktopScreen.Recording.2025-01-22.at.12.24.07.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.
Code LGTM and tests well
@parasharrajat can you please resolve the conflicts |
@mountiny Done,. |
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.
Thanks @parasharrajat
✋ 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/mountiny in version: 9.0.89-0 🚀
|
🚀 Deployed to production by https://github.com/yuwenmemon in version: 9.0.89-8 🚀
|
Explanation of Change
Fixed Issues
$ #54800
PROPOSAL: #54800 (comment)
Tests
Prerequisite:
Expensify Card
Company Card
Assigned Cards
Offline tests
QA Steps
Same as tests
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
15.01.2025_14.13.10_REC.mp4
Android: mWeb Chrome
15.01.2025_14.19.33_REC.mp4
iOS: Native
15.01.2025_14.57.38_REC.mp4
22.01.2025_02.39.11_REC.mp4
iOS: mWeb Safari
15.01.2025_14.16.48_REC.mp4
MacOS: Chrome / Safari
15.01.2025_14.11.28_REC.mp4
22.01.2025_02.31.45_REC.mp4
MacOS: Desktop
15.01.2025_14.12.33_REC.mp4