-
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
Improve Switch to Expensify Classic flow #52526
Conversation
@shubham1206agra 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] |
Case 1: Screen.Recording.2024-11-14.at.15.31.48.movCase 2: Screen.Recording.2024-11-14.at.14.15.10.mov |
src/languages/es.ts
Outdated
quickTip: 'Consejo rápido...', | ||
quickTipSubTitle: 'Puedes ir directamente a Expensify Classic visitando expensify.com. Márcalo como favorito para tener un acceso directo fácil.', | ||
bookACall: 'Reservar una llamada', | ||
noThanks: 'No, gracias', | ||
bookACallTitle: '¿Desea hablar con un responsable de producto?', | ||
benefits: { | ||
[CONST.EXIT_SURVEY.BENEFIT.CHATTING_DIRECTLY]: 'Charla directa sobre gastos e informes', | ||
[CONST.EXIT_SURVEY.BENEFIT.EVERYTHING_MOBILE]: 'Posibilidad de hacerlo todo desde el móvil', | ||
[CONST.EXIT_SURVEY.BENEFIT.TRAVEL_EXPENSE]: 'Viajes y gastos a la velocidad del chat', | ||
}, | ||
bookACallTextTop: 'Al cambiar a Expensify Classic, se perderá:', | ||
bookACallTextBottom: 'Nos encantaría hablar con usted para entender por qué. Puede concertar una llamada con uno de nuestros jefes de producto para hablar de sus necesidades.', |
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.
Requested translations here:
https://expensify.slack.com/archives/C01GTK53T8Q/p1731572881369449
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.
Asked @Gonals to take a look to keep this moving!
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.
Translation for "Take me to Expensify Classic" === Llévame a Expensify Classic
cc @trjExpensify for eyes on the review too, but I think this looks pretty good from a design perspective. |
Buildin'... |
@hungvu193 can we make sure the button label on the quickTip page reads We purposely had a slightly different label name for that, so we can set up tracking it separately in FullStory. |
Sure. I think I missed that case, will update it along with the new translations. |
Great, I made the suggestion on the one translation change. @shubham1206agra is this a PR you'll be able to review today? If not, please do let me know so I can find a replacement. :) |
This comment has been minimized.
This comment has been minimized.
Alright, did some testing. Overall looking good!
2024-11-14_13-17-51.mp4
2024-11-14_13-24-26.mp4
2024-11-14_13-06-31.mp4So two things on that bug:
|
@hungvu193 Please fix the tests. |
|
@trjExpensify Can you try to check the Onyx key |
|
I've figured it out.
|
🧪🧪 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.
Code looks good ot me, just waiting for the checklist
Screen.Recording.2024-11-14.at.11.11.47.PM.movBUG: Page switches in background when clicked no thanks |
Screen.Recording.2024-11-14.at.11.13.28.PM.movBUG: Navigation gets broken after clicking no thanks. |
Screen.Recording.2024-11-14.at.11.15.09.PM.movBUG: Modal gets closed on clicking |
This one is expected. We talked about it with design in the thread linked in the issue OP. |
This one also happens on staging Screen.Recording.2024-11-15.at.09.52.17.mov |
I don't think this is related to this PR 😄 |
Fixed the navigation issue. Please test again @shubham1206agra Screen.Recording.2024-11-15.at.10.22.33.mov |
Alrighty, how we looking here? |
Reviewer Checklist
Screenshots/VideosAndroid: NativeScreen.Recording.2024-11-15.at.9.50.59.PM.movAndroid: mWeb ChromeScreen.Recording.2024-11-15.at.8.32.33.PM.moviOS: NativeScreen.Recording.2024-11-15.at.9.05.30.PM.moviOS: mWeb SafariScreen.Recording.2024-11-15.at.8.00.17.PM.movMacOS: Chrome / SafariScreen.Recording.2024-11-14.at.11.08.36.PM.movMacOS: DesktopScreen.Recording.2024-11-15.at.8.45.17.PM.mov |
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.
Thanks for the changes and testing
@hungvu193 can you think of ways to add unit tests for these changes? Lets try to add it in a follow up. thanks! |
✋ 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/mountiny in version: 9.0.64-0 🚀
|
Sure. Lemme think about it. |
@mountiny This PR is not fully testable easily on Hybrid app. |
🚀 Deployed to production by https://github.com/chiragsalian in version: 9.0.64-4 🚀
|
Explanation of Change
Update the Switch to Expensify Classic survey to drive better feedback
Fixed Issues
$ #51703
PROPOSAL: N/A
Tests
Case 1: People that are redirected into NewDot
Switch to Expensify Classic
Switch to Expensify Classic
No thanks
Next
Next
Switch to Expensify Classic
Case 2: People not redirected into NewDot
Switch to Expensify Classic
Offline tests
N/A
QA Steps
Same with 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
Screen.Recording.2024-11-14.at.16.21.51.mov
Screen.Recording.2024-11-14.at.16.22.44.mov
Android: mWeb Chrome
Screen.Recording.2024-11-14.at.16.04.22.mov
Screen.Recording.2024-11-14.at.16.05.03.mov
iOS: Native
Screen.Recording.2024-11-14.at.15.55.50.mov
Screen.Recording.2024-11-14.at.15.48.15.mov
iOS: mWeb Safari
Screen.Recording.2024-11-14.at.15.39.58.mov
Screen.Recording.2024-11-14.at.15.41.05.mov
MacOS: Chrome / Safari
Screen.Recording.2024-11-14.at.14.15.10.mov
Screen.Recording.2024-11-14.at.15.31.48.mov
MacOS: Desktop
Screen.Recording.2024-11-14.at.15.31.48.mov
Screen.Recording.2024-11-14.at.14.15.10.mov