-
Notifications
You must be signed in to change notification settings - Fork 3.2k
Fix: On tapping device back button to close troubleshoot box from LHN, navigated out of site #58608
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
Conversation
…, navigated out of site
@hungvu193 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] |
Everything is working well 🎉. The only concern here is that after the App/src/components/Modal/index.tsx Lines 26 to 28 in ece826f
I think that's the reason why we only use Screen.Recording.2025-03-18.at.14.18.28.mov |
What do you think about the comment about? Do you have any idea to fix it? |
I noticed that the Should we address this issue in the current PR or handle it in a separate one? I think we could add a new function to manage the forward click, which would reopen the modal. Will check further if we need to handle that case on this PR |
If a generic solution can be applied for all other places then I think Yes. |
Alright, I will look into that issue further. I'll let you know if we can use that approach |
Any luck? @nyomanjyotisa |
I'm thinking about the modal screen: I think we also have a plan to convert all the current modal to screen (not sure what the latest decision is), but let me know your opinion. |
Unfortunately, I encountered some issues when implementing the approach to re-open the modal when the forward button is clicked
Sure, let me try this approach |
Any updates here? Let me know anything that I can help |
Still in progress. |
Hey @nyomanjyotisa, any updates here? If the modal screen solution doesn't work and you don't have any other solution to resolve the issue, then I think we should open this issue for proposals again. |
@hungvu193 Sorry for the slow update here, could you please check the updated code? Thanks! |
Can you also update all the screenshots/videos please? |
Sure, I'll create a Slack post about overlay on native as soon as possible |
Checked on main, same behavior we can go ahead with current solution
|
@@ -67,6 +68,11 @@ function PublicScreens() { | |||
component={PublicRightModalNavigator} | |||
options={rootNavigatorScreenOptions.rightModalNavigator} | |||
/> | |||
<RootStack.Screen | |||
name={NAVIGATORS.TEST_TOOLS_MODAL_NAVIGATOR} | |||
options={rootNavigatorScreenOptions.basicModalNavigator} |
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.
We can create a custom screen options here with the native option for the native to enable the overlay:
options={rootNavigatorScreenOptions.basicModalNavigator} | |
options={{ | |
...rootNavigatorScreenOptions.basicModalNavigator, | |
native: { | |
contentStyle: { | |
backgroundColor: theme.overlay | |
} | |
} | |
}} |
We can confirm the backdrop color with design team later:
Screen.Recording.2025-05-05.at.10.22.22.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.
Now we have another thing to address. Click on overlay to dismiss the modal. You can see the references here:
https://reactnavigation.org/docs/stack-navigator/#transparent-modals
If you want to further customize how the dialog animates, or want to close the screen when tapping the overlay etc., you can use the useCardAnimation hook to customize elements inside your screen.
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've updated the PR and addressed this issue as well
Click on overlay to dismiss the modal
Could you please review it again?
contentStyle: { | ||
backgroundColor: theme.overlay, | ||
}, | ||
animation: InternalPlatformAnimations.SLIDE_FROM_BOTTOM, |
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 use FADE here for smoother animation
animation: InternalPlatformAnimations.SLIDE_FROM_BOTTOM, | |
animation: InternalPlatformAnimations.FADE, |
Hi @Expensify/design team, we plan to migrate the current TestTools modal to Modal Screen. Could you please review the UI below and give us feedback? Thank you With slide from bottom animation: push.from.bottom.movWith fade animation: fade.movChrome: Screen.Recording.2025-05-07.at.09.34.10.mov |
The PR has been updated to address the native overlay issue. Here’s how it looks now: iOS-Native.mp4 |
Good shout Jon. Also, not sure how much we want to mess with this, but on mobile I would expect the overlay to fade in and the modal to slide up—that's how our other modal/bottom sheets work. (But I realize getting this perfect might not be our top priority.) |
So I think #58608 (comment) will look correctly |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid.movAndroid: mWeb ChromemAndroid.moviOS: HybridAppfade.movpush.from.bottom.movScreen.Recording.2025-05-08.at.16.26.52.moviOS: mWeb SafariSafari.movMacOS: Chrome / SafariChrome.movMacOS: DesktopDesktop.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.
Looks good, nice work!
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
Revert "Merge pull request #58608 from nyomanjyotisa/issue-58383"
@kavimuru yes. This PR was reverted to fix few blockers |
Explanation of Change
Fixed Issues
$ #58383
PROPOSAL: #58383 (comment)
Tests
Same as QA Steps
Offline tests
Same as QA Steps
QA Steps
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
Android-Native.mp4
Android: mWeb Chrome
Android-mWeb.Chrome.mp4
iOS: Native
iOS-Native.mp4
iOS: mWeb Safari
iOS-mWeb.Safari.mp4
MacOS: Chrome / Safari
MacOS-Chrome.mp4
MacOS: Desktop
MacOS-Desktop.mp4