-
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 Expensify Card promoting banner to Company cards page #56561
base: main
Are you sure you want to change the base?
Add Expensify Card promoting banner to Company cards page #56561
Conversation
♻ Here's something to sort out before we merge this: currently I am using Onyx to store a boolean value locally based on which I decide to show / hide the banner - this value is set to ⚠ Issue with this is:
I. Since this
II. With this local Onyx variable there's one more thing to consider: when the user will press Settings > Trubleshooting >
|
Nice! The videos look pretty good to me 👍 |
This looks great. Can I ask what the back stack is? If you click back after clicking |
Yeah I kinda think we should theme it cause it's VERY bright in dark mode there. I like that Danny! |
@dubielzyk-expensify Correct, that's how it works on all platforms - seems logical to me in terms of navigation flow. Screen.Recording.2025-02-07.at.15.20.45.mov
I'm sure it can be done by replacing the |
Super down with that! |
@Expensify/design Does this look good ? I noticed that the title / subtitle text color in the issue's OP (this design) is a dark green, whereas with what you can see below: I used already existing styles that we use in the |
I agree with that! |
W.r.t @ikevin127’s comment here, the OP does not specify whether we remove the banner after a user clicks on “Learn more” once. We keep the banner on the Company Cards page even after the user clicks on “Learn more,” right? |
src/libs/PolicyUtils.ts
Outdated
@@ -1080,6 +1081,13 @@ function goBackWhenEnableFeature(policyID: string) { | |||
}, CONST.WORKSPACE_ENABLE_FEATURE_REDIRECT_DELAY); | |||
} | |||
|
|||
function navigateToExpensifyCardPage(policyID: string) { | |||
setTimeout(() => { |
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.
setTimeout(() => { |
I think we need not use setTimeout
here. It looks like we are using setTimeout
for navigating the user back to workspace initial page with goBackWhenEnableFeature
only for narrow layouts after enabling a feature. setTimeout
does not seem to serve any purpose here.
src/libs/actions/Policy/Policy.ts
Outdated
@@ -2896,6 +2904,12 @@ function enableExpensifyCard(policyID: string, enabled: boolean) { | |||
|
|||
API.write(WRITE_COMMANDS.ENABLE_POLICY_EXPENSIFY_CARDS, parameters, onyxData); | |||
|
|||
if (enabled && shouldNavigateToExpensifyCardPage) { | |||
navigateToExpensifyCardPage(policyID); | |||
setHasSeenExpensifyCardPromotionBanner(); |
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.
setHasSeenExpensifyCardPromotionBanner(); |
We are already calling setHasSeenExpensifyCardPromotionBanner
in navigateToExpensifyCardPage
.
src/libs/actions/Policy/Policy.ts
Outdated
@@ -2849,7 +2850,14 @@ function savePreferredExportMethod(policyID: string, exportMethod: ReportExportT | |||
Onyx.merge(`${ONYXKEYS.LAST_EXPORT_METHOD}`, {[policyID]: exportMethod}); | |||
} | |||
|
|||
function enableExpensifyCard(policyID: string, enabled: boolean) { | |||
/** |
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.
We can omit these JSDoc comments that are redundant with TypeScript.
CompanyCards.clearAddNewCardFlow(); | ||
Navigation.navigate(ROUTES.WORKSPACE_COMPANY_CARDS_ADD_NEW.getRoute(policy?.id ?? '-1')); | ||
clearAddNewCardFlow(); | ||
Navigation.navigate(ROUTES.WORKSPACE_COMPANY_CARDS_ADD_NEW.getRoute(policy?.id ?? String(CONST.DEFAULT_NUMBER_ID))); |
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.
Do not default string IDs to any value.
@@ -1080,6 +1081,13 @@ function goBackWhenEnableFeature(policyID: string) { | |||
}, CONST.WORKSPACE_ENABLE_FEATURE_REDIRECT_DELAY); | |||
} | |||
|
|||
function navigateToExpensifyCardPage(policyID: string) { | |||
setTimeout(() => { | |||
Navigation.navigate(ROUTES.WORKSPACE_EXPENSIFY_CARD.getRoute(policyID)); |
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.
Navigation.navigate(ROUTES.WORKSPACE_EXPENSIFY_CARD.getRoute(policyID)); | |
Navigation.setNavigationActionToMicrotaskQueue(() => Navigation.navigate(ROUTES.WORKSPACE_EXPENSIFY_CARD.getRoute(policyID))); | |
If this is added to prevent the brief “not found” page because the navigation action might have occurred before the Onyx update enabling the Expensify Cards page, we can add the navigation task to the microtask queue to ensure it happens only after the Onyx update using setNavigationActionToMicrotaskQueue
and avoid using setTimeout
.
@Expensify/design Addressed the latest text style change request, here's how it looks now: @c3024 Addressed all code related change requests (👍), all that's left to decide regarding the logic is what I asked in #56561 (comment) and you reiterated in #56561 (comment). |
Right, I dont think we need this logic, we should show the banner all the time until they add a card feed - then the ui changes and we would not promote for the card with this pattern (we could add something more later but that is not in scope of this project) |
…7-expensifyCardPromotion
✅ Done, makes total sense - thanks for jumping in over the weekend 🚀
@c3024 PR is 💯% ready for review! |
That looks løvely! Thank you! |
Explanation of Change
We want to make sure we promote the Expensify Card to customers who have the company cards or seeking this setup, this PR implements a promotional banner to the Company cards page such that:
Learn more
button we should take them to Expensify Card pageFixed Issues
$ #56485
PROPOSAL: #56485 (comment)
Tests
Precondition: Login and create a new workspace.
Test A (Expensify Card - Disabled)
Company cards
.Company cards
and notice the newExpensify Card
promoting banner.Learn more
button.Expensify Card
feature and navigate the user to it.Company cards
page and verify that the banner doesn't show up anymore.Test B (Expensify Card - Enabled)
Company cards
andExpensify Card
features.Company cards
and notice the newExpensify Card
promoting banner.Learn more
button.Expensify Card
feature page.Company cards
page and verify that the banner doesn't show up anymore.Offline tests
Same as 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
android.webm
Android: mWeb Chrome
android-mweb.webm
iOS: Native
ios.mp4
iOS: mWeb Safari
ios-mweb.mp4
MacOS: Chrome / Safari
web.mov
MacOS: Desktop
desktop.mov