-
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
Mandatory exit survey for users going back to OldDot #34925
Conversation
# Conflicts: # src/CONST.ts # src/ONYXKEYS.ts
# Conflicts: # src/ONYXKEYS.ts
Regarding the bug I found, I agree it shouldn't be a blocker and there's maybe a big chance that happens because of the ideal-nav project. |
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.
LGTM
@roryabraham I'll leave it to you to merge since we're kind of in a merge/deploy freeze until Friday |
# Conflicts: # package-lock.json # package.json # src/ONYXKEYS.ts # src/libs/API/parameters/index.ts # src/libs/API/types.ts # src/libs/Navigation/linkingConfig/config.ts
conflicts resolved |
TS failing on main, PR to fix it: #37171 |
✋ 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/roryabraham in version: 1.4.44-0 🚀
|
🚀 Deployed to production by https://github.com/puneetlath in version: 1.4.44-13 🚀
|
onInputChange?: (value: string) => void; | ||
|
||
/** The checked value, if you're using this component as a controlled input. */ | ||
value?: string; |
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.
success | ||
text={translate('exitSurvey.goToExpensifyClassic')} | ||
onPress={() => { | ||
ExitSurvey.switchToOldDot(); |
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.
Coming from #44926 (BZ Checklist)
we should also close the RHP after switching to Olddot
const formMaxHeight = Math.floor( | ||
windowHeight - | ||
keyboardHeight - | ||
safeAreaInsetsTop - | ||
// Minus the height of HeaderWithBackButton | ||
variables.contentHeaderHeight - | ||
// Minus the top margins on the form | ||
formTopMarginsStyle.marginTop, | ||
); |
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.
Coming from #47280 (BZ checklist), The formMaxHeight
calculation is not correct for Android native, the window height on Android doesn't take the inset top height into account, so subtracting it from the inset top will make it smaller, we need to add StatusBar.currentHeight
to window height. More info in this proposal: #47280 (comment)
)} | ||
</View> | ||
<FixedFooter> | ||
<Button |
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.
Coming from #51145 (BZ Checklist)
We should add pressOnEnter to maintain consistency with the previous step, where pressing enter submits to the next step.
Details
This PR introduces a mandatory exit survey to gather feedback from users going from NewDot -> OldDot.
Fixed Issues
$ https://github.com/Expensify/Expensify/issues/358172
Tests
Press on your avatar in the LHN to open the account settings page.
Click on
Go to Expensify Classic
Verify that you see a page titled
Before you go
and has three radio buttons with the reasons:Verify all options of the radio buttons can be selected, and that only one can be selected at a time.
Proceed with
I need a feature that's only available in Expensify Classic
selected.Verify that the next page shows a text input with the title
What feature do you need that isn't available in New Expensify?
Try to proceed without composing a response. Verify that you see an error saying that the response is required.
Compose a response. Verify that it can be multiple lines, not just one.
Press back, then Next. Verify that your response was saved.
Refresh the page, verify your response was saved.
press back. Verify that you are taken to the reason page with the radio buttons.
press Next and verify your response was still there.
(you only need to do this step exactly once) add a bunch of newlines and verify that the response expands to fill the page, but doesn't interfere with the next button or push it off the page.
Press Next to submit the form. Verify you are taken to a confirmation page that looks like this:
The copy should be:
(web/desktop only) Press back, and verify your response was saved. Edit the response, then press CMD+Enter (macos) or
CTRL+Enter
(windows). Verify that the form submits and you are taken to the confirmation page again.Press
Go to Expensify Classic
and verify that you are taken to the OldDot inbox page. Note that OldDot web doesn't really work well on mobile, and hybrid web isn't really in place yet. The purpose of this PR is just to test the survey – the connection between NewDot and OldDot has not been changed at all.Open OldDot web in the same account on a desktop or laptop computer, then open the JS console and run
NVP.get('tryNewDot')
. Verify that under theclassicRedirect->dismissedReason
key you see the reason you selected in the first page of the form and the response you entered in the second.Repeat steps 1-17 for the other two options in the response page. Expected copy
Switch to Spanish, and repeat steps 1-18 in Spanish. Expected copy:
The expected copy for the confirmation page will be:
Offline tests
Go offline.
Verify the UI updates to show this (note center-aligned text):
Verify that the
Next
button is disabled.Go back online
Verify that the form restores to the state it was in before you go offline.
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
android.mp4
Android: mWeb Chrome
android.mp4
iOS: Native
ios.mov
iOS: mWeb Safari
iosWeb.mov
MacOS: Chrome / Safari
web.mov
MacOS: Desktop
desktop720.mov