From a839a643cb85229719146b09c9de6e9f15378fd6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kacper=20Miko=C5=82ajczak?= Date: Fri, 6 Oct 2023 13:36:51 +0200 Subject: [PATCH 1/8] fix refs --- src/components/Hoverable/index.js | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/src/components/Hoverable/index.js b/src/components/Hoverable/index.js index f39c44b278ae..bd495e51470c 100644 --- a/src/components/Hoverable/index.js +++ b/src/components/Hoverable/index.js @@ -17,6 +17,21 @@ function mapChildren(children, callbackParam) { return children; } +function assignRef(ref, el) { + if (!ref) { + return; + } + + if (_.has(ref, 'current')) { + // eslint-disable-next-line no-param-reassign + ref.current = el; + } + + if (_.isFunction(ref)) { + ref(el); + } +} + /** * It is necessary to create a Hoverable component instead of relying solely on Pressable support for hover state, * because nesting Pressables causes issues where the hovered state of the child cannot be easily propagated to the @@ -166,7 +181,10 @@ function InnerHoverable({disabled, onHoverIn, onHoverOut, children, shouldHandle } return React.cloneElement(child, { - ref, + ref: (el) => { + ref.current = el; + assignRef(child.ref, el); + }, onMouseEnter, onMouseLeave, onBlur, From 95ce7ae7512bd8829d200d92b129d7136b4cd767 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kacper=20Miko=C5=82ajczak?= Date: Fri, 6 Oct 2023 13:58:11 +0200 Subject: [PATCH 2/8] fix Tooltip memoization --- src/components/Tooltip/index.js | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/src/components/Tooltip/index.js b/src/components/Tooltip/index.js index f60982f52dd4..9fb0dcae988c 100644 --- a/src/components/Tooltip/index.js +++ b/src/components/Tooltip/index.js @@ -47,10 +47,7 @@ function Tooltip(props) { * Display the tooltip in an animation. */ const showTooltip = useCallback(() => { - if (!isRendered) { - setIsRendered(true); - } - + setIsRendered(true); setIsVisible(true); animation.current.stopAnimation(); @@ -70,7 +67,7 @@ function Tooltip(props) { }); } TooltipSense.activate(); - }, [isRendered]); + }, []); // eslint-disable-next-line rulesdir/prefer-early-return useEffect(() => { @@ -100,7 +97,7 @@ function Tooltip(props) { /** * Hide the tooltip in an animation. */ - const hideTooltip = () => { + const hideTooltip = useCallback(() => { animation.current.stopAnimation(); if (TooltipSense.isActive() && !isTooltipSenseInitiator.current) { @@ -118,7 +115,7 @@ function Tooltip(props) { TooltipSense.deactivate(); setIsVisible(false); - }; + }, []); // Skip the tooltip and return the children if the text is empty, // we don't have a render function or the device does not support hovering From c9eb5a79083214ffee0ec20d065490d7f6f3e9af Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kacper=20Miko=C5=82ajczak?= Date: Sat, 7 Oct 2023 15:38:42 +0200 Subject: [PATCH 3/8] obey the forwardRef tips --- src/components/Hoverable/index.js | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/components/Hoverable/index.js b/src/components/Hoverable/index.js index bd495e51470c..d1bb3219ea69 100644 --- a/src/components/Hoverable/index.js +++ b/src/components/Hoverable/index.js @@ -38,7 +38,7 @@ function assignRef(ref, el) { * parent. https://github.com/necolas/react-native-web/issues/1875 */ -function InnerHoverable({disabled, onHoverIn, onHoverOut, children, shouldHandleScroll}, outerRef) { +const Hoverable = React.forwardRef(({disabled, onHoverIn, onHoverOut, children, shouldHandleScroll}, outerRef) => { const [isHovered, setIsHovered] = useState(false); const isScrolling = useRef(false); @@ -189,9 +189,7 @@ function InnerHoverable({disabled, onHoverIn, onHoverOut, children, shouldHandle onMouseLeave, onBlur, }); -} - -const Hoverable = React.forwardRef(InnerHoverable); +}); Hoverable.propTypes = propTypes; Hoverable.defaultProps = defaultProps; From 16cabc5541ef0db8873b754705a2ca9a133d139f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kacper=20Miko=C5=82ajczak?= Date: Mon, 9 Oct 2023 11:20:53 +0200 Subject: [PATCH 4/8] fix Tooltip and BaseTooltip as wrong file was changed --- src/components/Tooltip/BaseTooltip.js | 11 +- src/components/Tooltip/index.js | 178 ++++---------------------- 2 files changed, 29 insertions(+), 160 deletions(-) diff --git a/src/components/Tooltip/BaseTooltip.js b/src/components/Tooltip/BaseTooltip.js index f60982f52dd4..9fb0dcae988c 100644 --- a/src/components/Tooltip/BaseTooltip.js +++ b/src/components/Tooltip/BaseTooltip.js @@ -47,10 +47,7 @@ function Tooltip(props) { * Display the tooltip in an animation. */ const showTooltip = useCallback(() => { - if (!isRendered) { - setIsRendered(true); - } - + setIsRendered(true); setIsVisible(true); animation.current.stopAnimation(); @@ -70,7 +67,7 @@ function Tooltip(props) { }); } TooltipSense.activate(); - }, [isRendered]); + }, []); // eslint-disable-next-line rulesdir/prefer-early-return useEffect(() => { @@ -100,7 +97,7 @@ function Tooltip(props) { /** * Hide the tooltip in an animation. */ - const hideTooltip = () => { + const hideTooltip = useCallback(() => { animation.current.stopAnimation(); if (TooltipSense.isActive() && !isTooltipSenseInitiator.current) { @@ -118,7 +115,7 @@ function Tooltip(props) { TooltipSense.deactivate(); setIsVisible(false); - }; + }, []); // Skip the tooltip and return the children if the text is empty, // we don't have a render function or the device does not support hovering diff --git a/src/components/Tooltip/index.js b/src/components/Tooltip/index.js index 9fb0dcae988c..191aae44eeaa 100644 --- a/src/components/Tooltip/index.js +++ b/src/components/Tooltip/index.js @@ -1,165 +1,37 @@ -import _ from 'underscore'; -import React, {memo, useCallback, useEffect, useRef, useState} from 'react'; -import {Animated} from 'react-native'; -import {BoundsObserver} from '@react-ng/bounds-observer'; -import TooltipRenderedOnPageBody from './TooltipRenderedOnPageBody'; -import Hoverable from '../Hoverable'; -import * as tooltipPropTypes from './tooltipPropTypes'; -import TooltipSense from './TooltipSense'; -import * as DeviceCapabilities from '../../libs/DeviceCapabilities'; -import usePrevious from '../../hooks/usePrevious'; -import useLocalize from '../../hooks/useLocalize'; -import useWindowDimensions from '../../hooks/useWindowDimensions'; +import React, {memo} from 'react'; +import PropTypes from 'prop-types'; +import {propTypes as tooltipPropTypes, defaultProps as tooltipDefaultProps} from './tooltipPropTypes'; +import BaseTooltip from './BaseTooltip'; -const hasHoverSupport = DeviceCapabilities.hasHoverSupport(); +const propTypes = { + ...tooltipPropTypes, -/** - * A component used to wrap an element intended for displaying a tooltip. The term "tooltip's target" refers to the - * wrapped element, which, upon hover, triggers the tooltip to be shown. - * @param {propTypes} props - * @returns {ReactNodeLike} - */ -function Tooltip(props) { - const {children, numberOfLines, maxWidth, text, renderTooltipContent, renderTooltipContentKey} = props; + /** Whether the actual Tooltip should be rendered. If false, it's just going to return the children */ + shouldRender: PropTypes.bool, +}; - const {preferredLocale} = useLocalize(); - const {windowWidth} = useWindowDimensions(); +const defaultProps = { + ...tooltipDefaultProps, + shouldRender: true, +}; - // Is tooltip already rendered on the page's body? happens once. - const [isRendered, setIsRendered] = useState(false); - // Is the tooltip currently visible? - const [isVisible, setIsVisible] = useState(false); - // The distance between the left side of the wrapper view and the left side of the window - const [xOffset, setXOffset] = useState(0); - // The distance between the top of the wrapper view and the top of the window - const [yOffset, setYOffset] = useState(0); - // The width and height of the wrapper view - const [wrapperWidth, setWrapperWidth] = useState(0); - const [wrapperHeight, setWrapperHeight] = useState(0); - - // Whether the tooltip is first tooltip to activate the TooltipSense - const isTooltipSenseInitiator = useRef(false); - const animation = useRef(new Animated.Value(0)); - const isAnimationCanceled = useRef(false); - const prevText = usePrevious(text); - - /** - * Display the tooltip in an animation. - */ - const showTooltip = useCallback(() => { - setIsRendered(true); - setIsVisible(true); - - animation.current.stopAnimation(); - - // When TooltipSense is active, immediately show the tooltip - if (TooltipSense.isActive()) { - animation.current.setValue(1); - } else { - isTooltipSenseInitiator.current = true; - Animated.timing(animation.current, { - toValue: 1, - duration: 140, - delay: 500, - useNativeDriver: false, - }).start(({finished}) => { - isAnimationCanceled.current = !finished; - }); - } - TooltipSense.activate(); - }, []); - - // eslint-disable-next-line rulesdir/prefer-early-return - useEffect(() => { - // if the tooltip text changed before the initial animation was finished, then the tooltip won't be shown - // we need to show the tooltip again - if (isVisible && isAnimationCanceled.current && text && prevText !== text) { - isAnimationCanceled.current = false; - showTooltip(); - } - }, [isVisible, text, prevText, showTooltip]); - - /** - * Update the tooltip bounding rectangle - * - * @param {Object} bounds - updated bounds - */ - const updateBounds = (bounds) => { - if (bounds.width === 0) { - setIsRendered(false); - } - setWrapperWidth(bounds.width); - setWrapperHeight(bounds.height); - setXOffset(bounds.x); - setYOffset(bounds.y); - }; - - /** - * Hide the tooltip in an animation. - */ - const hideTooltip = useCallback(() => { - animation.current.stopAnimation(); - - if (TooltipSense.isActive() && !isTooltipSenseInitiator.current) { - animation.current.setValue(0); - } else { - // Hide the first tooltip which initiated the TooltipSense with animation - isTooltipSenseInitiator.current = false; - Animated.timing(animation.current, { - toValue: 0, - duration: 140, - useNativeDriver: false, - }).start(); - } - - TooltipSense.deactivate(); - - setIsVisible(false); - }, []); - - // Skip the tooltip and return the children if the text is empty, - // we don't have a render function or the device does not support hovering - if ((_.isEmpty(text) && renderTooltipContent == null) || !hasHoverSupport) { +function Tooltip({shouldRender, children, ...props}) { + if (!shouldRender) { return children; } return ( - <> - {isRendered && ( - - )} - - - {children} - - - + + {children} + ); } -Tooltip.propTypes = tooltipPropTypes.propTypes; -Tooltip.defaultProps = tooltipPropTypes.defaultProps; +Tooltip.displayName = 'Tooltip'; +Tooltip.propTypes = propTypes; +Tooltip.defaultProps = defaultProps; + export default memo(Tooltip); From b5f9a86bd9af752f828f94424ed6e11381dee132 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kacper=20Miko=C5=82ajczak?= Date: Mon, 9 Oct 2023 11:22:14 +0200 Subject: [PATCH 5/8] remove unnecessary memo in Tooltip --- src/components/Tooltip/index.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/components/Tooltip/index.js b/src/components/Tooltip/index.js index 191aae44eeaa..2e6789ec73f6 100644 --- a/src/components/Tooltip/index.js +++ b/src/components/Tooltip/index.js @@ -1,4 +1,4 @@ -import React, {memo} from 'react'; +import React from 'react'; import PropTypes from 'prop-types'; import {propTypes as tooltipPropTypes, defaultProps as tooltipDefaultProps} from './tooltipPropTypes'; import BaseTooltip from './BaseTooltip'; @@ -34,4 +34,4 @@ Tooltip.displayName = 'Tooltip'; Tooltip.propTypes = propTypes; Tooltip.defaultProps = defaultProps; -export default memo(Tooltip); +export default Tooltip; From 5461a716aba43ae2f68fbde7f7a293eaf461f78e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kacper=20Miko=C5=82ajczak?= Date: Mon, 9 Oct 2023 11:34:41 +0200 Subject: [PATCH 6/8] add clarification comments --- src/components/Hoverable/index.js | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/components/Hoverable/index.js b/src/components/Hoverable/index.js index d1bb3219ea69..a0fcfd4a5a22 100644 --- a/src/components/Hoverable/index.js +++ b/src/components/Hoverable/index.js @@ -5,6 +5,14 @@ import {propTypes, defaultProps} from './hoverablePropTypes'; import * as DeviceCapabilities from '../../libs/DeviceCapabilities'; import CONST from '../../CONST'; +/** + * Maps the children of a Hoverable component to + * - a function that is called with the parameter + * - the child itself if it is the only child + * @param {Array|Function|ReactNode} children - The children to map. + * @param {Object} callbackParam - The parameter to pass to the children function. + * @returns {ReactNode} The mapped children. + */ function mapChildren(children, callbackParam) { if (_.isArray(children) && children.length === 1) { return children[0]; @@ -17,6 +25,11 @@ function mapChildren(children, callbackParam) { return children; } +/** + * Assigns a ref to an element, either by setting the current property of the ref object or by calling the ref function + * @param {Object|Function} ref - The ref object or function. + * @param {HTMLElement} el - The element to assign the ref to. + */ function assignRef(ref, el) { if (!ref) { return; From 024628230fec43283a7663f9ec7ff3e89e94444d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kacper=20Miko=C5=82ajczak?= Date: Mon, 9 Oct 2023 18:16:38 +0200 Subject: [PATCH 7/8] Add meaningful names to event callbacks --- src/components/Hoverable/index.js | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/components/Hoverable/index.js b/src/components/Hoverable/index.js index a0fcfd4a5a22..267c35d716c3 100644 --- a/src/components/Hoverable/index.js +++ b/src/components/Hoverable/index.js @@ -152,7 +152,7 @@ const Hoverable = React.forwardRef(({disabled, onHoverIn, onHoverOut, children, const child = useMemo(() => React.Children.only(mapChildren(children, isHovered)), [children, isHovered]); - const onMouseEnter = useCallback( + const enableHoveredOnMouseEnter = useCallback( (el) => { updateIsHoveredOnScrolling(true); @@ -163,7 +163,7 @@ const Hoverable = React.forwardRef(({disabled, onHoverIn, onHoverOut, children, [child.props, updateIsHoveredOnScrolling], ); - const onMouseLeave = useCallback( + const disableHoveredOnMouseLeave = useCallback( (el) => { updateIsHoveredOnScrolling(false); @@ -174,7 +174,7 @@ const Hoverable = React.forwardRef(({disabled, onHoverIn, onHoverOut, children, [child.props, updateIsHoveredOnScrolling], ); - const onBlur = useCallback( + const disableHoveredOnBlur = useCallback( (el) => { // Check if the blur event occurred due to clicking outside the element // and the wrapperView contains the element that caused the blur and reset isHovered @@ -198,9 +198,9 @@ const Hoverable = React.forwardRef(({disabled, onHoverIn, onHoverOut, children, ref.current = el; assignRef(child.ref, el); }, - onMouseEnter, - onMouseLeave, - onBlur, + onMouseEnter: enableHoveredOnMouseEnter, + onMouseLeave: disableHoveredOnMouseLeave, + onBlur: disableHoveredOnBlur, }); }); From 599b119aa82a2d1b544d1b081c9b77d19f48ecae Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kacper=20Miko=C5=82ajczak?= Date: Tue, 10 Oct 2023 11:57:07 +0200 Subject: [PATCH 8/8] incorporate unsetHoveredIfOutside into useEffect --- src/components/Hoverable/index.js | 31 ++++++++++++++----------------- 1 file changed, 14 insertions(+), 17 deletions(-) diff --git a/src/components/Hoverable/index.js b/src/components/Hoverable/index.js index 267c35d716c3..5ef9fc159241 100644 --- a/src/components/Hoverable/index.js +++ b/src/components/Hoverable/index.js @@ -97,14 +97,18 @@ const Hoverable = React.forwardRef(({disabled, onHoverIn, onHoverOut, children, return () => scrollingListener.remove(); }, [shouldHandleScroll]); - /** - * Checks the hover state of a component and updates it based on the event target. - * This is necessary to handle cases where the hover state might get stuck due to an unreliable mouseleave trigger, - * such as when an element is removed before the mouseleave event is triggered. - * @param {Event} e - The hover event object. - */ - const unsetHoveredIfOutside = useCallback( - (e) => { + useEffect(() => { + if (!DeviceCapabilities.hasHoverSupport()) { + return; + } + + /** + * Checks the hover state of a component and updates it based on the event target. + * This is necessary to handle cases where the hover state might get stuck due to an unreliable mouseleave trigger, + * such as when an element is removed before the mouseleave event is triggered. + * @param {Event} e - The hover event object. + */ + const unsetHoveredIfOutside = (e) => { if (!ref.current || !isHovered) { return; } @@ -114,19 +118,12 @@ const Hoverable = React.forwardRef(({disabled, onHoverIn, onHoverOut, children, } setIsHovered(false); - }, - [isHovered], - ); - - useEffect(() => { - if (!DeviceCapabilities.hasHoverSupport()) { - return; - } + }; document.addEventListener('mouseover', unsetHoveredIfOutside); return () => document.removeEventListener('mouseover', unsetHoveredIfOutside); - }, [unsetHoveredIfOutside]); + }, [isHovered]); useEffect(() => { if (!disabled || !isHovered) {