-
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
fix: requires validate when opening bank account #49230
Conversation
@hungvu193 I used the App/src/components/ValidateCodeActionModal/ValidateCodeForm/BaseValidateCodeForm.tsx Lines 131 to 137 in 8d19726
I see that in other places, we show the error after the modal is closed, but why don't we show the error directly in the form, so the user don't have to open the modal again? |
Yeah, we have a plan to change that behavior in here, we should clear the error when modal is closed. |
@jjcoffee According to #49230 (comment), should we hold until #49270 is resolved until we can continue on this one? cc @hungvu193 |
@jjcoffee I tried using |
@daledah Following the flow |
Reviewer Checklist
Screenshots/VideosAndroid: Nativeandroid-app-2024-09-19_14.15.23.mp4Android: mWeb Chromeandroid-chrome-2024-09-19_14.17.18.mp4iOS: Nativeios-app-2024-09-19_14.01.39.mp4iOS: mWeb Safariios-safari-2024-09-19_13.55.34.mp4MacOS: Chrome / Safaridesktop-chrome-2024-09-19_14.04.03.mp4MacOS: Desktopdesktop-app-2024-09-19_14.12.43.mp4 |
This comment was marked as resolved.
This comment was marked as resolved.
Tagging @Expensify/design to take a look! Do we need to tag anyone else to review the copy? |
This comment was marked as resolved.
This comment was marked as resolved.
Looks pretty good to me 👍 |
Could have sworn I saw @dannymcclain leave some feedback in another PR that we could use some more spacing between the inputs and the "Didn't receive a code?" line: Does that sound right? |
@jjcoffee Does this look good to you? Screen.Recording.2024-09-27.at.12.19.17.mov |
Looks good to me! Any thoughts on the updated flow @dannymcclain @dubielzyk-expensify? |
Oh I think that feels much better. Let's see what Jon thinks too! |
Yep, looks good. Send it! |
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!
@jjcoffee @arosiclair Looks like #47218 has implemented another add bank account button on Workspace invoices page. Should we keep current behavior on the button as well? Video: Screen.Recording.2024-10-01.at.10.57.07.mov |
It looks like that feature is in the process of being developed (it doesn't show up in the menu items yet), so I would say we shouldn't add it here yet. It also feels a bit janky to navigate back to the wallet page in the background as in your video, so we'd probably want to add this as a modal instead which would be out of scope of this particular issue. cc @arosiclair |
Agree! |
@jjcoffee I updated. FYI, I left the new logic unchanged. |
Over to you @arosiclair! |
✋ 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/arosiclair in version: 9.0.44-0 🚀
|
🚀 Deployed to production by https://github.com/jasperhuangg in version: 9.0.44-12 🚀
|
@@ -37,6 +37,8 @@ const CENTRAL_PANE_TO_RHP_MAPPING: Partial<Record<CentralPaneName, string[]>> = | |||
SCREENS.SETTINGS.WALLET.CARD_ACTIVATE, | |||
SCREENS.SETTINGS.WALLET.REPORT_VIRTUAL_CARD_FRAUD, | |||
SCREENS.SETTINGS.WALLET.CARDS_DIGITAL_DETAILS_UPDATE_ADDRESS, | |||
SCREENS.SETTINGS.WALLET.VERIFY_ACCOUNT, | |||
SCREENS.SETTINGS.ADD_BANK_ACCOUNT, |
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 #50214 this caused an issue for Add bank account
buttons that shouldn't show the settings page as the central pane.
Details
Fixed Issues
$ #48041
PROPOSAL: #48041 (comment)
Tests
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 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-09-18.at.18.10.51.mov
Android: mWeb Chrome
Screen.Recording.2024-09-18.at.18.12.56.mov
iOS: Native
Screen.Recording.2024-09-18.at.23.29.02.mov
iOS: mWeb Safari
Screen.Recording.2024-09-18.at.23.30.55.mov
MacOS: Chrome / Safari
Screen.Recording.2024-09-18.at.17.48.10.mov
MacOS: Desktop
Screen.Recording.2024-09-18.at.23.31.26.mov