-
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
feat: add additional full screen engagement modal to NewDot #35019
feat: add additional full screen engagement modal to NewDot #35019
Conversation
Reviewer Checklist
Screenshots/VideosMacOS: Chrome / SafariMacOS: Desktop |
Going to do a quick run through on the testing and should merge by EOD |
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪 |
@marcaaron how do we reconcile this? do we make that fix here, or wait for the bug to be fixed. |
The proposed fix looks pretty simple we could try to incorporate it here and just close that other PR if it solves the issue. |
Everything works for me with that line applied actually. I'm gonna propose we fold it into this PR so we can ship this. |
@teneeto add these changes and I can approve + merge. diff --git a/src/libs/Navigation/AppNavigator/createCustomBottomTabNavigator/BottomTabBar.tsx b/src/libs/Navigation/AppNavigator/createCustomBottomTabNavigator/BottomTabBar.tsx
index 3e77433bd0..a953175fdc 100644
--- a/src/libs/Navigation/AppNavigator/createCustomBottomTabNavigator/BottomTabBar.tsx
+++ b/src/libs/Navigation/AppNavigator/createCustomBottomTabNavigator/BottomTabBar.tsx
@@ -43,7 +43,7 @@ function BottomTabBar({isLoadingApp = false}: PurposeForUsingExpensifyModalProps
const routes = navigationState.routes;
const currentRoute = routes[navigationState.index];
- if (currentRoute && NAVIGATORS.BOTTOM_TAB_NAVIGATOR !== currentRoute.name) {
+ if (currentRoute && currentRoute.name !== NAVIGATORS.BOTTOM_TAB_NAVIGATOR && currentRoute.name !== NAVIGATORS.CENTRAL_PANE_NAVIGATOR) {
return;
} |
…dditional-screens-for-engagement-modal
dd69810
@marcaaron Done ✅ Thanks for the intervention too. |
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪 |
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.
Retested on all platforms and it's working and looking great.
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
@@ -109,7 +110,6 @@ import QuestionMark from '@assets/images/question-mark-circle.svg'; | |||
import ReceiptSearch from '@assets/images/receipt-search.svg'; | |||
import Receipt from '@assets/images/receipt.svg'; | |||
import Rotate from '@assets/images/rotate-image.svg'; | |||
import RotateLeft from '@assets/images/rotate-left.svg'; |
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.
@teneeto Is there a reason why this was removed?
This created a regression here
@marcaaron If you agree, can we handle this in #35663?
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.
@ shubham1206agra, I omitted it in a merge conflict. Is there an issue for it I can add it ASAP.
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.
Actually, I was thinking of doing it in #35663 since the actual PR was migrated 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.
Ok, that's brilliant too. whichever way is best, I'm here to assist when you need me. cc @marcaaron
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.
Ah good catch, yes let's please fix these regressions as quickly as we can. Thanks!
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.
Follow up PR #36144
🚀 Deployed to staging by https://github.com/marcaaron in version: 1.4.39-0 🚀
|
🚀 Deployed to production by https://github.com/Beamanator in version: 1.4.39-8 🚀
|
Details
Adds additional screens - new Engagement Modal that pops up for new users on the site, and sends concierge messages related to their choices.
Fixed Issues
$ #34468
PROPOSAL: issue-2079898524
Also fixes: #35663
Tests
New Account
Existing Account
Completed on OldDot
Closed on OldDot
Closed on NewDot
Offline tests
QA Steps
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
Screen.Recording.2024-02-01.at.17.46.25.mov
Android: mWeb Chrome
Screen.Recording.2024-02-05.at.10.20.43.mov
iOS: Native
Screen.Recording.2024-01-31.at.16.21.36.mov
iOS: mWeb Safari
Screen.Recording.2024-01-31.at.16.56.33.mov
MacOS: Chrome / Safari
Screen.Recording.2024-01-31.at.16.04.14.mov
MacOS: Desktop
Screen.Recording.2024-01-31.at.17.01.34.mov