-
Notifications
You must be signed in to change notification settings - Fork 3.3k
Component refactor: migrate PopoverWithMeasuredContent to function component #22868
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
Changes from 1 commit
cc61881
b91913b
320dc97
bd39164
ad0056b
66662bf
677d6ba
76c9d9d
1060bd3
8cc0916
21ac9f6
c8b6e74
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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({ | ||||||||||||||||||||
|
@@ -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); | ||||||||||||||||||||
alexxxwork marked this conversation as resolved.
Show resolved
Hide resolved
alexxxwork marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||
const [isContentMeasured, SetIsContentMeasured] = useState(popoverWidth > 0 && popoverHeight > 0); | ||||||||||||||||||||
alexxxwork marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||
const [isVisible, SetIsVisible] = useState(false); | ||||||||||||||||||||
alexxxwork marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||
|
||||||||||||||||||||
/** | ||||||||||||||||||||
* 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); | ||||||||||||||||||||
alexxxwork marked this conversation as resolved.
Show resolved
Hide resolved
alexxxwork marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||
} | ||||||||||||||||||||
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 = () => { | ||||||||||||||||||||
alexxxwork marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||
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)); | ||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 App/src/components/PopoverWithMeasuredContent.js Lines 97 to 105 in e2e4057
React.memo with props comparing function.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What I meant is the comparison logic is not correct. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's use the hook and for the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
Uh oh!
There was an error while loading. Please reload this page.