Refactor: reuse ValidateCodeActionModal #49445
Conversation
|
Note: |
|
@getusha mind looking at this convo and let me know your thoughts? 😄 |
|
@hungvu193 are you able to create account using a phone number? Screen.Recording.2024-09-24.at.11.19.22.in.the.morning.mov |
I'm not, Unfortunately :/ |
|
Testing now 👀 |
getusha
changed the title
There was a problem hiding this comment.
I think we might need to delete this page right?
There was a problem hiding this comment.
Let's remove this screen and add ValidateCodeActionModal to ConfirmDelegatePage and SecuritySettingsPage instead 😄
There was a problem hiding this comment.
Yes, i was thinking that too that makes the most sense. i'll update it :)
|
@hungvu193 I realized this a little bit late but, seems like we may need to keep the page to avoid duplicate code This page is accessed directly from SecuritySettingsPage as well. wdyt? You mentioned this earlier, i didn't catch it :') |
|
I think it's a little weird when a screen only contains a modal as a single view, I'm not sure if it's OK. So let's use |
@hungvu193 what i am saying is, we'll just be duplicating the code in two pages while we can have it in a single page and re-use it. |
What's your plan? We still keep this screen or create a reusable component based on ValidateCodeModal? |
Yeah, you're right. creating a reusable component that'll include all the logic based on ValidateCodeModal sounds ideal. Keeping the screen doesn't seem that bad as well if we test it thoroughly. |
|
Cool. Let's do it 🚀 |
|
@hungvu193 what do you think about this? 92a43b5 |
Looks pretty good to me. Can you address the issue that we found during our last preview so I can start testing review again? |
@hungvu193 sorry, which one? 😄 |
https://expensify.slack.com/archives/D07N0F7RE6Q/p1727171810042779 |
Let me test it. Please address the lining 😄 |
|
On it! even though i have no idea what |
| @@ -62,6 +62,8 @@ type ValidateCodeFormProps = { | |||
|
|
|||
| /** Function to clear error of the form */ | |||
| clearError: () => void; | |||
|
|
|||
| sendValidateCode: () => void; | |||
|
@mountiny looks like this was merged without a test passing. Please add a note explaining why this was done and remove the |
|
Great job, seeing this one through |
|
✋ 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.52-0 🚀
|
|
looks like this broke a flow with copilots #51266 |
|
Also causing an issue when validating a second contact method: #51274 |
|
This was not an emergency, seems like the checks are flaky and incorrectly flag prs as emergency, the tests were passing |
|
🚀 Deployed to production by https://github.com/yuwenmemon in version: 9.0.52-5 🚀
|
| @@ -61,10 +74,13 @@ function ValidateCodeActionModal({isVisible, title, description, onClose, valida | |||
| validatePendingAction={validatePendingAction} | |||
| validateError={validateError} | |||
| handleSubmitForm={handleSubmitForm} | |||
| sendValidateCode={sendValidateCode} | |||
There was a problem hiding this comment.
Is there any reason why we made this a prop instead of implementing it in this component?
There was a problem hiding this comment.
sendValidateCode is passed to be used only for requesting a new magic code in BaseValidateCodeForm
There was a problem hiding this comment.
Yes, so why isn't the implementation of sendValidateCode is not in ValidateCodeActionModal here
, why do we need to supply it from parent components given that it's always going to be the same?There was a problem hiding this comment.
Basically this code is repeated in every component that uses this modal
const sendValidateCode = () => {
if (loginData?.validateCodeSent) {
return;
}
User.requestValidateCodeAction();
};
There was a problem hiding this comment.
There are two functions to request a magic code requestContactMethodValidateCode & requestValidateCodeAction
And there may be conditions before calling the function, which means if we want to implement sendValidateCode in the modal we'll still need to supply loginData and contact method with a boolean prop for which function to use.
App/src/pages/settings/Wallet/ExpensifyCardPage.tsx
Lines 150 to 151 in 9508b17
I thought this would be the most convenient way to make the component flexible.
| @@ -74,6 +72,13 @@ function ConfirmDelegatePage({route}: ConfirmDelegatePageProps) { | |||
| onPress={() => Navigation.navigate(ROUTES.SETTINGS_DELEGATE_ROLE.getRoute(login, role))} | |||
| shouldShowRightIcon | |||
| /> | |||
|
|
|||
| {isValidateCodeActionModalVisible && ( | |||
There was a problem hiding this comment.
Coming from #51273, Here, when we close the modal from the child component, the isValidateCodeActionModalVisible value on the parent component doesn’t get updated. As a result, when we navigate back and try to open the modal again, the button won’t trigger it because the value remains true on the parent.
More context on this in the proposal: #51273 (comment)
| @@ -189,7 +188,7 @@ function BaseValidateCodeForm({ | |||
| errorText={formError?.validateCode ? translate(formError?.validateCode) : ErrorUtils.getLatestErrorMessage(account ?? {})} | |||
| hasError={!isEmptyObject(validateError)} | |||
| onFulfill={validateAndSubmitForm} | |||
| autoFocus={false} | |||
| autoFocus | |||
There was a problem hiding this comment.
Thank you. We already discussed it during PR phase, but then forgot to update it here.
@hungvu193
Details
Fixed Issues
$ #49270
PROPOSAL: N/A
Tests
New contact method
Verify contact method
Copilot: Delegated access
Offline tests
QA Steps
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Screen.Recording.2024-10-17.at.9.10.59.at.night.mov
Android: mWeb Chrome
Screen.Recording.2024-10-18.at.9.48.07.in.the.morning.mov
iOS: Native
Screen.Recording.2024-10-18.at.10.16.13.in.the.morning.mov
iOS: mWeb Safari
Screen.Recording.2024-10-18.at.9.42.45.in.the.morning.mov
MacOS: Chrome / Safari
Screen.Recording.2024-10-17.at.2.26.22.in.the.afternoon.mov
Screen.Recording.2024-10-17.at.8.54.26.in.the.evening.mov
MacOS: Desktop
Screen.Recording.2024-10-18.at.9.16.26.in.the.morning.mov