From cc61881b3647d4f1e33dcab89b58068331d5c7a5 Mon Sep 17 00:00:00 2001 From: Alex D Date: Fri, 14 Jul 2023 09:15:50 +0200 Subject: [PATCH 01/12] Component refactor: PopoverWithMeasuredContent --- src/components/PopoverWithMeasuredContent.js | 151 ++++++++----------- 1 file changed, 64 insertions(+), 87 deletions(-) diff --git a/src/components/PopoverWithMeasuredContent.js b/src/components/PopoverWithMeasuredContent.js index 438e6cce010e..57bd75a7297b 100644 --- a/src/components/PopoverWithMeasuredContent.js +++ b/src/components/PopoverWithMeasuredContent.js @@ -1,11 +1,12 @@ import _ from 'underscore'; -import React, {Component} from 'react'; +import React, {useState, useEffect} from 'react'; import PropTypes from 'prop-types'; import {View} from 'react-native'; import lodashGet from 'lodash/get'; import Popover from './Popover'; import {propTypes as popoverPropTypes, defaultProps as defaultPopoverProps} from './Popover/popoverPropTypes'; -import withWindowDimensions, {windowDimensionsPropTypes} from './withWindowDimensions'; +import useWindowDimensions from '../hooks/useWindowDimensions'; +import {windowDimensionsPropTypes} from './withWindowDimensions'; import CONST from '../CONST'; import styles from '../styles/styles'; import {computeHorizontalShift, computeVerticalShift} from '../styles/getPopoverWithMeasuredContentStyles'; @@ -13,7 +14,8 @@ import {computeHorizontalShift, computeVerticalShift} from '../styles/getPopover const propTypes = { // All popover props except: // 1) anchorPosition (which is overridden for this component) - ..._.omit(popoverPropTypes, ['anchorPosition']), + // 2) windowDimensionsPropTypes (which are replaced by useWindowDimensions) + ..._.omit(popoverPropTypes, ['anchorPosition', ..._.keys(windowDimensionsPropTypes)]), /** The horizontal and vertical anchors points for the popover */ anchorPosition: PropTypes.shape({ @@ -35,8 +37,6 @@ const propTypes = { height: PropTypes.number, width: PropTypes.number, }), - - ...windowDimensionsPropTypes, }; const defaultProps = { @@ -59,138 +59,115 @@ const defaultProps = { * This way, we can shift the position of popover so that the content is anchored where we want it relative to the * anchor position. */ -class PopoverWithMeasuredContent extends Component { - constructor(props) { - super(props); - - this.popoverWidth = lodashGet(this.props, 'popoverDimensions.width', 0); - this.popoverHeight = lodashGet(this.props, 'popoverDimensions.height', 0); - this.state = { - isContentMeasured: this.popoverWidth > 0 && this.popoverHeight > 0, - isVisible: false, - }; - - this.measurePopover = this.measurePopover.bind(this); - } +function PopoverWithMeasuredContent(props) { + const {windowWidth, windowHeight} = useWindowDimensions(); + const [popoverWidth, setPopoverWidth] = useState(props.popoverDimensions.width); + const [popoverHeight, setPopoverHeight] = useState(props.popoverDimensions.height); + const [isContentMeasured, SetIsContentMeasured] = useState(popoverWidth > 0 && popoverHeight > 0); + const [isVisible, SetIsVisible] = useState(false); /** * When Popover becomes visible, we need to recalculate the Dimensions. * Skip render on Popover until recalculations have done by setting isContentMeasured false as early as possible. - * - * @static - * @param {Object} props - * @param {Object} state - * @return {Object|null} */ - static getDerivedStateFromProps(props, state) { + useEffect(() => { // When Popover is shown recalculate - if (!state.isVisible && props.isVisible) { - return {isContentMeasured: lodashGet(props, 'popoverDimensions.width', 0) > 0 && lodashGet(props, 'popoverDimensions.height', 0) > 0, isVisible: true}; + if (!isVisible && props.isVisible) { + SetIsContentMeasured(lodashGet(props, 'popoverDimensions.width', 0) > 0 && lodashGet(props, 'popoverDimensions.height', 0) > 0); + SetIsVisible(true); } if (!props.isVisible) { - return {isVisible: false}; + SetIsVisible(false); } - return null; - } - - shouldComponentUpdate(nextProps, nextState) { - if (this.props.isVisible && (nextProps.windowWidth !== this.props.windowWidth || nextProps.windowHeight !== this.props.windowHeight)) { - return true; - } - - // This component does not require re-render until any prop or state changes as we get the necessary info - // at first render. This component is attached to each message on the Chat list thus we prevent its re-renders - return !_.isEqual(_.omit(this.props, ['windowWidth', 'windowHeight']), _.omit(nextProps, ['windowWidth', 'windowHeight'])) || !_.isEqual(this.state, nextState); - } + }, [props, isVisible]); /** * Measure the size of the popover's content. * * @param {Object} nativeEvent */ - measurePopover({nativeEvent}) { - this.popoverWidth = nativeEvent.layout.width; - this.popoverHeight = nativeEvent.layout.height; - this.setState({isContentMeasured: true}); - } + const measurePopover = ({nativeEvent}) => { + setPopoverWidth(nativeEvent.layout.width); + setPopoverHeight(nativeEvent.layout.height); + SetIsContentMeasured(true); + }; /** * Calculate the adjusted position of the popover. * * @returns {Object} */ - calculateAdjustedAnchorPosition() { + const calculateAdjustedAnchorPosition = () => { let horizontalConstraint; - switch (this.props.anchorAlignment.horizontal) { + switch (props.anchorAlignment.horizontal) { case CONST.MODAL.ANCHOR_ORIGIN_HORIZONTAL.RIGHT: - horizontalConstraint = {left: this.props.anchorPosition.horizontal - this.popoverWidth}; + horizontalConstraint = {left: props.anchorPosition.horizontal - popoverWidth}; break; case CONST.MODAL.ANCHOR_ORIGIN_HORIZONTAL.CENTER: horizontalConstraint = { - left: Math.floor(this.props.anchorPosition.horizontal - this.popoverWidth / 2), + left: Math.floor(props.anchorPosition.horizontal - popoverWidth / 2), }; break; case CONST.MODAL.ANCHOR_ORIGIN_HORIZONTAL.LEFT: default: - horizontalConstraint = {left: this.props.anchorPosition.horizontal}; + horizontalConstraint = {left: props.anchorPosition.horizontal}; } let verticalConstraint; - switch (this.props.anchorAlignment.vertical) { + switch (props.anchorAlignment.vertical) { case CONST.MODAL.ANCHOR_ORIGIN_VERTICAL.BOTTOM: - verticalConstraint = {top: this.props.anchorPosition.vertical - this.popoverHeight}; + verticalConstraint = {top: props.anchorPosition.vertical - popoverHeight}; break; case CONST.MODAL.ANCHOR_ORIGIN_VERTICAL.CENTER: verticalConstraint = { - top: Math.floor(this.props.anchorPosition.vertical - this.popoverHeight / 2), + top: Math.floor(props.anchorPosition.vertical - popoverHeight / 2), }; break; case CONST.MODAL.ANCHOR_ORIGIN_VERTICAL.TOP: default: - verticalConstraint = {top: this.props.anchorPosition.vertical}; + verticalConstraint = {top: props.anchorPosition.vertical}; } return { ...horizontalConstraint, ...verticalConstraint, }; - } - - render() { - const adjustedAnchorPosition = this.calculateAdjustedAnchorPosition(); - const horizontalShift = computeHorizontalShift(adjustedAnchorPosition.left, this.popoverWidth, this.props.windowWidth); - const verticalShift = computeVerticalShift(adjustedAnchorPosition.top, this.popoverHeight, this.props.windowHeight); - const shiftedAnchorPosition = { - left: adjustedAnchorPosition.left + horizontalShift, - top: adjustedAnchorPosition.top + verticalShift, - }; - return this.state.isContentMeasured ? ( - - {this.props.children} - - ) : ( - /* - This is an invisible view used to measure the size of the popover, - before it ever needs to be displayed. - We do this because we need to know its dimensions in order to correctly animate the popover, - but we can't measure its dimensions without first rendering it. - */ - - {this.props.children} - - ); - } + }; + + const adjustedAnchorPosition = calculateAdjustedAnchorPosition(); + const horizontalShift = computeHorizontalShift(adjustedAnchorPosition.left, popoverWidth, windowWidth); + const verticalShift = computeVerticalShift(adjustedAnchorPosition.top, popoverHeight, windowHeight); + const shiftedAnchorPosition = { + left: adjustedAnchorPosition.left + horizontalShift, + top: adjustedAnchorPosition.top + verticalShift, + }; + return isContentMeasured ? ( + + {props.children} + + ) : ( + /* + This is an invisible view used to measure the size of the popover, + before it ever needs to be displayed. + We do this because we need to know its dimensions in order to correctly animate the popover, + but we can't measure its dimensions without first rendering it. + */ + + {props.children} + + ); } PopoverWithMeasuredContent.propTypes = propTypes; PopoverWithMeasuredContent.defaultProps = defaultProps; +PopoverWithMeasuredContent.displayName = 'PopoverWithMeasuredContent'; -export default withWindowDimensions(PopoverWithMeasuredContent); +export default React.memo(PopoverWithMeasuredContent, (prevProps, nextProps) => prevProps.isVisible && !_.isEqual(prevProps, nextProps)); From b91913b5c528e156c394a0348c4fa68d79cd9f37 Mon Sep 17 00:00:00 2001 From: Aleksei Dvoretskii Date: Fri, 14 Jul 2023 13:49:40 -0700 Subject: [PATCH 02/12] Update src/components/PopoverWithMeasuredContent.js Co-authored-by: Carlos Martins --- src/components/PopoverWithMeasuredContent.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/PopoverWithMeasuredContent.js b/src/components/PopoverWithMeasuredContent.js index 57bd75a7297b..5fd4dbb218e8 100644 --- a/src/components/PopoverWithMeasuredContent.js +++ b/src/components/PopoverWithMeasuredContent.js @@ -64,7 +64,7 @@ function PopoverWithMeasuredContent(props) { const {windowWidth, windowHeight} = useWindowDimensions(); const [popoverWidth, setPopoverWidth] = useState(props.popoverDimensions.width); const [popoverHeight, setPopoverHeight] = useState(props.popoverDimensions.height); - const [isContentMeasured, SetIsContentMeasured] = useState(popoverWidth > 0 && popoverHeight > 0); + const [isContentMeasured, setIsContentMeasured] = useState(popoverWidth > 0 && popoverHeight > 0); const [isVisible, SetIsVisible] = useState(false); /** From 320dc97071e0cc959f6d22d53ca3215eb5829818 Mon Sep 17 00:00:00 2001 From: Aleksei Dvoretskii Date: Fri, 14 Jul 2023 13:49:47 -0700 Subject: [PATCH 03/12] Update src/components/PopoverWithMeasuredContent.js Co-authored-by: Carlos Martins --- src/components/PopoverWithMeasuredContent.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/PopoverWithMeasuredContent.js b/src/components/PopoverWithMeasuredContent.js index 5fd4dbb218e8..5268837993c0 100644 --- a/src/components/PopoverWithMeasuredContent.js +++ b/src/components/PopoverWithMeasuredContent.js @@ -65,7 +65,7 @@ function PopoverWithMeasuredContent(props) { const [popoverWidth, setPopoverWidth] = useState(props.popoverDimensions.width); const [popoverHeight, setPopoverHeight] = useState(props.popoverDimensions.height); const [isContentMeasured, setIsContentMeasured] = useState(popoverWidth > 0 && popoverHeight > 0); - const [isVisible, SetIsVisible] = useState(false); + const [isVisible, setIsVisible] = useState(false); /** * When Popover becomes visible, we need to recalculate the Dimensions. From bd391647cccd46919f0c61a3af28f06fddc7b391 Mon Sep 17 00:00:00 2001 From: Alex D Date: Sat, 15 Jul 2023 01:39:21 +0300 Subject: [PATCH 04/12] Fix hook typo, fix React.memo, minor fixes --- src/components/PopoverWithMeasuredContent.js | 62 +++++++++++--------- 1 file changed, 33 insertions(+), 29 deletions(-) diff --git a/src/components/PopoverWithMeasuredContent.js b/src/components/PopoverWithMeasuredContent.js index 5268837993c0..1983056243d4 100644 --- a/src/components/PopoverWithMeasuredContent.js +++ b/src/components/PopoverWithMeasuredContent.js @@ -1,12 +1,12 @@ import _ from 'underscore'; -import React, {useState, useEffect} from 'react'; +import React, {useState, useEffect, useRef, useMemo} from 'react'; import PropTypes from 'prop-types'; import {View} from 'react-native'; import lodashGet from 'lodash/get'; import Popover from './Popover'; import {propTypes as popoverPropTypes, defaultProps as defaultPopoverProps} from './Popover/popoverPropTypes'; import useWindowDimensions from '../hooks/useWindowDimensions'; -import {windowDimensionsPropTypes} from './withWindowDimensions'; +import withWindowDimensions, {windowDimensionsPropTypes} from './withWindowDimensions'; import CONST from '../CONST'; import styles from '../styles/styles'; import {computeHorizontalShift, computeVerticalShift} from '../styles/getPopoverWithMeasuredContentStyles'; @@ -14,8 +14,7 @@ import {computeHorizontalShift, computeVerticalShift} from '../styles/getPopover const propTypes = { // All popover props except: // 1) anchorPosition (which is overridden for this component) - // 2) windowDimensionsPropTypes (which are replaced by useWindowDimensions) - ..._.omit(popoverPropTypes, ['anchorPosition', ..._.keys(windowDimensionsPropTypes)]), + ..._.omit(popoverPropTypes, ['anchorPosition']), /** The horizontal and vertical anchors points for the popover */ anchorPosition: PropTypes.shape({ @@ -37,6 +36,8 @@ const propTypes = { height: PropTypes.number, width: PropTypes.number, }), + + ...windowDimensionsPropTypes, }; const defaultProps = { @@ -62,9 +63,9 @@ const defaultProps = { function PopoverWithMeasuredContent(props) { const {windowWidth, windowHeight} = useWindowDimensions(); - const [popoverWidth, setPopoverWidth] = useState(props.popoverDimensions.width); - const [popoverHeight, setPopoverHeight] = useState(props.popoverDimensions.height); - const [isContentMeasured, setIsContentMeasured] = useState(popoverWidth > 0 && popoverHeight > 0); + const popoverWidth = useRef(props.popoverDimensions.width); + const popoverHeight = useRef(props.popoverDimensions.height); + const [isContentMeasured, setIsContentMeasured] = useState(popoverWidth.current > 0 && popoverHeight.current > 0); const [isVisible, setIsVisible] = useState(false); /** @@ -74,13 +75,17 @@ function PopoverWithMeasuredContent(props) { useEffect(() => { // When Popover is shown recalculate if (!isVisible && props.isVisible) { - SetIsContentMeasured(lodashGet(props, 'popoverDimensions.width', 0) > 0 && lodashGet(props, 'popoverDimensions.height', 0) > 0); - SetIsVisible(true); + // We use additional function to guarantee that async state change would be completed + setIsVisible(() => { + setIsContentMeasured(lodashGet(props, 'popoverDimensions.width', 0) > 0 && lodashGet(props, 'popoverDimensions.height', 0) > 0); + return true; + }); } if (!props.isVisible) { - SetIsVisible(false); + setIsVisible(false); } - }, [props, isVisible]); + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [props.popoverDimensions.width, props.popoverDimensions.height, props.isVisible, isVisible]); /** * Measure the size of the popover's content. @@ -88,25 +93,20 @@ function PopoverWithMeasuredContent(props) { * @param {Object} nativeEvent */ const measurePopover = ({nativeEvent}) => { - setPopoverWidth(nativeEvent.layout.width); - setPopoverHeight(nativeEvent.layout.height); - SetIsContentMeasured(true); + popoverWidth.current = nativeEvent.layout.width; + popoverHeight.current = nativeEvent.layout.height; + setIsContentMeasured(true); }; - /** - * Calculate the adjusted position of the popover. - * - * @returns {Object} - */ - const calculateAdjustedAnchorPosition = () => { + const adjustedAnchorPosition = useMemo(() => { let horizontalConstraint; switch (props.anchorAlignment.horizontal) { case CONST.MODAL.ANCHOR_ORIGIN_HORIZONTAL.RIGHT: - horizontalConstraint = {left: props.anchorPosition.horizontal - popoverWidth}; + horizontalConstraint = {left: props.anchorPosition.horizontal - popoverWidth.current}; break; case CONST.MODAL.ANCHOR_ORIGIN_HORIZONTAL.CENTER: horizontalConstraint = { - left: Math.floor(props.anchorPosition.horizontal - popoverWidth / 2), + left: Math.floor(props.anchorPosition.horizontal - popoverWidth.current / 2), }; break; case CONST.MODAL.ANCHOR_ORIGIN_HORIZONTAL.LEFT: @@ -117,11 +117,11 @@ function PopoverWithMeasuredContent(props) { let verticalConstraint; switch (props.anchorAlignment.vertical) { case CONST.MODAL.ANCHOR_ORIGIN_VERTICAL.BOTTOM: - verticalConstraint = {top: props.anchorPosition.vertical - popoverHeight}; + verticalConstraint = {top: props.anchorPosition.vertical - popoverHeight.current}; break; case CONST.MODAL.ANCHOR_ORIGIN_VERTICAL.CENTER: verticalConstraint = { - top: Math.floor(props.anchorPosition.vertical - popoverHeight / 2), + top: Math.floor(props.anchorPosition.vertical - popoverHeight.current / 2), }; break; case CONST.MODAL.ANCHOR_ORIGIN_VERTICAL.TOP: @@ -133,11 +133,10 @@ function PopoverWithMeasuredContent(props) { ...horizontalConstraint, ...verticalConstraint, }; - }; + }, [props.anchorPosition, props.anchorAlignment]); - const adjustedAnchorPosition = calculateAdjustedAnchorPosition(); - const horizontalShift = computeHorizontalShift(adjustedAnchorPosition.left, popoverWidth, windowWidth); - const verticalShift = computeVerticalShift(adjustedAnchorPosition.top, popoverHeight, windowHeight); + const horizontalShift = computeHorizontalShift(adjustedAnchorPosition.left, popoverWidth.current, windowWidth); + const verticalShift = computeVerticalShift(adjustedAnchorPosition.top, popoverHeight.current, windowHeight); const shiftedAnchorPosition = { left: adjustedAnchorPosition.left + horizontalShift, top: adjustedAnchorPosition.top + verticalShift, @@ -170,4 +169,9 @@ PopoverWithMeasuredContent.propTypes = propTypes; PopoverWithMeasuredContent.defaultProps = defaultProps; PopoverWithMeasuredContent.displayName = 'PopoverWithMeasuredContent'; -export default React.memo(PopoverWithMeasuredContent, (prevProps, nextProps) => prevProps.isVisible && !_.isEqual(prevProps, nextProps)); +export default React.memo( + withWindowDimensions(PopoverWithMeasuredContent), + (prevProps, nextProps) => + (prevProps.isVisible && (nextProps.windowWidth !== prevProps.windowWidth || nextProps.windowHeight !== prevProps.windowHeight)) || + !_.isEqual(_.omit(prevProps, ['windowWidth', 'windowHeight']), _.omit(nextProps, ['windowWidth', 'windowHeight'])), +); From ad0056b37a54c571e2aef40639d106461bce743a Mon Sep 17 00:00:00 2001 From: Alex D Date: Mon, 17 Jul 2023 11:47:32 +0300 Subject: [PATCH 05/12] Fix add vars to state, fix react.memo --- src/components/PopoverWithMeasuredContent.js | 41 +++++++++----------- 1 file changed, 18 insertions(+), 23 deletions(-) diff --git a/src/components/PopoverWithMeasuredContent.js b/src/components/PopoverWithMeasuredContent.js index 1983056243d4..3802f487f22d 100644 --- a/src/components/PopoverWithMeasuredContent.js +++ b/src/components/PopoverWithMeasuredContent.js @@ -1,12 +1,12 @@ import _ from 'underscore'; -import React, {useState, useEffect, useRef, useMemo} from 'react'; +import React, {useState, useEffect, useMemo} from 'react'; import PropTypes from 'prop-types'; import {View} from 'react-native'; import lodashGet from 'lodash/get'; import Popover from './Popover'; import {propTypes as popoverPropTypes, defaultProps as defaultPopoverProps} from './Popover/popoverPropTypes'; import useWindowDimensions from '../hooks/useWindowDimensions'; -import withWindowDimensions, {windowDimensionsPropTypes} from './withWindowDimensions'; +import withWindowDimensions from './withWindowDimensions'; import CONST from '../CONST'; import styles from '../styles/styles'; import {computeHorizontalShift, computeVerticalShift} from '../styles/getPopoverWithMeasuredContentStyles'; @@ -36,8 +36,6 @@ const propTypes = { height: PropTypes.number, width: PropTypes.number, }), - - ...windowDimensionsPropTypes, }; const defaultProps = { @@ -63,9 +61,9 @@ const defaultProps = { function PopoverWithMeasuredContent(props) { const {windowWidth, windowHeight} = useWindowDimensions(); - const popoverWidth = useRef(props.popoverDimensions.width); - const popoverHeight = useRef(props.popoverDimensions.height); - const [isContentMeasured, setIsContentMeasured] = useState(popoverWidth.current > 0 && popoverHeight.current > 0); + const [popoverWidth, setPopoverWidth] = useState(props.popoverDimensions.width); + const [popoverHeight, setPopoverHeight] = useState(props.popoverDimensions.height); + const [isContentMeasured, setIsContentMeasured] = useState(popoverWidth > 0 && popoverHeight > 0); const [isVisible, setIsVisible] = useState(false); /** @@ -93,20 +91,22 @@ function PopoverWithMeasuredContent(props) { * @param {Object} nativeEvent */ const measurePopover = ({nativeEvent}) => { - popoverWidth.current = nativeEvent.layout.width; - popoverHeight.current = nativeEvent.layout.height; - setIsContentMeasured(true); + setIsContentMeasured(() => { + setPopoverWidth(nativeEvent.layout.width); + setPopoverHeight(nativeEvent.layout.height); + return true; + }); }; const adjustedAnchorPosition = useMemo(() => { let horizontalConstraint; switch (props.anchorAlignment.horizontal) { case CONST.MODAL.ANCHOR_ORIGIN_HORIZONTAL.RIGHT: - horizontalConstraint = {left: props.anchorPosition.horizontal - popoverWidth.current}; + horizontalConstraint = {left: props.anchorPosition.horizontal - popoverWidth}; break; case CONST.MODAL.ANCHOR_ORIGIN_HORIZONTAL.CENTER: horizontalConstraint = { - left: Math.floor(props.anchorPosition.horizontal - popoverWidth.current / 2), + left: Math.floor(props.anchorPosition.horizontal - popoverWidth / 2), }; break; case CONST.MODAL.ANCHOR_ORIGIN_HORIZONTAL.LEFT: @@ -117,11 +117,11 @@ function PopoverWithMeasuredContent(props) { let verticalConstraint; switch (props.anchorAlignment.vertical) { case CONST.MODAL.ANCHOR_ORIGIN_VERTICAL.BOTTOM: - verticalConstraint = {top: props.anchorPosition.vertical - popoverHeight.current}; + verticalConstraint = {top: props.anchorPosition.vertical - popoverHeight}; break; case CONST.MODAL.ANCHOR_ORIGIN_VERTICAL.CENTER: verticalConstraint = { - top: Math.floor(props.anchorPosition.vertical - popoverHeight.current / 2), + top: Math.floor(props.anchorPosition.vertical - popoverHeight / 2), }; break; case CONST.MODAL.ANCHOR_ORIGIN_VERTICAL.TOP: @@ -133,10 +133,10 @@ function PopoverWithMeasuredContent(props) { ...horizontalConstraint, ...verticalConstraint, }; - }, [props.anchorPosition, props.anchorAlignment]); + }, [props.anchorPosition, props.anchorAlignment, popoverWidth, popoverHeight]); - const horizontalShift = computeHorizontalShift(adjustedAnchorPosition.left, popoverWidth.current, windowWidth); - const verticalShift = computeVerticalShift(adjustedAnchorPosition.top, popoverHeight.current, windowHeight); + const horizontalShift = computeHorizontalShift(adjustedAnchorPosition.left, popoverWidth, windowWidth); + const verticalShift = computeVerticalShift(adjustedAnchorPosition.top, popoverHeight, windowHeight); const shiftedAnchorPosition = { left: adjustedAnchorPosition.left + horizontalShift, top: adjustedAnchorPosition.top + verticalShift, @@ -169,9 +169,4 @@ PopoverWithMeasuredContent.propTypes = propTypes; PopoverWithMeasuredContent.defaultProps = defaultProps; PopoverWithMeasuredContent.displayName = 'PopoverWithMeasuredContent'; -export default React.memo( - withWindowDimensions(PopoverWithMeasuredContent), - (prevProps, nextProps) => - (prevProps.isVisible && (nextProps.windowWidth !== prevProps.windowWidth || nextProps.windowHeight !== prevProps.windowHeight)) || - !_.isEqual(_.omit(prevProps, ['windowWidth', 'windowHeight']), _.omit(nextProps, ['windowWidth', 'windowHeight'])), -); +export default React.memo(withWindowDimensions(PopoverWithMeasuredContent)); From 66662bf73ac169a91d5e2b802f89b2a22e60ec4e Mon Sep 17 00:00:00 2001 From: Alex D Date: Mon, 17 Jul 2023 14:02:01 +0300 Subject: [PATCH 06/12] Fix React.memo update logic --- src/components/PopoverWithMeasuredContent.js | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/components/PopoverWithMeasuredContent.js b/src/components/PopoverWithMeasuredContent.js index 3802f487f22d..59136cd67cca 100644 --- a/src/components/PopoverWithMeasuredContent.js +++ b/src/components/PopoverWithMeasuredContent.js @@ -169,4 +169,9 @@ PopoverWithMeasuredContent.propTypes = propTypes; PopoverWithMeasuredContent.defaultProps = defaultProps; PopoverWithMeasuredContent.displayName = 'PopoverWithMeasuredContent'; -export default React.memo(withWindowDimensions(PopoverWithMeasuredContent)); +export default React.memo( + withWindowDimensions(PopoverWithMeasuredContent), + (prevProps, nextProps) => + (prevProps.isVisible && (nextProps.windowWidth === prevProps.windowWidth || nextProps.windowHeight === prevProps.windowHeight)) || + _.isEqual(_.omit(prevProps, ['windowWidth', 'windowHeight']), _.omit(nextProps, ['windowWidth', 'windowHeight'])), +); From 677d6ba08fca8482840c06204721cd324ab68621 Mon Sep 17 00:00:00 2001 From: Alex D Date: Mon, 17 Jul 2023 14:14:53 +0300 Subject: [PATCH 07/12] Fix add windowDimensionsPropTypes explicitly --- src/components/PopoverWithMeasuredContent.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/components/PopoverWithMeasuredContent.js b/src/components/PopoverWithMeasuredContent.js index 59136cd67cca..0601a42b3007 100644 --- a/src/components/PopoverWithMeasuredContent.js +++ b/src/components/PopoverWithMeasuredContent.js @@ -6,7 +6,7 @@ import lodashGet from 'lodash/get'; import Popover from './Popover'; import {propTypes as popoverPropTypes, defaultProps as defaultPopoverProps} from './Popover/popoverPropTypes'; import useWindowDimensions from '../hooks/useWindowDimensions'; -import withWindowDimensions from './withWindowDimensions'; +import withWindowDimensions, {windowDimensionsPropTypes} from './withWindowDimensions'; import CONST from '../CONST'; import styles from '../styles/styles'; import {computeHorizontalShift, computeVerticalShift} from '../styles/getPopoverWithMeasuredContentStyles'; @@ -36,6 +36,8 @@ const propTypes = { height: PropTypes.number, width: PropTypes.number, }), + + ...windowDimensionsPropTypes, }; const defaultProps = { From 76c9d9d3da313072221d385210d7ec3349469e13 Mon Sep 17 00:00:00 2001 From: Alex D Date: Sun, 23 Jul 2023 15:35:28 +0300 Subject: [PATCH 08/12] Fix proper getDerivedStateFromProps replace --- src/components/PopoverWithMeasuredContent.js | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/components/PopoverWithMeasuredContent.js b/src/components/PopoverWithMeasuredContent.js index 0601a42b3007..b8c4a3209112 100644 --- a/src/components/PopoverWithMeasuredContent.js +++ b/src/components/PopoverWithMeasuredContent.js @@ -72,15 +72,15 @@ function PopoverWithMeasuredContent(props) { * When Popover becomes visible, we need to recalculate the Dimensions. * Skip render on Popover until recalculations have done by setting isContentMeasured false as early as possible. */ + if (!isVisible && props.isVisible) { + // We use additional function to guarantee that async state change would be completed + setIsVisible(() => { + setIsContentMeasured(props.popoverDimensions.width > 0 && props.popoverDimensions.height > 0); + return true; + }); + } + useEffect(() => { - // When Popover is shown recalculate - if (!isVisible && props.isVisible) { - // We use additional function to guarantee that async state change would be completed - setIsVisible(() => { - setIsContentMeasured(lodashGet(props, 'popoverDimensions.width', 0) > 0 && lodashGet(props, 'popoverDimensions.height', 0) > 0); - return true; - }); - } if (!props.isVisible) { setIsVisible(false); } From 1060bd31515863ee7629bdec387867fedd2f3abe Mon Sep 17 00:00:00 2001 From: Alex D Date: Mon, 24 Jul 2023 03:23:47 +0300 Subject: [PATCH 09/12] Fix condition when not visible and react.memo props comparing --- src/components/PopoverWithMeasuredContent.js | 26 ++++++++------------ 1 file changed, 10 insertions(+), 16 deletions(-) diff --git a/src/components/PopoverWithMeasuredContent.js b/src/components/PopoverWithMeasuredContent.js index b8c4a3209112..1ba6eac791c0 100644 --- a/src/components/PopoverWithMeasuredContent.js +++ b/src/components/PopoverWithMeasuredContent.js @@ -1,8 +1,7 @@ import _ from 'underscore'; -import React, {useState, useEffect, useMemo} from 'react'; +import React, {useState, useMemo} from 'react'; import PropTypes from 'prop-types'; import {View} from 'react-native'; -import lodashGet from 'lodash/get'; import Popover from './Popover'; import {propTypes as popoverPropTypes, defaultProps as defaultPopoverProps} from './Popover/popoverPropTypes'; import useWindowDimensions from '../hooks/useWindowDimensions'; @@ -74,19 +73,14 @@ function PopoverWithMeasuredContent(props) { */ if (!isVisible && props.isVisible) { // We use additional function to guarantee that async state change would be completed - setIsVisible(() => { + setIsVisible(() => { setIsContentMeasured(props.popoverDimensions.width > 0 && props.popoverDimensions.height > 0); return true; }); + } else if (isVisible && !props.isVisible) { + setIsVisible(false); } - useEffect(() => { - if (!props.isVisible) { - setIsVisible(false); - } - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [props.popoverDimensions.width, props.popoverDimensions.height, props.isVisible, isVisible]); - /** * Measure the size of the popover's content. * @@ -171,9 +165,9 @@ PopoverWithMeasuredContent.propTypes = propTypes; PopoverWithMeasuredContent.defaultProps = defaultProps; PopoverWithMeasuredContent.displayName = 'PopoverWithMeasuredContent'; -export default React.memo( - withWindowDimensions(PopoverWithMeasuredContent), - (prevProps, nextProps) => - (prevProps.isVisible && (nextProps.windowWidth === prevProps.windowWidth || nextProps.windowHeight === prevProps.windowHeight)) || - _.isEqual(_.omit(prevProps, ['windowWidth', 'windowHeight']), _.omit(nextProps, ['windowWidth', 'windowHeight'])), -); +export default React.memo(withWindowDimensions(PopoverWithMeasuredContent), (prevProps, nextProps) => { + if (prevProps.isVisible === nextProps.isVisible && nextProps.isVisible === false) { + return true; + } + return _.isEqual(prevProps, nextProps); +}); From 8cc09167928ff70ae6e9fc8b0bda7500cd2fbcb7 Mon Sep 17 00:00:00 2001 From: Alex D Date: Mon, 24 Jul 2023 03:41:12 +0300 Subject: [PATCH 10/12] Fix remove windowDimensionsPropTypes --- src/components/PopoverWithMeasuredContent.js | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/components/PopoverWithMeasuredContent.js b/src/components/PopoverWithMeasuredContent.js index 1ba6eac791c0..0ffc7887c15b 100644 --- a/src/components/PopoverWithMeasuredContent.js +++ b/src/components/PopoverWithMeasuredContent.js @@ -5,7 +5,7 @@ import {View} from 'react-native'; import Popover from './Popover'; import {propTypes as popoverPropTypes, defaultProps as defaultPopoverProps} from './Popover/popoverPropTypes'; import useWindowDimensions from '../hooks/useWindowDimensions'; -import withWindowDimensions, {windowDimensionsPropTypes} from './withWindowDimensions'; +import {windowDimensionsPropTypes} from './withWindowDimensions'; import CONST from '../CONST'; import styles from '../styles/styles'; import {computeHorizontalShift, computeVerticalShift} from '../styles/getPopoverWithMeasuredContentStyles'; @@ -13,7 +13,7 @@ import {computeHorizontalShift, computeVerticalShift} from '../styles/getPopover const propTypes = { // All popover props except: // 1) anchorPosition (which is overridden for this component) - ..._.omit(popoverPropTypes, ['anchorPosition']), + ..._.omit(popoverPropTypes, ['anchorPosition', ..._.keys(windowDimensionsPropTypes)]), /** The horizontal and vertical anchors points for the popover */ anchorPosition: PropTypes.shape({ @@ -35,8 +35,6 @@ const propTypes = { height: PropTypes.number, width: PropTypes.number, }), - - ...windowDimensionsPropTypes, }; const defaultProps = { @@ -165,7 +163,7 @@ PopoverWithMeasuredContent.propTypes = propTypes; PopoverWithMeasuredContent.defaultProps = defaultProps; PopoverWithMeasuredContent.displayName = 'PopoverWithMeasuredContent'; -export default React.memo(withWindowDimensions(PopoverWithMeasuredContent), (prevProps, nextProps) => { +export default React.memo(PopoverWithMeasuredContent, (prevProps, nextProps) => { if (prevProps.isVisible === nextProps.isVisible && nextProps.isVisible === false) { return true; } From 21ac9f62138b996c97e9bf817154d0e863a1c493 Mon Sep 17 00:00:00 2001 From: Alex D Date: Mon, 24 Jul 2023 07:25:17 +0300 Subject: [PATCH 11/12] Fix order of setstate calls --- src/components/PopoverWithMeasuredContent.js | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/src/components/PopoverWithMeasuredContent.js b/src/components/PopoverWithMeasuredContent.js index 0ffc7887c15b..b26a24824f34 100644 --- a/src/components/PopoverWithMeasuredContent.js +++ b/src/components/PopoverWithMeasuredContent.js @@ -70,11 +70,8 @@ function PopoverWithMeasuredContent(props) { * Skip render on Popover until recalculations have done by setting isContentMeasured false as early as possible. */ if (!isVisible && props.isVisible) { - // We use additional function to guarantee that async state change would be completed - setIsVisible(() => { - setIsContentMeasured(props.popoverDimensions.width > 0 && props.popoverDimensions.height > 0); - return true; - }); + setIsContentMeasured(props.popoverDimensions.width > 0 && props.popoverDimensions.height > 0); + setIsVisible(true); } else if (isVisible && !props.isVisible) { setIsVisible(false); } @@ -85,11 +82,9 @@ function PopoverWithMeasuredContent(props) { * @param {Object} nativeEvent */ const measurePopover = ({nativeEvent}) => { - setIsContentMeasured(() => { - setPopoverWidth(nativeEvent.layout.width); - setPopoverHeight(nativeEvent.layout.height); - return true; - }); + setPopoverWidth(nativeEvent.layout.width); + setPopoverHeight(nativeEvent.layout.height); + setIsContentMeasured(true); }; const adjustedAnchorPosition = useMemo(() => { From c8b6e7403954e7041fd509d251decd5fd455d1ec Mon Sep 17 00:00:00 2001 From: Alex D Date: Mon, 24 Jul 2023 12:23:03 +0300 Subject: [PATCH 12/12] Fix add comment for props excluding --- src/components/PopoverWithMeasuredContent.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/components/PopoverWithMeasuredContent.js b/src/components/PopoverWithMeasuredContent.js index b26a24824f34..93ce9be9c415 100644 --- a/src/components/PopoverWithMeasuredContent.js +++ b/src/components/PopoverWithMeasuredContent.js @@ -13,6 +13,7 @@ import {computeHorizontalShift, computeVerticalShift} from '../styles/getPopover const propTypes = { // All popover props except: // 1) anchorPosition (which is overridden for this component) + // 2) windowDimensionsPropTypes - we exclude them because we use useWindowDimensions hook instead ..._.omit(popoverPropTypes, ['anchorPosition', ..._.keys(windowDimensionsPropTypes)]), /** The horizontal and vertical anchors points for the popover */ @@ -70,6 +71,7 @@ function PopoverWithMeasuredContent(props) { * Skip render on Popover until recalculations have done by setting isContentMeasured false as early as possible. */ if (!isVisible && props.isVisible) { + // When Popover is shown recalculate setIsContentMeasured(props.popoverDimensions.width > 0 && props.popoverDimensions.height > 0); setIsVisible(true); } else if (isVisible && !props.isVisible) {