From d1391af1f305ec0f4961b37fc65c0f4c7fb99f90 Mon Sep 17 00:00:00 2001 From: Ana Margarida Silva Date: Thu, 17 Aug 2023 14:00:56 +0100 Subject: [PATCH 1/8] feat: migrate PressableWithSecondaryInteraction to function component --- .../index.js | 162 ++++++++++-------- 1 file changed, 92 insertions(+), 70 deletions(-) diff --git a/src/components/PressableWithSecondaryInteraction/index.js b/src/components/PressableWithSecondaryInteraction/index.js index 2771db6206ae..fd50c6ff0994 100644 --- a/src/components/PressableWithSecondaryInteraction/index.js +++ b/src/components/PressableWithSecondaryInteraction/index.js @@ -1,106 +1,128 @@ import _ from 'underscore'; -import React, {Component} from 'react'; -import * as pressableWithSecondaryInteractionPropTypes from './pressableWithSecondaryInteractionPropTypes'; +import React, {forwardRef, useCallback, useEffect, useRef} from 'react'; import styles from '../../styles/styles'; import * as DeviceCapabilities from '../../libs/DeviceCapabilities'; import * as StyleUtils from '../../styles/StyleUtils'; import PressableWithFeedback from '../Pressable/PressableWithFeedback'; +import * as pressableWithSecondaryInteractionPropTypes from './pressableWithSecondaryInteractionPropTypes'; /** * This is a special Pressable that calls onSecondaryInteraction when LongPressed, or right-clicked. */ -class PressableWithSecondaryInteraction extends Component { - constructor(props) { - super(props); - this.executeSecondaryInteraction = this.executeSecondaryInteraction.bind(this); - this.executeSecondaryInteractionOnContextMenu = this.executeSecondaryInteractionOnContextMenu.bind(this); - } - - componentDidMount() { - if (this.props.forwardedRef) { - if (_.isFunction(this.props.forwardedRef)) { - this.props.forwardedRef(this.pressableRef); - } else if (_.isObject(this.props.forwardedRef)) { - this.props.forwardedRef.current = this.pressableRef; - } - } - this.pressableRef.addEventListener('contextmenu', this.executeSecondaryInteractionOnContextMenu); - } - componentWillUnmount() { - this.pressableRef.removeEventListener('contextmenu', this.executeSecondaryInteractionOnContextMenu); - } +function PressableWithSecondaryInteraction(props) { + const { + children, + inline, + style, + enableLongPressWithHover, + withoutFocusOnSecondaryInteraction, + preventDefaultContextMenu, + onSecondaryInteraction, + onPressIn, + onPress, + onPressOut, + activeOpacity, + innerRef, + ...rest + } = props; + + const pressableRef = useRef(null); /** * @param {Event} e - the secondary interaction event */ - executeSecondaryInteraction(e) { - if (DeviceCapabilities.hasHoverSupport() && !this.props.enableLongPressWithHover) { + const executeSecondaryInteraction = (e) => { + if (DeviceCapabilities.hasHoverSupport() && !enableLongPressWithHover) { return; } - if (this.props.withoutFocusOnSecondaryInteraction && this.pressableRef) { - this.pressableRef.blur(); + if (withoutFocusOnSecondaryInteraction && pressableRef && pressableRef.current) { + pressableRef.current.blur(); } - this.props.onSecondaryInteraction(e); - } + onSecondaryInteraction(e); + }; /** * @param {contextmenu} e - A right-click MouseEvent. * https://developer.mozilla.org/en-US/docs/Web/API/Element/contextmenu_event */ - executeSecondaryInteractionOnContextMenu(e) { - if (!this.props.onSecondaryInteraction) { + const executeSecondaryInteractionOnContextMenu = useCallback( + (e) => { + if (!onSecondaryInteraction) { + return; + } + + e.stopPropagation(); + if (preventDefaultContextMenu) { + e.preventDefault(); + } + + onSecondaryInteraction(e); + + /** + * This component prevents the tapped element from capturing focus. + * We need to blur this element when clicked as it opens modal that implements focus-trapping. + * When the modal is closed it focuses back to the last active element. + * Therefore it shifts the element to bring it back to focus. + * https://github.com/Expensify/App/issues/14148 + */ + if (withoutFocusOnSecondaryInteraction && pressableRef && pressableRef.current) { + pressableRef.current.blur(); + } + }, + [onSecondaryInteraction, preventDefaultContextMenu, pressableRef, withoutFocusOnSecondaryInteraction], + ); + + useEffect(() => { + if (!pressableRef || !pressableRef.current) { return; } - e.stopPropagation(); - if (this.props.preventDefaultContextMenu) { - e.preventDefault(); + if (innerRef) { + if (_.isFunction(innerRef)) { + innerRef(pressableRef); + } else if (_.isObject(innerRef)) { + innerRef.current = pressableRef.current; + } } - this.props.onSecondaryInteraction(e); - /** - * This component prevents the tapped element from capturing focus. - * We need to blur this element when clicked as it opens modal that implements focus-trapping. - * When the modal is closed it focuses back to the last active element. - * Therefore it shifts the element to bring it back to focus. - * https://github.com/Expensify/App/issues/14148 - */ - if (this.props.withoutFocusOnSecondaryInteraction && this.pressableRef) { - this.pressableRef.blur(); - } - } - - render() { - const defaultPressableProps = _.omit(this.props, ['onSecondaryInteraction', 'children', 'onLongPress']); - const inlineStyle = this.props.inline ? styles.dInline : {}; - - // On Web, Text does not support LongPress events thus manage inline mode with styling instead of using Text. - return ( - (this.pressableRef = el)} - // eslint-disable-next-line react/jsx-props-no-spreading - {...defaultPressableProps} - style={(state) => [StyleUtils.parseStyleFromFunction(this.props.style, state), inlineStyle]} - > - {this.props.children} - - ); - } + const element = pressableRef.current; + element.addEventListener('contextmenu', executeSecondaryInteractionOnContextMenu); + + return () => { + element.removeEventListener('contextmenu', executeSecondaryInteractionOnContextMenu); + }; + }, [executeSecondaryInteractionOnContextMenu, innerRef, pressableRef]); + + const defaultPressableProps = _.omit(rest, ['onLongPress']); + const inlineStyle = inline ? styles.dInline : {}; + + return ( + [StyleUtils.parseStyleFromFunction(style, state), inlineStyle]} + > + {children} + + ); } PressableWithSecondaryInteraction.propTypes = pressableWithSecondaryInteractionPropTypes.propTypes; PressableWithSecondaryInteraction.defaultProps = pressableWithSecondaryInteractionPropTypes.defaultProps; -export default React.forwardRef((props, ref) => ( +PressableWithSecondaryInteraction.displayName = 'PressableWithSecondaryInteraction'; + +export default forwardRef((props, ref) => ( )); From aa4e245d6deee5c0ad2af2af21f2d51ef46b0617 Mon Sep 17 00:00:00 2001 From: Ana Margarida Silva Date: Thu, 17 Aug 2023 15:14:48 +0100 Subject: [PATCH 2/8] refactor: code improvements --- .../index.js | 81 +++++++++---------- 1 file changed, 39 insertions(+), 42 deletions(-) diff --git a/src/components/PressableWithSecondaryInteraction/index.js b/src/components/PressableWithSecondaryInteraction/index.js index fd50c6ff0994..ce1204967a8b 100644 --- a/src/components/PressableWithSecondaryInteraction/index.js +++ b/src/components/PressableWithSecondaryInteraction/index.js @@ -1,5 +1,5 @@ import _ from 'underscore'; -import React, {forwardRef, useCallback, useEffect, useRef} from 'react'; +import React, {forwardRef, useEffect, useRef} from 'react'; import styles from '../../styles/styles'; import * as DeviceCapabilities from '../../libs/DeviceCapabilities'; import * as StyleUtils from '../../styles/StyleUtils'; @@ -10,23 +10,21 @@ import * as pressableWithSecondaryInteractionPropTypes from './pressableWithSeco * This is a special Pressable that calls onSecondaryInteraction when LongPressed, or right-clicked. */ -function PressableWithSecondaryInteraction(props) { - const { - children, - inline, - style, - enableLongPressWithHover, - withoutFocusOnSecondaryInteraction, - preventDefaultContextMenu, - onSecondaryInteraction, - onPressIn, - onPress, - onPressOut, - activeOpacity, - innerRef, - ...rest - } = props; - +function PressableWithSecondaryInteraction({ + children, + inline, + style, + enableLongPressWithHover, + withoutFocusOnSecondaryInteraction, + preventDefaultContextMenu, + onSecondaryInteraction, + onPressIn, + onPress, + onPressOut, + activeOpacity, + forwardedRef, + ...rest +}) { const pressableRef = useRef(null); /** @@ -42,12 +40,25 @@ function PressableWithSecondaryInteraction(props) { onSecondaryInteraction(e); }; - /** - * @param {contextmenu} e - A right-click MouseEvent. - * https://developer.mozilla.org/en-US/docs/Web/API/Element/contextmenu_event - */ - const executeSecondaryInteractionOnContextMenu = useCallback( - (e) => { + useEffect(() => { + if (!pressableRef || !pressableRef.current) { + return; + } + + if (forwardedRef) { + if (_.isFunction(forwardedRef)) { + forwardedRef(pressableRef); + } else if (_.isObject(forwardedRef)) { + // eslint-disable-next-line no-param-reassign + forwardedRef.current = pressableRef.current; + } + } + + /** + * @param {contextmenu} e - A right-click MouseEvent. + * https://developer.mozilla.org/en-US/docs/Web/API/Element/contextmenu_event + */ + const executeSecondaryInteractionOnContextMenu = (e) => { if (!onSecondaryInteraction) { return; } @@ -69,22 +80,7 @@ function PressableWithSecondaryInteraction(props) { if (withoutFocusOnSecondaryInteraction && pressableRef && pressableRef.current) { pressableRef.current.blur(); } - }, - [onSecondaryInteraction, preventDefaultContextMenu, pressableRef, withoutFocusOnSecondaryInteraction], - ); - - useEffect(() => { - if (!pressableRef || !pressableRef.current) { - return; - } - - if (innerRef) { - if (_.isFunction(innerRef)) { - innerRef(pressableRef); - } else if (_.isObject(innerRef)) { - innerRef.current = pressableRef.current; - } - } + }; const element = pressableRef.current; element.addEventListener('contextmenu', executeSecondaryInteractionOnContextMenu); @@ -92,7 +88,8 @@ function PressableWithSecondaryInteraction(props) { return () => { element.removeEventListener('contextmenu', executeSecondaryInteractionOnContextMenu); }; - }, [executeSecondaryInteractionOnContextMenu, innerRef, pressableRef]); + // eslint-disable-next-line react-hooks/exhaustive-deps + }, []); const defaultPressableProps = _.omit(rest, ['onLongPress']); const inlineStyle = inline ? styles.dInline : {}; @@ -123,6 +120,6 @@ export default forwardRef((props, ref) => ( )); From dfd18964a115a0e7039e36dadcc13a8b1f8c866a Mon Sep 17 00:00:00 2001 From: Ana Margarida Silva Date: Thu, 17 Aug 2023 15:17:16 +0100 Subject: [PATCH 3/8] refactor: simplify --- src/components/PressableWithSecondaryInteraction/index.js | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/components/PressableWithSecondaryInteraction/index.js b/src/components/PressableWithSecondaryInteraction/index.js index ce1204967a8b..ec74de1837b2 100644 --- a/src/components/PressableWithSecondaryInteraction/index.js +++ b/src/components/PressableWithSecondaryInteraction/index.js @@ -54,6 +54,8 @@ function PressableWithSecondaryInteraction({ } } + const element = pressableRef.current; + /** * @param {contextmenu} e - A right-click MouseEvent. * https://developer.mozilla.org/en-US/docs/Web/API/Element/contextmenu_event @@ -77,12 +79,11 @@ function PressableWithSecondaryInteraction({ * Therefore it shifts the element to bring it back to focus. * https://github.com/Expensify/App/issues/14148 */ - if (withoutFocusOnSecondaryInteraction && pressableRef && pressableRef.current) { - pressableRef.current.blur(); + if (withoutFocusOnSecondaryInteraction) { + element.blur(); } }; - const element = pressableRef.current; element.addEventListener('contextmenu', executeSecondaryInteractionOnContextMenu); return () => { From 46d11cfea68cf4a609ab5748b5f052567294244a Mon Sep 17 00:00:00 2001 From: Ana Margarida Silva Date: Thu, 17 Aug 2023 15:18:45 +0100 Subject: [PATCH 4/8] fix: add missing comment --- src/components/PressableWithSecondaryInteraction/index.js | 1 + 1 file changed, 1 insertion(+) diff --git a/src/components/PressableWithSecondaryInteraction/index.js b/src/components/PressableWithSecondaryInteraction/index.js index ec74de1837b2..46de532156c9 100644 --- a/src/components/PressableWithSecondaryInteraction/index.js +++ b/src/components/PressableWithSecondaryInteraction/index.js @@ -89,6 +89,7 @@ function PressableWithSecondaryInteraction({ return () => { element.removeEventListener('contextmenu', executeSecondaryInteractionOnContextMenu); }; + // We only want this to run on mount // eslint-disable-next-line react-hooks/exhaustive-deps }, []); From c1dd8616fd6228b154a6d27c4c298b9c97c65273 Mon Sep 17 00:00:00 2001 From: Ana Margarida Silva Date: Thu, 17 Aug 2023 15:36:31 +0100 Subject: [PATCH 5/8] refactor: simplify --- src/components/PressableWithSecondaryInteraction/index.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/components/PressableWithSecondaryInteraction/index.js b/src/components/PressableWithSecondaryInteraction/index.js index 46de532156c9..2cd7004a68df 100644 --- a/src/components/PressableWithSecondaryInteraction/index.js +++ b/src/components/PressableWithSecondaryInteraction/index.js @@ -34,14 +34,14 @@ function PressableWithSecondaryInteraction({ if (DeviceCapabilities.hasHoverSupport() && !enableLongPressWithHover) { return; } - if (withoutFocusOnSecondaryInteraction && pressableRef && pressableRef.current) { + if (withoutFocusOnSecondaryInteraction && pressableRef.current) { pressableRef.current.blur(); } onSecondaryInteraction(e); }; useEffect(() => { - if (!pressableRef || !pressableRef.current) { + if (!pressableRef.current) { return; } From 56aff086e00fe074ab4308ebca9430d87d0c89e4 Mon Sep 17 00:00:00 2001 From: Ana Margarida Silva Date: Thu, 17 Aug 2023 15:48:33 +0100 Subject: [PATCH 6/8] revert: useEffect dependencies --- src/components/PressableWithSecondaryInteraction/index.js | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/components/PressableWithSecondaryInteraction/index.js b/src/components/PressableWithSecondaryInteraction/index.js index 2cd7004a68df..50fb48fc1525 100644 --- a/src/components/PressableWithSecondaryInteraction/index.js +++ b/src/components/PressableWithSecondaryInteraction/index.js @@ -89,9 +89,7 @@ function PressableWithSecondaryInteraction({ return () => { element.removeEventListener('contextmenu', executeSecondaryInteractionOnContextMenu); }; - // We only want this to run on mount - // eslint-disable-next-line react-hooks/exhaustive-deps - }, []); + }, [forwardedRef, onSecondaryInteraction, preventDefaultContextMenu, withoutFocusOnSecondaryInteraction]); const defaultPressableProps = _.omit(rest, ['onLongPress']); const inlineStyle = inline ? styles.dInline : {}; From 3a0f64767d6ca9ce916e9be4e558ce496e684590 Mon Sep 17 00:00:00 2001 From: Ana Margarida Silva Date: Thu, 17 Aug 2023 16:04:08 +0100 Subject: [PATCH 7/8] docs: add more info to onSecondaryInteraction doc --- .../pressableWithSecondaryInteractionPropTypes.js | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/components/PressableWithSecondaryInteraction/pressableWithSecondaryInteractionPropTypes.js b/src/components/PressableWithSecondaryInteraction/pressableWithSecondaryInteractionPropTypes.js index de0adbe81297..f521a57957f3 100644 --- a/src/components/PressableWithSecondaryInteraction/pressableWithSecondaryInteractionPropTypes.js +++ b/src/components/PressableWithSecondaryInteraction/pressableWithSecondaryInteractionPropTypes.js @@ -12,7 +12,12 @@ const propTypes = { /** The function that should be called when this pressable is pressedOut */ onPressOut: PropTypes.func, - /** The function that should be called when this pressable is LongPressed or right-clicked. */ + /** + * The function that should be called when this pressable is LongPressed or right-clicked. + * + * This function should be stable, preferably wrapped in a `useCallback` so that it does not + * cause several re-renders. + */ onSecondaryInteraction: PropTypes.func, /** The children which should be contained in this wrapper component. */ From ba3fc9bb590c3fcaed55cc3136e5c532ddcc2e27 Mon Sep 17 00:00:00 2001 From: Ana Margarida Silva Date: Mon, 21 Aug 2023 13:45:11 +0100 Subject: [PATCH 8/8] revert: restore comment --- src/components/PressableWithSecondaryInteraction/index.js | 1 + 1 file changed, 1 insertion(+) diff --git a/src/components/PressableWithSecondaryInteraction/index.js b/src/components/PressableWithSecondaryInteraction/index.js index 50fb48fc1525..3f9039489b7c 100644 --- a/src/components/PressableWithSecondaryInteraction/index.js +++ b/src/components/PressableWithSecondaryInteraction/index.js @@ -94,6 +94,7 @@ function PressableWithSecondaryInteraction({ const defaultPressableProps = _.omit(rest, ['onLongPress']); const inlineStyle = inline ? styles.dInline : {}; + // On Web, Text does not support LongPress events thus manage inline mode with styling instead of using Text. return (