Close all popovers when Keyboard shortcut modal is displayed #13956
Conversation
melvin-bot
bot
requested review from
mananjadhav and
thienlnam
and removed request for
a team
|
@mananjadhav @thienlnam One of you needs to 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] |
|
@mananjadhav PR is pretty simple, no need to review this one! |
pecanoro
changed the title
|
@thienlnam Don't review it yet, it seems that moving it doesn't solve all issues completely |
pecanoro
changed the title
|
This PR ended up not being that simple hehe The main problem here is that popovers are also modals and react native does not work well with opening multiple modals. In theory, clicking away from one popover should let the modal go away, but we are opening the shortcut modal using a key listener so any other popovers won't get the memo to get closed. The problem with being one on top of the other is something related to the styles that the modal gets here. However, since this is not supported behavior either way, I decided to simply dismiss any popovers that were already opened if we were to show the shortcut modal. I am going to require a review from @mananjadhav as well because I think it needs two pair of eyes. |
|
Okay @pecanoro I'll review and test this today. |
pecanoro
changed the title
src/components/Popover/index.js
Outdated
| if (!props.fullscreen && !props.isSmallScreenWidth) { | ||
| return createPortal( | ||
|
|
||
| class Popover extends React.Component { |
There was a problem hiding this comment.
qq, any specific reason why we needed this to be moved to a class component?
Wouldn't the following check suffice the requirement?
if ( this.props.isShortcutsModalOpen && this.props.isVisible) {
this.props.onClose();
return null;
}
There was a problem hiding this comment.
Yeah, the linter was complaining that I should return early 😞
There was a problem hiding this comment.
32:35 error Prefer an early return to a conditionally-wrapped function body rulesdir/prefer-early-return
There was a problem hiding this comment.
I just attempted:
diff --git a/src/components/Popover/index.js b/src/components/Popover/index.js
index 9a2e20ed3d..8205db0c1e 100644
--- a/src/components/Popover/index.js
+++ b/src/components/Popover/index.js
@@ -1,15 +1,23 @@
import React from 'react';
+import {withOnyx} from 'react-native-onyx';
import {createPortal} from 'react-dom';
import {propTypes, defaultProps} from './popoverPropTypes';
import CONST from '../../CONST';
import Modal from '../Modal';
+import compose from '../../libs/compose';
import withWindowDimensions from '../withWindowDimensions';
+import ONYXKEYS from '../../ONYXKEYS';
/*
* This is a convenience wrapper around the Modal component for a responsive Popover.
* On small screen widths, it uses BottomDocked modal type, and a Popover type on wide screen widths.
*/
const Popover = (props) => {
+ if (props.isShortcutsModalOpen && props.isVisible) {
+ props.onClose();
+ return null;
+ }
+
if (!props.fullscreen && !props.isSmallScreenWidth) {
return createPortal(
<Modal
@@ -41,4 +49,9 @@ Popover.propTypes = propTypes;
Popover.defaultProps = defaultProps;
Popover.displayName = 'Popover';
-export default withWindowDimensions(Popover);
+export default compose(
+ withWindowDimensions,
+ withOnyx({
+ isShortcutsModalOpen: {key: ONYXKEYS.IS_SHORTCUTS_MODAL_OPEN},
+ }),
+)(Popover);
and I didn't get any lint errors
There was a problem hiding this comment.
Ahh you mean why I moved it to a class and not exactly about the if condition?
There was a problem hiding this comment.
About the class, yeah, I guess we didn't need it anymore, I played with the prevProps at the beginning and that's why I created it for componentDidUpdate, but then I realized the condition was redundant and worked well without checking any prevProps. I can revert it back now that I am only using the props and nothing else.
There was a problem hiding this comment.
I can revert it back now that I am only using the props and nothing else
yeah, exactly. Thank you.
| @@ -66,7 +65,6 @@ class BaseSidebarScreen extends Component { | |||
| onLayout={this.props.onLayout} | |||
| /> | |||
| </View> | |||
| <KeyboardShortcutsModal /> | |||
There was a problem hiding this comment.
We moved the KeybaordShoftcutsModal from BaseSidebarScreen to Expensify.js as a refactor or for any specific reason?
There was a problem hiding this comment.
Since it'a high level modal that should displayed at any level, I thought the highest would be the best. We need to mount it at least one. We do something similar with UpdateAppModal for example
There was a problem hiding this comment.
Agreed with the reasoning, even I wasn't sure why it was in BaseSidebarScreen. I asked because the Popover changes worked even without moving the KeyboardsModal to Expensify.js.
There was a problem hiding this comment.
I think this was to prevent shortcut modal when user logged out.
BaseSidebarScreen is highest level on auth screens.
Expensify.js is highest level on all screens (including unauthorized screens).
Now I can see shortcut modal open on login page. This is not an issue though 🙂
|
Updated! |
|
@pecanoro Thanks for the changes. They look ok. I'll be testing them by Monday. |
|
@pecanoro This tests well with different Popovers. We have a similar case for the Modal, I think we can deal with that as a separate issue right?
|
Reviewer Checklist
Screenshots/VideosWebweb-keyboard-modal-popover.movMobile Web - Chromemweb-chrome-keyboard-modal-popover.movMobile Web - Safarimweb-safari-keyboard-modal-popover.movDesktopweb-keyboard-modal-popover.moviOSios-keyboard-modal-popover.movAndroidandroid-keyboard-modal-popover.movThanks for the patience here. This tests well, except for the |
|
ConfirmModal is not a "Popover", right? Yeah, I focused only on popovers and not the other types of modals because it is more unclear what we would want to do in those cases when we also add the backdrop. |
|
Yeah not a Popover. It's a Modal. Hence, I mentioned here let's do it as a separate follow up PR. |
There was a problem hiding this comment.
The main problem here is that popovers are also modals and react native does not work well with opening multiple modals. In theory, clicking away from one popover should let the modal go away, but we are opening the shortcut modal using a key listener so any other popovers won't get the memo to get closed.
I agree with the solution, but it seems like what we're doing here is specific to just the keyboardShortcut modal, and if we were to add additional modals we are adding more onyx keys and logic checks
Is this something that would be possible to add inside the popover or modal component so that this works generally?
|
The keyboard shortcut one is the only one that opens from the keyboard (unless I am missing something), any other will trigger the "click outside the modal" event and will get closed. This needs to listen to the Onyx key that keeps track if the modal is opened. If we need to do this for any of the modals, then I agree we can generalize it and use maybe a general Onyx key for all of them, but I feel that is getting ahead of it. |
|
Sure, we can definitely address when it comes up |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
There are other cases of opening modal using shortcut key: |
|
@situchan I tested the keyboard opening over these and it was working but guess what I didn't test, actually using the shortcut for them. I will create another PR to fix that case as well, it wasn't working before no matter what so at least we didn;t break anything (yet) |
|
There's already issue involving all these cases and I proposed solution for these 3 shortcuts but this still doesn't fix this case @mananjadhav reported since this is non-popover. |
|
@situchan If I am following properly, you are saying you will propose a fix even for this case in the other issue, right? If so, all good, we can keep this one closed then! |
|
🚀 Deployed to staging by https://github.com/thienlnam in version: 1.2.72-0 🚀
|
|
🚀 Deployed to production by https://github.com/francoisl in version: 1.2.72-1 🚀
|
By keeping the modal in BaseSidebarScreen.js, it wasn't being rendered on top of everything and so we were getting weird behavior in which you could still click on menus even though the shortcut was opened.
Fixed Issues
$ #13871
Tests / Offline tests / QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)Avataris modified, I verified thatAvataris working as expected in all cases)ScrollViewcomponent to make it scrollable when more elements are added to the page.Screenshots/Videos
Web
Monosnap.screencast.2023-02-08.16-03-33.mp4
Mobile Web - Chrome
Monosnap.screencast.2023-02-08.16-43-56.mp4
Mobile Web - Safari
Monosnap.screencast.2023-02-08.16-20-14.mp4
Desktop
Monosnap.screencast.2023-02-08.16-14-48.mp4
iOS
Monosnap.screencast.2023-02-08.16-29-42.mp4
Android
Monosnap.screencast.2023-02-08.16-40-43.mp4