From d6c5c3a0492823b02393ff8f3aef3b2359644e4c Mon Sep 17 00:00:00 2001 From: situchan Date: Thu, 23 Feb 2023 16:46:50 +0100 Subject: [PATCH 1/4] close modal when opening new page using keyboard shortcut --- src/components/KeyboardShortcutsModal.js | 2 ++ src/components/Modal/BaseModal.js | 9 ++++++ src/components/Popover/index.js | 32 ++----------------- .../Navigation/AppNavigator/AuthScreens.js | 2 ++ src/libs/actions/Modal.js | 18 +++++++++++ 5 files changed, 33 insertions(+), 30 deletions(-) diff --git a/src/components/KeyboardShortcutsModal.js b/src/components/KeyboardShortcutsModal.js index fc4f2e5cf17e..81ad2f642831 100644 --- a/src/components/KeyboardShortcutsModal.js +++ b/src/components/KeyboardShortcutsModal.js @@ -14,6 +14,7 @@ import withLocalize, {withLocalizePropTypes} from './withLocalize'; import compose from '../libs/compose'; import KeyboardShortcut from '../libs/KeyboardShortcut'; import * as KeyboardShortcutsActions from '../libs/actions/KeyboardShortcuts'; +import * as ModalActions from '../libs/actions/Modal'; import ONYXKEYS from '../ONYXKEYS'; const propTypes = { @@ -35,6 +36,7 @@ class KeyboardShortcutsModal extends React.Component { componentDidMount() { const shortcutConfig = CONST.KEYBOARD_SHORTCUTS.SHORTCUT_MODAL; this.unsubscribeShortcutModal = KeyboardShortcut.subscribe(shortcutConfig.shortcutKey, () => { + ModalActions.close(); KeyboardShortcutsActions.showKeyboardShortcutModal(); }, shortcutConfig.descriptionKey, shortcutConfig.modifiers, true); } diff --git a/src/components/Modal/BaseModal.js b/src/components/Modal/BaseModal.js index dc29bc489618..708130d76b3c 100644 --- a/src/components/Modal/BaseModal.js +++ b/src/components/Modal/BaseModal.js @@ -30,17 +30,26 @@ class BaseModal extends PureComponent { this.hideModal = this.hideModal.bind(this); } + componentDidMount() { + if (!this.props.isVisible) { return; } + Modal.setModalClose(this.props.onClose); + } + componentDidUpdate(prevProps) { if (prevProps.isVisible === this.props.isVisible) { return; } Modal.willAlertModalBecomeVisible(this.props.isVisible); + Modal.setModalClose(this.props.isVisible ? this.props.onClose : null); } componentWillUnmount() { // we don't want to call the onModalHide on unmount this.hideModal(this.props.isVisible); + + // we don't want to call the onClose on unmount + Modal.setModalClose(null); } /** diff --git a/src/components/Popover/index.js b/src/components/Popover/index.js index 54ca87239593..031d9703d2c4 100644 --- a/src/components/Popover/index.js +++ b/src/components/Popover/index.js @@ -1,42 +1,20 @@ import React from 'react'; -import PropTypes from 'prop-types'; import {createPortal} from 'react-dom'; -import {withOnyx} from 'react-native-onyx'; -import {propTypes as popoverPropTypes, defaultProps as popoverDefaultProps} from './popoverPropTypes'; +import {propTypes, defaultProps} from './popoverPropTypes'; import CONST from '../../CONST'; import Modal from '../Modal'; import withWindowDimensions from '../withWindowDimensions'; -import compose from '../../libs/compose'; -import ONYXKEYS from '../../ONYXKEYS'; - -const propTypes = { - isShortcutsModalOpen: PropTypes.bool, - ...popoverPropTypes, -}; - -const defaultProps = { - isShortcutsModalOpen: false, - ...popoverDefaultProps, -}; /* * 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) { - // There are modals that can show up on top of these pop-overs, for example, the keyboard shortcut menu, - // if that happens, close the pop-over as if we were clicking outside. - props.onClose(); - return null; - } - if (!props.fullscreen && !props.isSmallScreenWidth) { return createPortal( { { + Modal.close(); Navigation.navigate(ROUTES.SEARCH); }, searchShortcutConfig.descriptionKey, searchShortcutConfig.modifiers, true); this.unsubscribeGroupShortcut = KeyboardShortcut.subscribe(groupShortcutConfig.shortcutKey, () => { + Modal.close(); Navigation.navigate(ROUTES.NEW_GROUP); }, groupShortcutConfig.descriptionKey, groupShortcutConfig.modifiers, true); } diff --git a/src/libs/actions/Modal.js b/src/libs/actions/Modal.js index 37e54093d1b1..b28ab43fab39 100644 --- a/src/libs/actions/Modal.js +++ b/src/libs/actions/Modal.js @@ -1,6 +1,22 @@ import Onyx from 'react-native-onyx'; import ONYXKEYS from '../../ONYXKEYS'; +let closeModal; + +/** + * Allows other parts of the app to call modal close function + * + * @param {Function} [onClose] + */ +function setModalClose(onClose) { + closeModal = onClose; +} + +function close() { + if (!closeModal) { return; } + closeModal(); +} + /** * Allows other parts of the app to know when a modal has been opened or closed * @@ -21,6 +37,8 @@ function willAlertModalBecomeVisible(isVisible) { } export { + setModalClose, + close, setModalVisibility, willAlertModalBecomeVisible, }; From 7805e07adf5bf8b07db0d06a2b9c9170f5e62854 Mon Sep 17 00:00:00 2001 From: situchan Date: Thu, 23 Feb 2023 20:24:21 +0100 Subject: [PATCH 2/4] add explanation of setCloseModal calls --- src/components/Modal/BaseModal.js | 10 ++++++---- src/libs/actions/Modal.js | 4 ++-- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/src/components/Modal/BaseModal.js b/src/components/Modal/BaseModal.js index 708130d76b3c..e98138d28eb0 100644 --- a/src/components/Modal/BaseModal.js +++ b/src/components/Modal/BaseModal.js @@ -32,7 +32,9 @@ class BaseModal extends PureComponent { componentDidMount() { if (!this.props.isVisible) { return; } - Modal.setModalClose(this.props.onClose); + + // to handle case of modal already visible when mounted, i.e. PopoverReportActionContextMenu + Modal.setCloseModal(this.props.onClose); } componentDidUpdate(prevProps) { @@ -41,15 +43,15 @@ class BaseModal extends PureComponent { } Modal.willAlertModalBecomeVisible(this.props.isVisible); - Modal.setModalClose(this.props.isVisible ? this.props.onClose : null); + Modal.setCloseModal(this.props.isVisible ? this.props.onClose : null); } componentWillUnmount() { // we don't want to call the onModalHide on unmount this.hideModal(this.props.isVisible); - // we don't want to call the onClose on unmount - Modal.setModalClose(null); + // to handle case of modal unmounted with visible state + Modal.setCloseModal(null); } /** diff --git a/src/libs/actions/Modal.js b/src/libs/actions/Modal.js index b28ab43fab39..ebf4fd2999c3 100644 --- a/src/libs/actions/Modal.js +++ b/src/libs/actions/Modal.js @@ -8,7 +8,7 @@ let closeModal; * * @param {Function} [onClose] */ -function setModalClose(onClose) { +function setCloseModal(onClose) { closeModal = onClose; } @@ -37,7 +37,7 @@ function willAlertModalBecomeVisible(isVisible) { } export { - setModalClose, + setCloseModal, close, setModalVisibility, willAlertModalBecomeVisible, From 1de1a4598eaca24fd5d93e4784bbab34c52db896 Mon Sep 17 00:00:00 2001 From: Situ Chandra Shil <108292595+situchan@users.noreply.github.com> Date: Fri, 24 Feb 2023 14:33:21 -0600 Subject: [PATCH 3/4] update comment Co-authored-by: Puneet Lath --- src/components/Modal/BaseModal.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/Modal/BaseModal.js b/src/components/Modal/BaseModal.js index e98138d28eb0..48a3c7033111 100644 --- a/src/components/Modal/BaseModal.js +++ b/src/components/Modal/BaseModal.js @@ -33,7 +33,7 @@ class BaseModal extends PureComponent { componentDidMount() { if (!this.props.isVisible) { return; } - // to handle case of modal already visible when mounted, i.e. PopoverReportActionContextMenu + // To handle closing any modal already visible when this modal is mounted, i.e. PopoverReportActionContextMenu Modal.setCloseModal(this.props.onClose); } From 2c92f43ed3ef45a01be2146d0229d451d4b907f2 Mon Sep 17 00:00:00 2001 From: Situ Chandra Shil <108292595+situchan@users.noreply.github.com> Date: Fri, 24 Feb 2023 14:43:20 -0600 Subject: [PATCH 4/4] update comment --- src/components/Modal/BaseModal.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/Modal/BaseModal.js b/src/components/Modal/BaseModal.js index 48a3c7033111..a4b7ed90a8fe 100644 --- a/src/components/Modal/BaseModal.js +++ b/src/components/Modal/BaseModal.js @@ -50,7 +50,7 @@ class BaseModal extends PureComponent { // we don't want to call the onModalHide on unmount this.hideModal(this.props.isVisible); - // to handle case of modal unmounted with visible state + // To prevent closing any modal already unmounted when this modal still remains as visible state Modal.setCloseModal(null); }