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

Component refactor: migrate PopoverWithMeasuredContent to function component #22868

Merged
merged 12 commits into from
Jul 25, 2023
151 changes: 64 additions & 87 deletions src/components/PopoverWithMeasuredContent.js
Original file line number Diff line number Diff line change
@@ -1,19 +1,21 @@
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';

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({
Expand All @@ -35,8 +37,6 @@ const propTypes = {
height: PropTypes.number,
width: PropTypes.number,
}),

...windowDimensionsPropTypes,
};

const defaultProps = {
Expand All @@ -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 ? (
<Popover
// eslint-disable-next-line react/jsx-props-no-spreading
{...this.props}
anchorPosition={shiftedAnchorPosition}
>
{this.props.children}
</Popover>
) : (
/*
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.
*/
<View
style={styles.invisible}
onLayout={this.measurePopover}
>
{this.props.children}
</View>
);
}
};

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 ? (
<Popover
// eslint-disable-next-line react/jsx-props-no-spreading
{...props}
anchorPosition={shiftedAnchorPosition}
>
{props.children}
</Popover>
) : (
/*
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.
*/
<View
style={styles.invisible}
onLayout={measurePopover}
>
{props.children}
</View>
);
}

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));
Copy link
Contributor

Choose a reason for hiding this comment

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

This logic is not correct. Previously this was used with the window dimensions props.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@s77rt This is not a replace. We change withWindowDimensions to useWindowDimensions hook so hoc is not needed anymore. And we had a shouldComponentUpdate here

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);
}
which is replaced with React.memo with props comparing function.

Copy link
Contributor

Choose a reason for hiding this comment

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

What I meant is the comparison logic is not correct. prevProps.isVisible && !_.isEqual(prevProps, nextProps) this makes no sense and it seems to be the cause of a regression, the floating action button is broken.

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 I'd better return windowWidth/Height props to handle this update logic - if isVisible update only on dimensions change.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use the hook and for the React.memo() we can omit the function parameters. The "do not update if only window dimensions change" is useless, the component will pretty much already re-render because we are passing a lot of changing data (e.g. anchorPosition is always a new object).

Copy link
Contributor Author

@alexxxwork alexxxwork Jul 17, 2023

Choose a reason for hiding this comment

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

No, this leads to a regression with browser window dimensions change. I'll fix the logic

Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting bug. We should fix that bug instead of hiding it. Any ideas yet on the root cause?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@s77rt I think the root cause is that shiftedAnchorPosition left/top could be <0 on some updates. You could see it if you add console.log right before return and leave React.memo without props comparing function. So it seems to be not hiding the bug but prevent unnecessary updates.

Copy link
Contributor

Choose a reason for hiding this comment

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

@alexxxwork This still does not explain the root cause. Something is not right, the whole content changes.