Eliran/update card view#73090
Conversation
Codecov Report❌ Patch coverage is
... and 1565 files with indirect coverage changes 🚀 New features to boost your workflow:
|
|
@DylanDylann 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] |
|
The change looks straighforward. However, we currently cannot assign a card to unvalidated users. @elirangoshen I am not sure how to test this PR.
|
Reviewer Checklist
Screenshots/VideosiOS: HybridApp |
|
Maybe @madmax330 can assist ? as I see he opened the issue. |
|
THanks @elirangoshen @DylanDylann |
fabioh8010
left a comment
There was a problem hiding this comment.
LGTM from the code perspective
|
Hi @madmax330 , |
|
Yeah, the backend PR was merged, should be deployed today |
|
@elirangoshen the backend PR for expensify cards was deployed, so we should be able to test this now with a test expensify card feed |
Thanks ! Great news :) |
|
@elirangoshen Please update to display the pending status on ExpensifyCard too |
|
@madmax330 Should we update to allow assigning a company card to unvalidated users?
|
Yes, that PR should be deployed today |
This is deployed now, you should be able to test both expensify cards and third party cards |
|
@elirangoshen please let me know when you update the pending status on the Expensify Card |
|
Hey, I noticed you changed If you want to automatically generate translations for other locales, an Expensify employee will have to:
Alternatively, if you are an external contributor, you can run the translation script locally with your own OpenAI API key. To learn more, try running: npx ts-node ./scripts/generateTranslations.ts --helpTypically, you'd want to translate only what you changed by running |
ed64643 to
8e40a54
Compare
I updated and now tested and its working, I also fixed lint error related o use of depracated function |
| @@ -87,7 +89,7 @@ function WorkspaceExpensifyCardDetailsPage({route}: WorkspaceExpensifyCardDetail | |||
| const deactivateCard = () => { | |||
| setIsDeactivateModalVisible(false); | |||
| shouldGoBack.current = true; | |||
| InteractionManager.runAfterInteractions(() => { | |||
| requestAnimationFrame(() => { | |||
There was a problem hiding this comment.
due to eslint error of deprecation of InteractionManager
There was a problem hiding this comment.
@elirangoshen Is there any guideline confirming that we can use requestAnimationFrame instead of InteractionManager.runAfterInteractions?
There was a problem hiding this comment.
I think we can safely add
// eslint-disable-next-line @typescript-eslint/no-deprecated
and we will handle this deprecation in the deprecation epic
|
@elirangoshen I think it shouldn't be truncated, we should display it fully cc @madmax330
|
Yeah I agree |
Ok I fixed it |
|
@elirangoshen One comment: #73090 (comment) |
|
thanks for the update, Done |
|
thanks for the approve @DylanDylann , @madmax330 what do you think ? |
|
✋ 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/madmax330 in version: 9.2.44-0 🚀
|
|
🚀 Deployed to production by https://github.com/luacmartins in version: 9.2.44-5 🚀
|








Explanation of Change
Adding icon and text to card view for unvaldiated card holder user
Fixed Issues
$ #72765
PROPOSAL:
Tests
Offline tests
QA Steps
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
MacOS: Desktop