-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Refactor: reuse ValidateCodeActionModal #49445
base: main
Are you sure you want to change the base?
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 👀 |
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.
I think we might need to delete this page right?
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.
Let's remove this screen and add ValidateCodeActionModal
to ConfirmDelegatePage
and SecuritySettingsPage
instead 😄
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.
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 |
I think I still can reproduce this issue. Here's the reproduce steps:
|
Not the same issue but, it navigates back right? |
Yes and with a glitch animation |
@getusha @hungvu193 how is this looking? |
Pending on this one right here |
finally the lint issue is gone 😄 |
@hungvu193 please test this again 🙏 |
@hungvu193
Details
Fixed Issues
$
PROPOSAL:
Tests
New contact method
Verify contact method
Copilot: Delegated access
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
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop