Improve Side Pane behavior across various Modals and navigation actions #58576
Conversation
| @@ -39,7 +39,7 @@ function Modal({fullscreen = true, onModalHide = () => {}, type, onModalShow = ( | |||
| type === CONST.MODAL.MODAL_TYPE.CENTERED || | |||
| type === CONST.MODAL.MODAL_TYPE.CENTERED_UNSWIPEABLE || | |||
| type === CONST.MODAL.MODAL_TYPE.RIGHT_DOCKED || | |||
| CONST.MODAL.MODAL_TYPE.CENTERED_SWIPABLE_TO_RIGHT; | |||
| type === CONST.MODAL.MODAL_TYPE.CENTERED_SWIPABLE_TO_RIGHT; | |||
There was a problem hiding this comment.
This was a mistake that was in the code for couple months 😅
| import {Animated} from 'react-native'; | ||
| import {Animated, View} from 'react-native'; | ||
| import {useOnyx} from 'react-native-onyx'; | ||
| // @ts-expect-error This is a workaround to display HelpPane on top of everything, |
There was a problem hiding this comment.
I’ve really tried every possible approach 😮💨
- react-native Modal : The modal must be in fullscreen mode; otherwise, the rest of the app becomes unresponsive (users can't click anything outside the side pane)
- react-native-modal (src/components/Modal/index.tsx): Similar issue as above, but additionally, the side pane was sometimes rendered under other modals in a non-deterministic way
- Portals: The help pane still appeared under React Native modals
- react-navigation primitives: Doesn’t work on web
- Custom overlay with a very high z-index: React Native modals are still rendered on top, causing the side pane to be hidden
| // Close side pane on small screens when navigation keyboard shortcuts are used | ||
| useKeyboardShortcut(CONST.KEYBOARD_SHORTCUTS.SEARCH, onCloseSidePaneOnSmallScreens, {shouldBubble: true}); | ||
| useKeyboardShortcut(CONST.KEYBOARD_SHORTCUTS.NEW_CHAT, onCloseSidePaneOnSmallScreens, {shouldBubble: true}); | ||
| useKeyboardShortcut(CONST.KEYBOARD_SHORTCUTS.SHORTCUTS, onCloseSidePaneOnSmallScreens, {shouldBubble: true}); |
There was a problem hiding this comment.
Logic for hiding the side pane when user triggers navigation shortcuts:
Screen.Recording.2025-03-18.at.13.48.09.mov
| useEffect(() => { | ||
| window.history.pushState({isSidePaneOpen: true}, '', null); | ||
| const handlePopState = () => { | ||
| if (isExtraLargeScreenWidth) { | ||
| return; | ||
| } | ||
|
|
||
| closeSidePane(); | ||
| }; | ||
|
|
||
| window.addEventListener('popstate', handlePopState); | ||
| return () => window.removeEventListener('popstate', handlePopState); | ||
| // eslint-disable-next-line react-compiler/react-compiler, react-hooks/exhaustive-deps | ||
| }, []); |
There was a problem hiding this comment.
This part handles the back button behavior on the web. We use a similar approach for Modals/Popovers. It's not perfect, but it works in most cases:
Screen.Recording.2025-03-18.at.14.35.24.mov
One known bug that I won't be able to fix is that when opening both a Modal (src/components/Modal/index.tsx) and the side pane, pressing the back button dismisses both at once instead of handling them separately:
Screen.Recording.2025-03-18.at.14.38.34.mov
| const styles = useThemeStyles(); | ||
| const {isExtraLargeScreenWidth, shouldUseNarrowLayout} = useResponsiveLayout(); | ||
| const {paddingTop, paddingBottom} = useSafeAreaPaddings(); | ||
| const [isRHPVisible = false] = useOnyx(ONYXKEYS.MODAL, {selector: (modal) => modal?.type === CONST.MODAL.MODAL_TYPE.RIGHT_DOCKED}); |
There was a problem hiding this comment.
This logic is necessary to prevent showing overlay twice:
Screen.Recording.2025-03-18.at.14.29.44.mov
| @@ -29,8 +31,9 @@ function useSidePane() { | |||
|
|
|||
| const [sidePaneNVP] = useOnyx(ONYXKEYS.NVP_SIDE_PANE); | |||
| const [language] = useOnyx(ONYXKEYS.NVP_PREFERRED_LOCALE); | |||
| const [isAttachmentModalVisible = false] = useOnyx(ONYXKEYS.MODAL, {selector: (modal) => modal?.type === CONST.MODAL.MODAL_TYPE.CENTERED}); | |||
There was a problem hiding this comment.
This change is necessary to fix the side pane on large screen:
Screen.Recording.2025-03-18.at.15.29.20.mov
blazejkustra
changed the title
|
@brunovjk I ended up fixing four issues in this PR because they were all closely connected. When I initially tried to address them separately, it actually caused even more problems along the way. Fixing them together allowed me to resolve the underlying bugs more effectively and ensure everything works smoothly. Let me know if you have any questions! |
|
Phew! Lots of work here :D 57812_web_chrome_bug.movThe rest of the issues seem pretty good to me: 57806 - MacOS: Chrome / Safari57806_web_chrome.mp458450 - MacOS: Chrome / Safari58450_web_chrome.mov57810 - MacOS: Chrome / Safari57810_web_chrome.mp4I'll keep testing on other platforms, let me know if you have any questions. Thanks a lot! |
|
Another detail, please correct me if i'm wrong, we should remove the Help Icon "Modal Avatar" as well, right? I can see the help icon when we open the avatar modal is this PR: Thanks. |
It is now removed in the PR you mentioned @brunovjk (here is the exact commit) |
Great, @blazejkustra should we remove the issue and its testing steps from this same PR description here? And place them in the mentioned PR? The changes seem good to me, but perhaps the QA team could report an errors in items 1 and 2 of the Test Steps of this PR here. |
Could you link this issue? 🤔 |
This comment was marked as off-topic.
This comment was marked as off-topic.
|
I think it's getting a little confusing. I'll ask @francoisl to review and merge the other PR first, so we can test it better here. What do you think? Thanks. |
|
@brunovjk I merged the newest main for you 👍 |
|
Great!!! Thank you |
Reviewer Checklist
Screenshots/VideosAndroid: Native58576_android_native.movAndroid: mWeb Chrome58576_android_web.moviOS: Native58576_ios_native.moviOS: mWeb Safari58576_ios_web.movMacOS: Chrome / Safari58576_web_chrome.mp4MacOS: Desktop58576_web_desktop.mp4 |
| // Close side pane on escape key press | ||
| useKeyboardShortcut(CONST.KEYBOARD_SHORTCUTS.ESCAPE, () => closeSidePane(), {isActive: !isExtraLargeScreenWidth, shouldBubble: false}); |
There was a problem hiding this comment.
I think I found an odd behavior while testing this. After closing the pane on a narrow screen (with the escape key or the < arrow) and then expanding to a wide screen view, the modal auto-reopens for some reason. It's also happening on main so NAB.
Screen.Recording.2025-03-19.at.4.58.44.PM.mov
There was a problem hiding this comment.
@francoisl @brunovjk This is the intended way it should work. When transitioning from a wide to a narrow screen, the pane is closed to prevent it from covering the rest of the app. When switching back to a wide screen, the previous state is preserved. If the pane was open before on a large screen, it will be shown again to the user.
That is why there are two booleans in the NVP:
type SidePane = {
/** Whether the side pane is open on large screens */
open: boolean;
/** Whether the side pane is open on small screens */
openNarrowScreen: boolean;
};
Same thing happens with the attachment modal on the chat view, but we might end up deciding to add a help description for those routes too, so I think it's fine to keep as is. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
My intention was to hide the help pane when attachment modal is displayed, and reopen it after: Screen.Recording.2025-03-20.at.09.07.23.movI missed the case with avatar modal, will fix it in Stage 3: Screen.Recording.2025-03-20.at.09.41.41.mov |
|
🚀 Deployed to staging by https://github.com/francoisl in version: 9.1.16-0 🚀
|
|
It seems this PR could have caused this deploy blocker: #58818 |
|
@blazejkustra PR is failing with issues #58818, #58823, #58831 |
Thanks! One was just closed, while the other two issues will be fixed in Stage 3 PR. None are deploy blockers as Side Pane is not displayed on prod. |
|
🚀 Deployed to production by https://github.com/luacmartins in version: 9.1.16-4 🚀
|
Explanation of Change
This PR addresses multiple issues related to the Side Pane visibility and behavior across different elements like the Attachment Modal, Profile Photo Editor and Chat Composer. It ensures that the Help Panel hides when necessary, modals behave consistently, and the chat input regains focus correctly after closing the RHP.
Fixed Issues
$ #57812
$ #57806
$ #58450
$ #57810
PROPOSAL: N/A
Tests
Prerequisite: Open the developer console and run the following snippet:
Offline tests
N/A
QA Steps
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)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
android.webm
Android: mWeb Chrome
and.webm
iOS: Native
ios2.mp4
iOS: mWeb Safari
Simulator.Screen.Recording.-.iPhone.16.Pro.-.2025-03-18.at.16.00.35.mp4
MacOS: Chrome / Safari
Screen.Recording.2025-03-18.at.13.48.09.mov
Screen.Recording.2025-03-18.at.15.33.29.-.01.mov
Screen.Recording.2025-03-18.at.15.29.20.mov
MacOS: Desktop
Screen.Recording.2025-03-18.at.16.49.20.mov