From 3ae6ffae5dbd63e7eeb590cdad669e1735376af2 Mon Sep 17 00:00:00 2001 From: Julian Kobrynski Date: Tue, 22 Aug 2023 15:09:00 +0200 Subject: [PATCH 1/6] rewrite BaseModal to a functional component --- src/components/Modal/BaseModal.js | 309 ++++++++++++++++-------------- 1 file changed, 163 insertions(+), 146 deletions(-) diff --git a/src/components/Modal/BaseModal.js b/src/components/Modal/BaseModal.js index 80ba1fc60344..1fae493a58fa 100644 --- a/src/components/Modal/BaseModal.js +++ b/src/components/Modal/BaseModal.js @@ -1,14 +1,15 @@ -import React, {PureComponent} from 'react'; +import React, {forwardRef, useEffect} from 'react'; import {View} from 'react-native'; import PropTypes from 'prop-types'; import ReactNativeModal from 'react-native-modal'; -import {SafeAreaInsetsContext} from 'react-native-safe-area-context'; +import {useSafeAreaInsets} from 'react-native-safe-area-context'; import styles from '../../styles/styles'; +import * as Modal from '../../libs/actions/Modal'; import * as StyleUtils from '../../styles/StyleUtils'; import themeColors from '../../styles/themes/default'; import {propTypes as modalPropTypes, defaultProps as modalDefaultProps} from './modalPropTypes'; -import * as Modal from '../../libs/actions/Modal'; import getModalStyles from '../../styles/getModalStyles'; +import useWindowDimensions from '../../hooks/useWindowDimensions'; import variables from '../../styles/variables'; const propTypes = { @@ -23,166 +24,182 @@ const defaultProps = { forwardedRef: () => {}, }; -class BaseModal extends PureComponent { - constructor(props) { - super(props); +function BaseModal({ + isVisible, + onClose, + shouldSetModalVisibility, + onModalHide, + type, + popoverAnchorPosition, + innerContainerStyle, + outerStyle, + onModalShow, + propagateSwipe, + fullscreen, + animationIn, + animationOut, + useNativeDriver, + hideModalContentWhileAnimating, + animationInTiming, + animationOutTiming, + statusBarTranslucent, + onLayout, + avoidKeyboard, + forwardedRef, + children, +}) { + const {windowWidth, windowHeight, isSmallScreenWidth} = useWindowDimensions(); + + const safeAreaInsets = useSafeAreaInsets(); - this.hideModal = this.hideModal.bind(this); - } + /** + * Hides modal + * @param {Boolean} [callHideCallback=true] Should we call the onModalHide callback + */ + const hideModal = (callHideCallback = true) => { + if (shouldSetModalVisibility) { + Modal.setModalVisibility(false); + } + if (callHideCallback) { + onModalHide(); + } + Modal.onModalDidClose(); + }; - componentDidMount() { - if (!this.props.isVisible) { + useEffect(() => { + if (!isVisible) { return; } Modal.willAlertModalBecomeVisible(true); // 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.setCloseModal(onClose); + + return () => { + // Only trigger onClose and setModalVisibility if the modal is unmounting while visible. + if (isVisible) { + hideModal(true); + Modal.willAlertModalBecomeVisible(false); + } + + // To prevent closing any modal already unmounted when this modal still remains as visible state + Modal.setCloseModal(null); + }; + // eslint-disable-next-line react-hooks/exhaustive-deps -- this effect only needs to run once on mount. It's an equivalent of componentDidMount and componentWillUnmount from the original class component + }, []); + + useEffect(() => { + Modal.willAlertModalBecomeVisible(isVisible); + Modal.setCloseModal(isVisible ? onClose : null); + }, [isVisible, onClose]); + + const handleShowModal = () => { + if (shouldSetModalVisibility) { + Modal.setModalVisibility(true); } + onModalShow(); + }; - Modal.willAlertModalBecomeVisible(this.props.isVisible); - Modal.setCloseModal(this.props.isVisible ? this.props.onClose : null); - } - - componentWillUnmount() { - // Only trigger onClose and setModalVisibility if the modal is unmounting while visible. - if (this.props.isVisible) { - this.hideModal(true); - Modal.willAlertModalBecomeVisible(false); - } - - // To prevent closing any modal already unmounted when this modal still remains as visible state - Modal.setCloseModal(null); - } - - /** - * Hides modal - * @param {Boolean} [callHideCallback=true] Should we call the onModalHide callback - */ - hideModal(callHideCallback = true) { - if (this.props.shouldSetModalVisibility) { - Modal.setModalVisibility(false); - } - if (callHideCallback) { - this.props.onModalHide(); + const handleBackdropPress = (e) => { + if (e && e.key === 'Enter') { + return; } - Modal.onModalDidClose(); - } - - render() { - const { - modalStyle, - modalContainerStyle, - swipeDirection, - animationIn, - animationOut, - shouldAddTopSafeAreaMargin, - shouldAddBottomSafeAreaMargin, - shouldAddTopSafeAreaPadding, - shouldAddBottomSafeAreaPadding, - hideBackdrop, - } = getModalStyles( - this.props.type, - { - windowWidth: this.props.windowWidth, - windowHeight: this.props.windowHeight, - isSmallScreenWidth: this.props.isSmallScreenWidth, - }, - this.props.popoverAnchorPosition, - this.props.innerContainerStyle, - this.props.outerStyle, - ); - return ( - { - if (e && e.key === 'Enter') { - return; - } - this.props.onClose(); - }} - // Note: Escape key on web/desktop will trigger onBackButtonPress callback - // eslint-disable-next-line react/jsx-props-no-multi-spaces - onBackButtonPress={this.props.onClose} - onModalShow={() => { - if (this.props.shouldSetModalVisibility) { - Modal.setModalVisibility(true); - } - this.props.onModalShow(); - }} - propagateSwipe={this.props.propagateSwipe} - onModalHide={this.hideModal} - onSwipeComplete={this.props.onClose} - swipeDirection={swipeDirection} - isVisible={this.props.isVisible} - backdropColor={themeColors.overlay} - backdropOpacity={hideBackdrop ? 0 : variables.overlayOpacity} - backdropTransitionOutTiming={0} - hasBackdrop={this.props.fullscreen} - coverScreen={this.props.fullscreen} - style={modalStyle} - deviceHeight={this.props.windowHeight} - deviceWidth={this.props.windowWidth} - animationIn={this.props.animationIn || animationIn} - animationOut={this.props.animationOut || animationOut} - useNativeDriver={this.props.useNativeDriver} - hideModalContentWhileAnimating={this.props.hideModalContentWhileAnimating} - animationInTiming={this.props.animationInTiming} - animationOutTiming={this.props.animationOutTiming} - statusBarTranslucent={this.props.statusBarTranslucent} - onLayout={this.props.onLayout} - avoidKeyboard={this.props.avoidKeyboard} + onClose(); + }; + + const { + modalStyle, + modalContainerStyle, + swipeDirection, + animationIn: modalStyleAnimationIn, + animationOut: modalStyleAnimationOut, + shouldAddTopSafeAreaMargin, + shouldAddBottomSafeAreaMargin, + shouldAddTopSafeAreaPadding, + shouldAddBottomSafeAreaPadding, + hideBackdrop, + } = getModalStyles( + type, + { + windowWidth, + windowHeight, + isSmallScreenWidth, + }, + popoverAnchorPosition, + innerContainerStyle, + outerStyle, + ); + + const { + paddingTop: safeAreaPaddingTop, + paddingBottom: safeAreaPaddingBottom, + paddingLeft: safeAreaPaddingLeft, + paddingRight: safeAreaPaddingRight, + } = StyleUtils.getSafeAreaPadding(safeAreaInsets); + + const modalPaddingStyles = StyleUtils.getModalPaddingStyles({ + safeAreaPaddingTop, + safeAreaPaddingBottom, + safeAreaPaddingLeft, + safeAreaPaddingRight, + shouldAddBottomSafeAreaMargin, + shouldAddTopSafeAreaMargin, + shouldAddBottomSafeAreaPadding, + shouldAddTopSafeAreaPadding, + modalContainerStyleMarginTop: modalContainerStyle.marginTop, + modalContainerStyleMarginBottom: modalContainerStyle.marginBottom, + modalContainerStylePaddingTop: modalContainerStyle.paddingTop, + modalContainerStylePaddingBottom: modalContainerStyle.paddingBottom, + insets: safeAreaInsets, + }); + + return ( + + - - {(insets) => { - const { - paddingTop: safeAreaPaddingTop, - paddingBottom: safeAreaPaddingBottom, - paddingLeft: safeAreaPaddingLeft, - paddingRight: safeAreaPaddingRight, - } = StyleUtils.getSafeAreaPadding(insets); - - const modalPaddingStyles = StyleUtils.getModalPaddingStyles({ - safeAreaPaddingTop, - safeAreaPaddingBottom, - safeAreaPaddingLeft, - safeAreaPaddingRight, - shouldAddBottomSafeAreaMargin, - shouldAddTopSafeAreaMargin, - shouldAddBottomSafeAreaPadding, - shouldAddTopSafeAreaPadding, - modalContainerStyleMarginTop: modalContainerStyle.marginTop, - modalContainerStyleMarginBottom: modalContainerStyle.marginBottom, - modalContainerStylePaddingTop: modalContainerStyle.paddingTop, - modalContainerStylePaddingBottom: modalContainerStyle.paddingBottom, - insets, - }); - - return ( - - {this.props.children} - - ); - }} - - - ); - } + {children} + + + ); } BaseModal.propTypes = propTypes; BaseModal.defaultProps = defaultProps; +BaseModal.displayName = 'BaseModal'; -export default React.forwardRef((props, ref) => ( +export default forwardRef((props, ref) => ( Date: Wed, 23 Aug 2023 09:25:18 +0200 Subject: [PATCH 2/6] apply minor style changes --- src/components/Modal/BaseModal.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/components/Modal/BaseModal.js b/src/components/Modal/BaseModal.js index 1fae493a58fa..cdb7722fa1a2 100644 --- a/src/components/Modal/BaseModal.js +++ b/src/components/Modal/BaseModal.js @@ -11,6 +11,7 @@ import {propTypes as modalPropTypes, defaultProps as modalDefaultProps} from './ import getModalStyles from '../../styles/getModalStyles'; import useWindowDimensions from '../../hooks/useWindowDimensions'; import variables from '../../styles/variables'; +import CONST from '../../CONST'; const propTypes = { ...modalPropTypes, @@ -102,7 +103,7 @@ function BaseModal({ }; const handleBackdropPress = (e) => { - if (e && e.key === 'Enter') { + if (e && e.key === CONST.KEYBOARD_SHORTCUTS.ENTER.shortcutKey) { return; } onClose(); @@ -185,7 +186,7 @@ function BaseModal({ avoidKeyboard={avoidKeyboard} > From d2f23313e55718d3f3a1bd24726299f9abec9019 Mon Sep 17 00:00:00 2001 From: Julian Kobrynski Date: Wed, 23 Aug 2023 09:35:31 +0200 Subject: [PATCH 3/6] wrap getModalStyles with useMemo --- src/components/Modal/BaseModal.js | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/src/components/Modal/BaseModal.js b/src/components/Modal/BaseModal.js index cdb7722fa1a2..aaaf023aa094 100644 --- a/src/components/Modal/BaseModal.js +++ b/src/components/Modal/BaseModal.js @@ -1,4 +1,4 @@ -import React, {forwardRef, useEffect} from 'react'; +import React, {forwardRef, useEffect, useMemo} from 'react'; import {View} from 'react-native'; import PropTypes from 'prop-types'; import ReactNativeModal from 'react-native-modal'; @@ -120,16 +120,20 @@ function BaseModal({ shouldAddTopSafeAreaPadding, shouldAddBottomSafeAreaPadding, hideBackdrop, - } = getModalStyles( - type, - { - windowWidth, - windowHeight, - isSmallScreenWidth, - }, - popoverAnchorPosition, - innerContainerStyle, - outerStyle, + } = useMemo( + () => + getModalStyles( + type, + { + windowWidth, + windowHeight, + isSmallScreenWidth, + }, + popoverAnchorPosition, + innerContainerStyle, + outerStyle, + ), + [innerContainerStyle, isSmallScreenWidth, outerStyle, popoverAnchorPosition, type, windowHeight, windowWidth], ); const { From 379107ff5b5ec65dbb78032d181b62c1c240c78c Mon Sep 17 00:00:00 2001 From: Julian Kobrynski Date: Wed, 23 Aug 2023 10:47:30 +0200 Subject: [PATCH 4/6] modify dependency arrays, merge two useEffects into one --- src/components/Modal/BaseModal.js | 40 +++++++++++++------------------ 1 file changed, 17 insertions(+), 23 deletions(-) diff --git a/src/components/Modal/BaseModal.js b/src/components/Modal/BaseModal.js index aaaf023aa094..2fb883b9e54a 100644 --- a/src/components/Modal/BaseModal.js +++ b/src/components/Modal/BaseModal.js @@ -1,4 +1,4 @@ -import React, {forwardRef, useEffect, useMemo} from 'react'; +import React, {forwardRef, useCallback, useEffect, useMemo} from 'react'; import {View} from 'react-native'; import PropTypes from 'prop-types'; import ReactNativeModal from 'react-native-modal'; @@ -57,25 +57,25 @@ function BaseModal({ * Hides modal * @param {Boolean} [callHideCallback=true] Should we call the onModalHide callback */ - const hideModal = (callHideCallback = true) => { - if (shouldSetModalVisibility) { - Modal.setModalVisibility(false); - } - if (callHideCallback) { - onModalHide(); - } - Modal.onModalDidClose(); - }; + const hideModal = useCallback( + (callHideCallback = true) => { + if (shouldSetModalVisibility) { + Modal.setModalVisibility(false); + } + if (callHideCallback) { + onModalHide(); + } + Modal.onModalDidClose(); + // eslint-disable-next-line react-hooks/exhaustive-deps -- adding onModalHide to the dependency array causes too many unnecessary rerenders + }, + [shouldSetModalVisibility], + ); useEffect(() => { - if (!isVisible) { - return; - } - - Modal.willAlertModalBecomeVisible(true); + Modal.willAlertModalBecomeVisible(isVisible); // To handle closing any modal already visible when this modal is mounted, i.e. PopoverReportActionContextMenu - Modal.setCloseModal(onClose); + Modal.setCloseModal(isVisible ? onClose : null); return () => { // Only trigger onClose and setModalVisibility if the modal is unmounting while visible. @@ -87,13 +87,7 @@ function BaseModal({ // To prevent closing any modal already unmounted when this modal still remains as visible state Modal.setCloseModal(null); }; - // eslint-disable-next-line react-hooks/exhaustive-deps -- this effect only needs to run once on mount. It's an equivalent of componentDidMount and componentWillUnmount from the original class component - }, []); - - useEffect(() => { - Modal.willAlertModalBecomeVisible(isVisible); - Modal.setCloseModal(isVisible ? onClose : null); - }, [isVisible, onClose]); + }, [hideModal, isVisible, onClose]); const handleShowModal = () => { if (shouldSetModalVisibility) { From 5fd5ee1c9752cf0b686cc3a12ea695daea5d8179 Mon Sep 17 00:00:00 2001 From: Julian Kobrynski Date: Wed, 23 Aug 2023 11:13:53 +0200 Subject: [PATCH 5/6] fix eslint disable placement --- src/components/Modal/BaseModal.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/Modal/BaseModal.js b/src/components/Modal/BaseModal.js index 2fb883b9e54a..0d7fcaa74cf6 100644 --- a/src/components/Modal/BaseModal.js +++ b/src/components/Modal/BaseModal.js @@ -66,8 +66,8 @@ function BaseModal({ onModalHide(); } Modal.onModalDidClose(); - // eslint-disable-next-line react-hooks/exhaustive-deps -- adding onModalHide to the dependency array causes too many unnecessary rerenders }, + // eslint-disable-next-line react-hooks/exhaustive-deps -- adding onModalHide to the dependency array causes too many unnecessary rerenders [shouldSetModalVisibility], ); From 7423e6858d8012157f8aef656333473e63aa833c Mon Sep 17 00:00:00 2001 From: Julian Kobrynski Date: Fri, 25 Aug 2023 13:23:39 +0200 Subject: [PATCH 6/6] format code --- src/components/Modal/BaseModal.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/components/Modal/BaseModal.js b/src/components/Modal/BaseModal.js index 5497641eb2ac..8035eca3d96a 100644 --- a/src/components/Modal/BaseModal.js +++ b/src/components/Modal/BaseModal.js @@ -108,8 +108,8 @@ function BaseModal({ }; const handleDismissModal = () => { - ComposerFocusManager.setReadyToFocus() - } + ComposerFocusManager.setReadyToFocus(); + }; const { modalStyle,