From 66b2c82a9390a98123e513119e12643f1a9100cb Mon Sep 17 00:00:00 2001 From: tienifr Date: Sun, 29 Oct 2023 18:15:02 +0700 Subject: [PATCH 1/7] fix: 29378 Distance - Search is open in background on distance image preview --- src/components/Modal/BaseModal.js | 8 +++++--- src/libs/actions/Modal.ts | 34 +++++++++++++++++++------------ 2 files changed, 26 insertions(+), 16 deletions(-) diff --git a/src/components/Modal/BaseModal.js b/src/components/Modal/BaseModal.js index 6ed3b16c676d..b3e5b26b64ed 100644 --- a/src/components/Modal/BaseModal.js +++ b/src/components/Modal/BaseModal.js @@ -59,6 +59,8 @@ function BaseModal({ const isVisibleRef = useRef(isVisible); const wasVisible = usePrevious(isVisible); + const onCloseKey = useRef(Modal.getAvailableKey()); + /** * Hides modal * @param {Boolean} [callHideCallback=true] Should we call the onModalHide callback @@ -84,10 +86,10 @@ function BaseModal({ if (isVisible) { Modal.willAlertModalBecomeVisible(true); // To handle closing any modal already visible when this modal is mounted, i.e. PopoverReportActionContextMenu - Modal.setCloseModal(onClose); + Modal.setCloseModal(onCloseKey.current, onClose); } else if (wasVisible && !isVisible) { Modal.willAlertModalBecomeVisible(false); - Modal.setCloseModal(null); + Modal.setCloseModal(onCloseKey.current, null); } }, [isVisible, wasVisible, onClose]); @@ -100,7 +102,7 @@ function BaseModal({ hideModal(true); Modal.willAlertModalBecomeVisible(false); // To prevent closing any modal already unmounted when this modal still remains as visible state - Modal.setCloseModal(null); + Modal.setCloseModal(onCloseKey.current, null); }, // eslint-disable-next-line react-hooks/exhaustive-deps [], diff --git a/src/libs/actions/Modal.ts b/src/libs/actions/Modal.ts index 5114d5c1f42f..431142adfb20 100644 --- a/src/libs/actions/Modal.ts +++ b/src/libs/actions/Modal.ts @@ -1,30 +1,38 @@ import Onyx from 'react-native-onyx'; import ONYXKEYS from '@src/ONYXKEYS'; -let closeModal: (isNavigating: boolean) => void; +const closeModals: Record void)> = {}; +let count = 0; + +// let closeModal: (isNavigating: boolean) => void; let onModalClose: null | (() => void); +/** + * Get the available key that we can store the onClose callback into it + */ +function getAvailableKey() { + return count++; +} + /** * Allows other parts of the app to call modal close function */ -function setCloseModal(onClose: () => void) { - closeModal = onClose; +function setCloseModal(key: number, onClose: () => void) { + closeModals[key] = onClose; } /** * Close modal in other parts of the app */ 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; - } onModalClose = onModalCloseCallback; - closeModal(isNavigating); + const reversalOnCloses = Object.values(closeModals).reverse(); + reversalOnCloses.forEach((onClose) => { + if (typeof onClose !== 'function') { + return; + } + onClose(isNavigating); + }); } function onModalDidClose() { @@ -50,4 +58,4 @@ function willAlertModalBecomeVisible(isVisible: boolean) { Onyx.merge(ONYXKEYS.MODAL, {willAlertModalBecomeVisible: isVisible}); } -export {setCloseModal, close, onModalDidClose, setModalVisibility, willAlertModalBecomeVisible}; +export {setCloseModal, close, onModalDidClose, setModalVisibility, willAlertModalBecomeVisible, getAvailableKey}; From a342b162be83e198420409ae57f393ceabb44230 Mon Sep 17 00:00:00 2001 From: tienifr Date: Wed, 1 Nov 2023 16:34:42 +0700 Subject: [PATCH 2/7] refactor --- src/components/PopoverWithoutOverlay/index.js | 8 +++++--- src/libs/actions/Modal.ts | 16 ++++++++-------- 2 files changed, 13 insertions(+), 11 deletions(-) diff --git a/src/components/PopoverWithoutOverlay/index.js b/src/components/PopoverWithoutOverlay/index.js index 43156cd11827..243543231ef3 100644 --- a/src/components/PopoverWithoutOverlay/index.js +++ b/src/components/PopoverWithoutOverlay/index.js @@ -1,4 +1,4 @@ -import React from 'react'; +import React, {useRef} from 'react'; import {View} from 'react-native'; import {SafeAreaInsetsContext} from 'react-native-safe-area-context'; import {defaultProps, propTypes} from '@components/Popover/popoverPropTypes'; @@ -11,6 +11,8 @@ import * as Modal from '@userActions/Modal'; function Popover(props) { const {onOpen, close} = React.useContext(PopoverContext); + const onCloseKey = useRef(Modal.getAvailableKey()); + const {modalStyle, modalContainerStyle, shouldAddTopSafeAreaMargin, shouldAddBottomSafeAreaMargin, shouldAddTopSafeAreaPadding, shouldAddBottomSafeAreaPadding} = getModalStyles( 'popover', { @@ -30,8 +32,8 @@ function Popover(props) { ref: props.withoutOverlayRef, close: props.onClose, anchorRef: props.anchorRef, - onCloseCallback: () => Modal.setCloseModal(null), - onOpenCallback: () => Modal.setCloseModal(() => props.onClose(props.anchorRef)), + onCloseCallback: () => Modal.setCloseModal(onCloseKey, null), + onOpenCallback: () => Modal.setCloseModal(onCloseKey, () => props.onClose(props.anchorRef)), }); } else { props.onModalHide(); diff --git a/src/libs/actions/Modal.ts b/src/libs/actions/Modal.ts index 431142adfb20..b466423d537b 100644 --- a/src/libs/actions/Modal.ts +++ b/src/libs/actions/Modal.ts @@ -4,7 +4,6 @@ import ONYXKEYS from '@src/ONYXKEYS'; const closeModals: Record void)> = {}; let count = 0; -// let closeModal: (isNavigating: boolean) => void; let onModalClose: null | (() => void); /** @@ -26,13 +25,14 @@ function setCloseModal(key: number, onClose: () => void) { */ function close(onModalCloseCallback: () => void, isNavigating = true) { onModalClose = onModalCloseCallback; - const reversalOnCloses = Object.values(closeModals).reverse(); - reversalOnCloses.forEach((onClose) => { - if (typeof onClose !== 'function') { - return; - } - onClose(isNavigating); - }); + Object.values(closeModals) + .reverse() + .forEach((onClose) => { + if (typeof onClose !== 'function') { + return; + } + onClose(isNavigating); + }); } function onModalDidClose() { From 50df7a9ebb4c0566138a36b35ee0e4e41c0c4347 Mon Sep 17 00:00:00 2001 From: tienifr Date: Tue, 7 Nov 2023 15:36:57 +0700 Subject: [PATCH 3/7] change to use list --- src/components/Modal/BaseModal.js | 9 +++--- src/components/PopoverWithoutOverlay/index.js | 9 ++++-- src/libs/actions/Modal.ts | 32 +++++++++---------- 3 files changed, 27 insertions(+), 23 deletions(-) diff --git a/src/components/Modal/BaseModal.js b/src/components/Modal/BaseModal.js index b3e5b26b64ed..74a8a2ff7120 100644 --- a/src/components/Modal/BaseModal.js +++ b/src/components/Modal/BaseModal.js @@ -59,7 +59,7 @@ function BaseModal({ const isVisibleRef = useRef(isVisible); const wasVisible = usePrevious(isVisible); - const onCloseKey = useRef(Modal.getAvailableKey()); + const removeOnClose = useRef(() => {}); /** * Hides modal @@ -86,10 +86,11 @@ function BaseModal({ if (isVisible) { Modal.willAlertModalBecomeVisible(true); // To handle closing any modal already visible when this modal is mounted, i.e. PopoverReportActionContextMenu - Modal.setCloseModal(onCloseKey.current, onClose); + removeOnClose.current(); + removeOnClose.current = Modal.setCloseModal(onClose); } else if (wasVisible && !isVisible) { Modal.willAlertModalBecomeVisible(false); - Modal.setCloseModal(onCloseKey.current, null); + removeOnClose.current(); } }, [isVisible, wasVisible, onClose]); @@ -102,7 +103,7 @@ function BaseModal({ hideModal(true); Modal.willAlertModalBecomeVisible(false); // To prevent closing any modal already unmounted when this modal still remains as visible state - Modal.setCloseModal(onCloseKey.current, null); + removeOnClose.current(); }, // eslint-disable-next-line react-hooks/exhaustive-deps [], diff --git a/src/components/PopoverWithoutOverlay/index.js b/src/components/PopoverWithoutOverlay/index.js index 243543231ef3..94d15bc5b0a2 100644 --- a/src/components/PopoverWithoutOverlay/index.js +++ b/src/components/PopoverWithoutOverlay/index.js @@ -11,7 +11,7 @@ import * as Modal from '@userActions/Modal'; function Popover(props) { const {onOpen, close} = React.useContext(PopoverContext); - const onCloseKey = useRef(Modal.getAvailableKey()); + const removeOnClose = useRef(() => {}); const {modalStyle, modalContainerStyle, shouldAddTopSafeAreaMargin, shouldAddBottomSafeAreaMargin, shouldAddTopSafeAreaPadding, shouldAddBottomSafeAreaPadding} = getModalStyles( 'popover', @@ -32,8 +32,11 @@ function Popover(props) { ref: props.withoutOverlayRef, close: props.onClose, anchorRef: props.anchorRef, - onCloseCallback: () => Modal.setCloseModal(onCloseKey, null), - onOpenCallback: () => Modal.setCloseModal(onCloseKey, () => props.onClose(props.anchorRef)), + onCloseCallback: removeOnClose.current, + onOpenCallback: () => { + removeOnClose.current(); + removeOnClose.current = Modal.setCloseModal(() => props.onClose(props.anchorRef)); + }, }); } else { props.onModalHide(); diff --git a/src/libs/actions/Modal.ts b/src/libs/actions/Modal.ts index b466423d537b..e02063ff1750 100644 --- a/src/libs/actions/Modal.ts +++ b/src/libs/actions/Modal.ts @@ -1,23 +1,27 @@ import Onyx from 'react-native-onyx'; import ONYXKEYS from '@src/ONYXKEYS'; -const closeModals: Record void)> = {}; -let count = 0; +const closeModals: Array<(isNavigating: boolean) => void> = []; let onModalClose: null | (() => void); /** - * Get the available key that we can store the onClose callback into it + * Remove onClose function in list */ -function getAvailableKey() { - return count++; +function removeCloseModal(onClose: () => void) { + const index = closeModals.findIndex((o) => o === onClose); + if (index === -1) { + return; + } + closeModals.splice(index, 1); } /** * Allows other parts of the app to call modal close function */ -function setCloseModal(key: number, onClose: () => void) { - closeModals[key] = onClose; +function setCloseModal(onClose: () => void) { + closeModals.push(onClose); + return () => removeCloseModal(onClose); } /** @@ -25,14 +29,10 @@ function setCloseModal(key: number, onClose: () => void) { */ function close(onModalCloseCallback: () => void, isNavigating = true) { onModalClose = onModalCloseCallback; - Object.values(closeModals) - .reverse() - .forEach((onClose) => { - if (typeof onClose !== 'function') { - return; - } - onClose(isNavigating); - }); + const reverseCloseModals = [...closeModals].reverse(); + reverseCloseModals.forEach((onClose) => { + onClose(isNavigating); + }); } function onModalDidClose() { @@ -58,4 +58,4 @@ function willAlertModalBecomeVisible(isVisible: boolean) { Onyx.merge(ONYXKEYS.MODAL, {willAlertModalBecomeVisible: isVisible}); } -export {setCloseModal, close, onModalDidClose, setModalVisibility, willAlertModalBecomeVisible, getAvailableKey}; +export {setCloseModal, close, onModalDidClose, setModalVisibility, willAlertModalBecomeVisible}; From ab389fba5139bd98023b77a8da92da7a73af4155 Mon Sep 17 00:00:00 2001 From: tienifr Date: Wed, 8 Nov 2023 11:31:07 +0700 Subject: [PATCH 4/7] clean code --- src/components/Modal/BaseModal.js | 12 +++++++----- src/libs/actions/Modal.ts | 23 +++++++++++------------ 2 files changed, 18 insertions(+), 17 deletions(-) diff --git a/src/components/Modal/BaseModal.js b/src/components/Modal/BaseModal.js index 7a13d2254969..4154dd933a2c 100644 --- a/src/components/Modal/BaseModal.js +++ b/src/components/Modal/BaseModal.js @@ -59,7 +59,7 @@ function BaseModal({ const isVisibleRef = useRef(isVisible); const wasVisible = usePrevious(isVisible); - const removeOnClose = useRef(() => {}); + const removeOnCloseListener = useRef(() => {}); /** * Hides modal @@ -86,12 +86,14 @@ function BaseModal({ if (isVisible) { Modal.willAlertModalBecomeVisible(true); // To handle closing any modal already visible when this modal is mounted, i.e. PopoverReportActionContextMenu - removeOnClose.current(); - removeOnClose.current = Modal.setCloseModal(onClose); + removeOnCloseListener.current = Modal.setCloseModal(onClose); } else if (wasVisible && !isVisible) { Modal.willAlertModalBecomeVisible(false); - removeOnClose.current(); } + + return () => { + removeOnCloseListener.current(); + }; }, [isVisible, wasVisible, onClose]); useEffect( @@ -103,7 +105,7 @@ function BaseModal({ hideModal(true); Modal.willAlertModalBecomeVisible(false); // To prevent closing any modal already unmounted when this modal still remains as visible state - removeOnClose.current(); + removeOnCloseListener.current(); }, // eslint-disable-next-line react-hooks/exhaustive-deps [], diff --git a/src/libs/actions/Modal.ts b/src/libs/actions/Modal.ts index e02063ff1750..15447bf84ade 100644 --- a/src/libs/actions/Modal.ts +++ b/src/libs/actions/Modal.ts @@ -5,29 +5,28 @@ const closeModals: Array<(isNavigating: boolean) => void> = []; let onModalClose: null | (() => void); -/** - * Remove onClose function in list - */ -function removeCloseModal(onClose: () => void) { - const index = closeModals.findIndex((o) => o === onClose); - if (index === -1) { - return; - } - closeModals.splice(index, 1); -} - /** * Allows other parts of the app to call modal close function */ function setCloseModal(onClose: () => void) { closeModals.push(onClose); - return () => removeCloseModal(onClose); + return () => { + const index = closeModals.indexOf(onClose); + if (index === -1) { + return; + } + closeModals.splice(index, 1); + }; } /** * Close modal in other parts of the app */ function close(onModalCloseCallback: () => void, isNavigating = true) { + if (closeModals.length === 0) { + onModalCloseCallback(); + return; + } onModalClose = onModalCloseCallback; const reverseCloseModals = [...closeModals].reverse(); reverseCloseModals.forEach((onClose) => { From d81596b79dd8719bfee550fb5ff85dee738cf4d9 Mon Sep 17 00:00:00 2001 From: tienifr Date: Wed, 8 Nov 2023 11:33:59 +0700 Subject: [PATCH 5/7] prevent duplicate --- src/libs/actions/Modal.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/libs/actions/Modal.ts b/src/libs/actions/Modal.ts index 15447bf84ade..6981959c160a 100644 --- a/src/libs/actions/Modal.ts +++ b/src/libs/actions/Modal.ts @@ -9,7 +9,9 @@ let onModalClose: null | (() => void); * Allows other parts of the app to call modal close function */ function setCloseModal(onClose: () => void) { - closeModals.push(onClose); + if (!closeModals.includes(onClose)) { + closeModals.push(onClose); + } return () => { const index = closeModals.indexOf(onClose); if (index === -1) { From f58d0ba62e4f9a8d7bc66253ed355bfe0d0b84e9 Mon Sep 17 00:00:00 2001 From: tienifr Date: Thu, 9 Nov 2023 15:28:52 +0700 Subject: [PATCH 6/7] clean code --- src/components/Modal/BaseModal.js | 12 ++++++------ src/components/PopoverWithoutOverlay/index.js | 16 +++++++++------- 2 files changed, 15 insertions(+), 13 deletions(-) diff --git a/src/components/Modal/BaseModal.js b/src/components/Modal/BaseModal.js index 4154dd933a2c..b57fad3a8d49 100644 --- a/src/components/Modal/BaseModal.js +++ b/src/components/Modal/BaseModal.js @@ -59,8 +59,6 @@ function BaseModal({ const isVisibleRef = useRef(isVisible); const wasVisible = usePrevious(isVisible); - const removeOnCloseListener = useRef(() => {}); - /** * Hides modal * @param {Boolean} [callHideCallback=true] Should we call the onModalHide callback @@ -83,16 +81,20 @@ function BaseModal({ useEffect(() => { isVisibleRef.current = isVisible; + let removeOnCloseListener; if (isVisible) { Modal.willAlertModalBecomeVisible(true); // To handle closing any modal already visible when this modal is mounted, i.e. PopoverReportActionContextMenu - removeOnCloseListener.current = Modal.setCloseModal(onClose); + removeOnCloseListener = Modal.setCloseModal(onClose); } else if (wasVisible && !isVisible) { Modal.willAlertModalBecomeVisible(false); } return () => { - removeOnCloseListener.current(); + if (!removeOnCloseListener) { + return; + } + removeOnCloseListener(); }; }, [isVisible, wasVisible, onClose]); @@ -104,8 +106,6 @@ function BaseModal({ } hideModal(true); Modal.willAlertModalBecomeVisible(false); - // To prevent closing any modal already unmounted when this modal still remains as visible state - removeOnCloseListener.current(); }, // eslint-disable-next-line react-hooks/exhaustive-deps [], diff --git a/src/components/PopoverWithoutOverlay/index.js b/src/components/PopoverWithoutOverlay/index.js index 94d15bc5b0a2..b7ba41116daa 100644 --- a/src/components/PopoverWithoutOverlay/index.js +++ b/src/components/PopoverWithoutOverlay/index.js @@ -1,4 +1,4 @@ -import React, {useRef} from 'react'; +import React from 'react'; import {View} from 'react-native'; import {SafeAreaInsetsContext} from 'react-native-safe-area-context'; import {defaultProps, propTypes} from '@components/Popover/popoverPropTypes'; @@ -11,7 +11,6 @@ import * as Modal from '@userActions/Modal'; function Popover(props) { const {onOpen, close} = React.useContext(PopoverContext); - const removeOnClose = useRef(() => {}); const {modalStyle, modalContainerStyle, shouldAddTopSafeAreaMargin, shouldAddBottomSafeAreaMargin, shouldAddTopSafeAreaPadding, shouldAddBottomSafeAreaPadding} = getModalStyles( 'popover', @@ -26,18 +25,15 @@ function Popover(props) { ); React.useEffect(() => { + let removeOnClose; if (props.isVisible) { props.onModalShow(); onOpen({ ref: props.withoutOverlayRef, close: props.onClose, anchorRef: props.anchorRef, - onCloseCallback: removeOnClose.current, - onOpenCallback: () => { - removeOnClose.current(); - removeOnClose.current = Modal.setCloseModal(() => props.onClose(props.anchorRef)); - }, }); + removeOnClose = Modal.setCloseModal(() => props.onClose(props.anchorRef)); } else { props.onModalHide(); close(props.anchorRef); @@ -45,6 +41,12 @@ function Popover(props) { } Modal.willAlertModalBecomeVisible(props.isVisible); + return () => { + if (!removeOnClose) { + return; + } + removeOnClose(); + }; // We want this effect to run strictly ONLY when isVisible prop changes // eslint-disable-next-line react-hooks/exhaustive-deps }, [props.isVisible]); From 2f0e1ee00ee0d3c31c548bb273e6f16e69c8874d Mon Sep 17 00:00:00 2001 From: tienifr Date: Fri, 17 Nov 2023 11:54:42 +0700 Subject: [PATCH 7/7] add type check --- src/components/Modal/BaseModal.tsx | 2 +- src/components/PopoverWithoutOverlay/index.js | 1 - src/libs/actions/Modal.ts | 3 +-- 3 files changed, 2 insertions(+), 4 deletions(-) diff --git a/src/components/Modal/BaseModal.tsx b/src/components/Modal/BaseModal.tsx index 7cd71d410604..54a178db1cdd 100644 --- a/src/components/Modal/BaseModal.tsx +++ b/src/components/Modal/BaseModal.tsx @@ -72,7 +72,7 @@ function BaseModal( useEffect(() => { isVisibleRef.current = isVisible; - let removeOnCloseListener; + let removeOnCloseListener: () => void; if (isVisible) { Modal.willAlertModalBecomeVisible(true); // To handle closing any modal already visible when this modal is mounted, i.e. PopoverReportActionContextMenu diff --git a/src/components/PopoverWithoutOverlay/index.js b/src/components/PopoverWithoutOverlay/index.js index d568f974c4f6..8b9dd4ac7a61 100644 --- a/src/components/PopoverWithoutOverlay/index.js +++ b/src/components/PopoverWithoutOverlay/index.js @@ -12,7 +12,6 @@ import * as Modal from '@userActions/Modal'; function Popover(props) { const styles = useThemeStyles(); const {onOpen, close} = React.useContext(PopoverContext); - const {modalStyle, modalContainerStyle, shouldAddTopSafeAreaMargin, shouldAddBottomSafeAreaMargin, shouldAddTopSafeAreaPadding, shouldAddBottomSafeAreaPadding} = getModalStyles( 'popover', { diff --git a/src/libs/actions/Modal.ts b/src/libs/actions/Modal.ts index 6981959c160a..e1e73d425281 100644 --- a/src/libs/actions/Modal.ts +++ b/src/libs/actions/Modal.ts @@ -30,8 +30,7 @@ function close(onModalCloseCallback: () => void, isNavigating = true) { return; } onModalClose = onModalCloseCallback; - const reverseCloseModals = [...closeModals].reverse(); - reverseCloseModals.forEach((onClose) => { + [...closeModals].reverse().forEach((onClose) => { onClose(isNavigating); }); }