-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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 full screen engagement modal to NewDot #32154
Conversation
…ame place for both mobile and web
@situchan can you check the reviewer checklist to see why it is failing? Looks like maybe a few more checkboxes have been added. |
It was caused by old checklist which should be removed |
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 think this is expected.
If signup with deeplink (i.e. /settings/preferences), no engagement modal opens. It shows Preferences page directly
Screen.Recording.2024-01-23.at.3.11.21.AM.mov
const navigationState = navigation.getState(); | ||
const routes = navigationState.routes; | ||
const currentRoute = routes[navigationState.index]; | ||
if (currentRoute && NAVIGATORS.CENTRAL_PANE_NAVIGATOR !== currentRoute.name && currentRoute.name !== SCREENS.HOME) { |
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 please add context behind this logic?
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.
If there is a route, we don't want to show the modal unless the page we're on is home or the central pane navigator
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.
ok so #32154 (review) is correct. Just wanted to confirm 🙂
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.
Yep that's correct! Thanks for double checking :)
@youssef-lr 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] |
My reviewer checklist is ignored because of old one |
Deleted it (sorry @bondydaa - for posterity, Bondy did a great job with the checklist 💪) |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
Thank you all for all of the reviews! I'm shipping this as is, I don't think that the minor style issues are worth delaying this any further given we wanted to get this out a long time ago! |
🚀 Deployed to production by https://github.com/thienlnam in version: 1.4.31-7 🚀
|
if (!Policy.isAdminOfFreePolicy(allPolicies ?? undefined) && !isDisplayingWorkspaceRoute) { | ||
showCreateMenu(); | ||
// we will show the engagement modal. | ||
if (!Policy.isAdminOfFreePolicy(allPolicies ?? undefined) && !isExitingToWorkspaceRoute && !hasSelectedChoice && !hasDismissedModal && Object.keys(allPolicies ?? {}).length === 1) { |
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.
@stitesExpensify Why do we need to check Object.keys(allPolicies ?? {}).length === 1
in here?
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.
Pretty sure it's because everyone should at least have a personal policy. So if you have some other policy on top of that then we're not trying to engage you as a new user.
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
Details
Creates a new Engagement Modal that pops up for new users on the site, and sends concierge messages related to their choices
Fixed Issues
$ https://github.com/Expensify/Expensify/issues/335513
$ #33324
PROPOSAL: n/a
Tests
New Account
Existing Account
Completed on OldDot
Closed on OldDot
x
in the top rightClosed on NewDot
x
on the modalOffline tests
QA Steps
Same as Tests
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.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 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: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop