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 1 commit
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
9 changes: 9 additions & 0 deletions src/components/Modal/BaseModal.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this necessary? I think modals are mounted when before they are visible, but maybe this is just to make sure? If so please add a comment explaining why.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a special case for PopoverReportActionContextMenu.
I confirmed that isVisible is already true when component mount, so componentDidUpdate is never called.
image


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);
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs an explanation of why

Copy link
Contributor Author

@situchan situchan Feb 23, 2023

Choose a reason for hiding this comment

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

we don't want to call the onClose on unmount
Is this explanation not enough?
I think this description is actually wrong.
This should be better description:
to handle case of modal unmounted with visible state

}

/**
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 setModalClose(onClose) {
situchan marked this conversation as resolved.
Show resolved Hide resolved
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 {
setModalClose,
close,
setModalVisibility,
willAlertModalBecomeVisible,
};