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 all popovers when Keyboard shortcut modal is displayed #13956

Merged
merged 11 commits into from
Feb 14, 2023
2 changes: 2 additions & 0 deletions src/Expensify.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import Navigation from './libs/Navigation/Navigation';
import DeeplinkWrapper from './components/DeeplinkWrapper';
import PopoverReportActionContextMenu from './pages/home/report/ContextMenu/PopoverReportActionContextMenu';
import * as ReportActionContextMenu from './pages/home/report/ContextMenu/ReportActionContextMenu';
import KeyboardShortcutsModal from './components/KeyboardShortcutsModal';

// This lib needs to be imported, but it has nothing to export since all it contains is an Onyx connection
// eslint-disable-next-line no-unused-vars
Expand Down Expand Up @@ -197,6 +198,7 @@ class Expensify extends PureComponent {
<DeeplinkWrapper>
{!this.state.isSplashShown && (
<>
<KeyboardShortcutsModal />
<GrowlNotification ref={Growl.growlRef} />
<PopoverReportActionContextMenu
ref={ReportActionContextMenu.contextMenuRef}
Expand Down
40 changes: 34 additions & 6 deletions src/components/Popover/index.js
Original file line number Diff line number Diff line change
@@ -1,22 +1,44 @@
import React from 'react';
import PropTypes from 'prop-types';
import {createPortal} from 'react-dom';
import {propTypes, defaultProps} from './popoverPropTypes';
import {withOnyx} from 'react-native-onyx';
import {propTypes as popoverPropTypes, defaultProps as popoverDefaultProps} 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
type={CONST.MODAL.MODAL_TYPE.POPOVER}
popoverAnchorPosition={props.anchorPosition}
// 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}
animationOutTiming={props.disableAnimation ? 1 : props.animationOutTiming}
shouldCloseOnOutsideClick
Expand All @@ -26,10 +48,11 @@ const Popover = (props) => {
}
return (
<Modal
type={props.isSmallScreenWidth ? CONST.MODAL.MODAL_TYPE.BOTTOM_DOCKED : CONST.MODAL.MODAL_TYPE.POPOVER}
popoverAnchorPosition={props.isSmallScreenWidth ? undefined : props.anchorPosition}
// 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}
animationInTiming={props.disableAnimation && !props.isSmallScreenWidth ? 1 : props.animationInTiming}
animationOutTiming={props.disableAnimation && !props.isSmallScreenWidth ? 1 : props.animationOutTiming}
Expand All @@ -41,4 +64,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);
2 changes: 0 additions & 2 deletions src/pages/home/sidebar/SidebarScreen/BaseSidebarScreen.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import Timing from '../../../../libs/actions/Timing';
import CONST from '../../../../CONST';
import Performance from '../../../../libs/Performance';
import withDrawerState from '../../../../components/withDrawerState';
import KeyboardShortcutsModal from '../../../../components/KeyboardShortcutsModal';
import withWindowDimensions, {windowDimensionsPropTypes} from '../../../../components/withWindowDimensions';
import compose from '../../../../libs/compose';
import sidebarPropTypes from './sidebarPropTypes';
Expand Down Expand Up @@ -66,7 +65,6 @@ class BaseSidebarScreen extends Component {
onLayout={this.props.onLayout}
/>
</View>
<KeyboardShortcutsModal />
Copy link
Collaborator

Choose a reason for hiding this comment

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

We moved the KeybaordShoftcutsModal from BaseSidebarScreen to Expensify.js as a refactor or for any specific reason?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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 🙂
image

{this.props.children}
</>
)}
Expand Down