Skip to content
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

close modal when opening new page using keyboard shortcut #15413

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/components/KeyboardShortcutsModal.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {
Expand All @@ -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();
situchan marked this conversation as resolved.
Show resolved Hide resolved
KeyboardShortcutsActions.showKeyboardShortcutModal();
}, shortcutConfig.descriptionKey, shortcutConfig.modifiers, true);
}
Expand Down
11 changes: 11 additions & 0 deletions src/components/Modal/BaseModal.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,17 +30,28 @@ class BaseModal extends PureComponent {
this.hideModal = this.hideModal.bind(this);
}

componentDidMount() {
if (!this.props.isVisible) { return; }

// To handle closing any modal already visible when this modal is mounted, i.e. PopoverReportActionContextMenu
Modal.setCloseModal(this.props.onClose);
}

componentDidUpdate(prevProps) {
if (prevProps.isVisible === this.props.isVisible) {
return;
}

Modal.willAlertModalBecomeVisible(this.props.isVisible);
Modal.setCloseModal(this.props.isVisible ? this.props.onClose : null);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@situchan Do you recall why this call is done in that way? i.e. why we need to set the callback to null if this modal is not visible? Why we don't do it the same as componentDidMount:

if (this.props.isVisible) {
    Modal.setModalClose(this.props.onClose);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We pass onClose callback to setCloseModal to allow other parts of the app to close this modal manually.
If already closed, why we still need to have this stale callback? Also see below code why it's needed:

function close(onModalCloseCallback: () => void, isNavigating = true) {
if (!closeModal) {
// If modal is already closed, no need to wait for modal close. So immediately call callback.
if (onModalCloseCallback) {
onModalCloseCallback();
}
onModalClose = null;
return;
}

Btw, does that cause any issue?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If already closed, why we still need to have this stale callback?

We may have other modals that did set the callback. If we render a new modal invisible, the old visible modal close callback will be overwritten.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw, does that cause any issue?

Not exactly, but the functional rewrite of the modal seems to be the cause of the regression. I think there was a misunderstanding of how the code is supposed to be executed. Here at this point, componentDidMount and componentDidUpdate call Modal.setModalClose differently (as explained above) but after the functional migration we only use useEffect and both logic get merged to look like componentDidUpdate,

}

componentWillUnmount() {
// we don't want to call the onModalHide on unmount
this.hideModal(this.props.isVisible);

// To prevent closing any modal already unmounted when this modal still remains as visible state
Modal.setCloseModal(null);
}

/**
Expand Down
32 changes: 2 additions & 30 deletions src/components/Popover/index.js
Original file line number Diff line number Diff line change
@@ -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;
}

situchan marked this conversation as resolved.
Show resolved Hide resolved
if (!props.fullscreen && !props.isSmallScreenWidth) {
return createPortal(
<Modal
// eslint-disable-next-line react/jsx-props-no-spreading
{...props}
isVisible={props.isVisible && !props.isShortcutsModalOpen}
type={CONST.MODAL.MODAL_TYPE.POPOVER}
popoverAnchorPosition={props.anchorPosition}
animationInTiming={props.disableAnimation ? 1 : props.animationInTiming}
Expand All @@ -50,7 +28,6 @@ const Popover = (props) => {
<Modal
// eslint-disable-next-line react/jsx-props-no-spreading
{...props}
isVisible={props.isVisible && !props.isShortcutsModalOpen}
type={props.isSmallScreenWidth ? CONST.MODAL.MODAL_TYPE.BOTTOM_DOCKED : CONST.MODAL.MODAL_TYPE.POPOVER}
popoverAnchorPosition={props.isSmallScreenWidth ? undefined : props.anchorPosition}
fullscreen={props.isSmallScreenWidth ? true : props.fullscreen}
Expand All @@ -64,9 +41,4 @@ Popover.propTypes = propTypes;
Popover.defaultProps = defaultProps;
Popover.displayName = 'Popover';

export default compose(
withWindowDimensions,
withOnyx({
isShortcutsModalOpen: {key: ONYXKEYS.IS_SHORTCUTS_MODAL_OPEN},
}),
)(Popover);
export default withWindowDimensions(Popover);
2 changes: 2 additions & 0 deletions src/libs/Navigation/AppNavigator/AuthScreens.js
Original file line number Diff line number Diff line change
Expand Up @@ -115,9 +115,11 @@ class AuthScreens extends React.Component {
// the chat switcher, or new group chat
// based on the key modifiers pressed and the operating system
this.unsubscribeSearchShortcut = KeyboardShortcut.subscribe(searchShortcutConfig.shortcutKey, () => {
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);
}
Expand Down
18 changes: 18 additions & 0 deletions src/libs/actions/Modal.js
Original file line number Diff line number Diff line change
@@ -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 setCloseModal(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
*
Expand All @@ -21,6 +37,8 @@ function willAlertModalBecomeVisible(isVisible) {
}

export {
setCloseModal,
close,
setModalVisibility,
willAlertModalBecomeVisible,
};
Loading