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.

Sign up for GitHub

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

Fix attachment modal doesn't close when pressing search shortcut #27639

Merged
merged 2 commits into from
Sep 19, 2023
Merged

Fix attachment modal doesn't close when pressing search shortcut #27639

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8314b4b
fix modal doesn't close when pressing search shortcut
bernhardoj Sep 18, 2023
fd015a0
simplify code
bernhardoj Sep 18, 2023
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
29 changes: 19 additions & 10 deletions src/components/Modal/BaseModal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React, {forwardRef, useCallback, useEffect, useMemo} from 'react';
import React, {forwardRef, useCallback, useEffect, useMemo, useRef} from 'react';
import {View} from 'react-native';
import PropTypes from 'prop-types';
import ReactNativeModal from 'react-native-modal';
Expand All @@ -14,6 +14,7 @@ import variables from '../../styles/variables';
import CONST from '../../CONST';
import ComposerFocusManager from '../../libs/ComposerFocusManager';
import useNativeDriver from '../../libs/useNativeDriver';
import usePrevious from '../../hooks/usePrevious';

const propTypes = {
...modalPropTypes,
Expand Down Expand Up @@ -55,6 +56,9 @@ function BaseModal({

const safeAreaInsets = useSafeAreaInsets();

const isVisibleRef = useRef(isVisible);
const wasVisible = usePrevious(isVisible);

/**
* Hides modal
* @param {Boolean} [callHideCallback=true] Should we call the onModalHide callback
Expand All @@ -76,20 +80,25 @@ function BaseModal({
);

useEffect(() => {
Modal.willAlertModalBecomeVisible(isVisible);

// To handle closing any modal already visible when this modal is mounted, i.e. PopoverReportActionContextMenu
Modal.setCloseModal(isVisible ? onClose : null);
}, [isVisible, onClose]);
isVisibleRef.current = isVisible;
Copy link
Contributor

Choose a reason for hiding this comment

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

NAB. Is there a significant gain merging the two useEffects together? I think it's more readable to have the isVisibleRef useEffect separated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, just to reduce loc :). I think it's fine to put it here, we can even put it outside of useEffect, but it will feels like out of place.

if (isVisible) {
Modal.willAlertModalBecomeVisible(true);
// To handle closing any modal already visible when this modal is mounted, i.e. PopoverReportActionContextMenu
Modal.setCloseModal(onClose);
} else if (wasVisible && !isVisible) {
Modal.willAlertModalBecomeVisible(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @bernhardoj, we are thinking of moving this line, and the same line from the next useEffect into the hideModal function. Was there any specific reason not to do it in this PR? Just asking so that we won't break any edge case that I'm not aware of...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think there is any reason but just replicating the previous code.

Modal.setCloseModal(null);
}
}, [isVisible, wasVisible, onClose]);

useEffect(
() => () => {
// Only trigger onClose and setModalVisibility if the modal is unmounting while visible.
if (isVisible) {
hideModal(true);
Modal.willAlertModalBecomeVisible(false);
if (!isVisibleRef.current) {
return;
}

hideModal(true);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we should create a ref for hideModal function too, but as it doesn't have any problem yet, I leave it as it is. Let me know if we should make it a ref too.

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 ref is only needed for variables that are passed by value (not confirmed).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe, haven't tested it too

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