code review fixes for tooltip

Expensify · Nov 26, 2024 · 334a0de · 334a0de
1 parent a7dfc67
commit 334a0de
Show file tree

Hide file tree

Showing 5 changed files with 22 additions and 44 deletions.
diff --git a/src/components/Tooltip/BaseGenericTooltip/index.native.tsx b/src/components/Tooltip/BaseGenericTooltip/index.native.tsx
@@ -52,7 +52,6 @@ function BaseGenericTooltip({
             StyleUtils.getTooltipStyles({
                 // eslint-disable-next-line react-compiler/react-compiler
                 tooltip: rootWrapper.current,
-                currentSize: animation.value,
                 windowWidth,
                 xOffset,
                 yOffset,
@@ -70,7 +69,6 @@ function BaseGenericTooltip({
             }),
         [
             StyleUtils,
-            animation,
             windowWidth,
             xOffset,
             yOffset,

diff --git a/src/components/Tooltip/BaseGenericTooltip/index.tsx b/src/components/Tooltip/BaseGenericTooltip/index.tsx
@@ -70,7 +70,6 @@ function BaseGenericTooltip({
         () =>
             StyleUtils.getTooltipStyles({
                 tooltip: rootWrapper.current,
-                currentSize: animation.value,
                 windowWidth,
                 xOffset,
                 yOffset,
@@ -87,7 +86,6 @@ function BaseGenericTooltip({
             }),
         [
             StyleUtils,
-            animation,
             windowWidth,
             xOffset,
             yOffset,

diff --git a/src/components/Tooltip/BaseGenericTooltip/types.ts b/src/components/Tooltip/BaseGenericTooltip/types.ts
@@ -1,6 +1,5 @@
-import type {Animated} from 'react-native';
+import type {SharedValue} from 'react-native-reanimated';
 import type {SharedTooltipProps} from '@components/Tooltip/types';
-import { SharedValue } from "react-native-reanimated";
 
 type BaseGenericTooltipProps = {
     /** Window width */

diff --git a/src/components/Tooltip/GenericTooltip.tsx b/src/components/Tooltip/GenericTooltip.tsx
@@ -1,6 +1,6 @@
 import React, {memo, useCallback, useEffect, useState} from 'react';
 import type {LayoutRectangle} from 'react-native';
-import {cancelAnimation, runOnJS, useSharedValue, withDelay, withTiming} from 'react-native-reanimated';
+import {cancelAnimation, useSharedValue, withDelay, withTiming} from 'react-native-reanimated';
 import useLocalize from '@hooks/useLocalize';
 import usePrevious from '@hooks/usePrevious';
 import useWindowDimensions from '@hooks/useWindowDimensions';
@@ -59,9 +59,9 @@ function GenericTooltip({
     const [shouldUseOverlay, setShouldUseOverlay] = useState(shouldUseOverlayProp);
 
     // Whether the tooltip is first tooltip to activate the TooltipSense
-    const animationSharedValue = useSharedValue<number>(0);
-    const isTooltipSenseInitiatorShared = useSharedValue<boolean>(true);
-    const isAnimationCanceledShared = useSharedValue<boolean>(false);
+    const animation = useSharedValue<number>(0);
+    const isTooltipSenseInitiator = useSharedValue<boolean>(true);
+    const isAnimationCanceled = useSharedValue<boolean>(false);
     const prevText = usePrevious(text);
 
     useEffect(() => {
@@ -78,40 +78,38 @@ function GenericTooltip({
         setIsRendered(true);
         setIsVisible(true);
 
-        cancelAnimation(animationSharedValue);
+        cancelAnimation(animation);
 
         // When TooltipSense is active, immediately show the tooltip
         if (TooltipSense.isActive() && !shouldForceAnimate) {
-            animationSharedValue.value = 1;
+            animation.value = 1;
         } else {
-            isTooltipSenseInitiatorShared.value = true;
-            animationSharedValue.value = withDelay(
+            isTooltipSenseInitiator.value = true;
+            animation.value = withDelay(
                 500,
                 withTiming(
                     1,
                     {
                         duration: 140,
                     },
                     (finished) => {
-                        runOnJS(() => {
-                            isAnimationCanceledShared.value = !finished;
-                        })();
+                        isAnimationCanceled.value = !finished;
                     },
                 ),
             );
         }
         TooltipSense.activate();
-    }, [shouldForceAnimate]);
+    }, [animation, isAnimationCanceled, isTooltipSenseInitiator, shouldForceAnimate]);
 
     // 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 && isAnimationCanceledShared.value && text && prevText !== text) {
-            isAnimationCanceledShared.value = false;
+        if (isVisible && isAnimationCanceled.value && text && prevText !== text) {
+            isAnimationCanceled.value = false;
             showTooltip();
         }
-    }, [isVisible, text, prevText, showTooltip]);
+    }, [isVisible, text, prevText, showTooltip, isAnimationCanceled]);
 
     /**
      * Update the tooltip's target bounding rectangle
@@ -130,19 +128,19 @@ function GenericTooltip({
      * Hide the tooltip in an animation.
      */
     const hideTooltip = useCallback(() => {
-        cancelAnimation(animationSharedValue);
+        cancelAnimation(animation);
 
-        if (TooltipSense.isActive() && !isTooltipSenseInitiatorShared.value) {
+        if (TooltipSense.isActive() && !isTooltipSenseInitiator.value) {
             // eslint-disable-next-line react-compiler/react-compiler
-            animationSharedValue.value = 0;
+            animation.value = 0;
         } else {
             // Hide the first tooltip which initiated the TooltipSense with animation
-            isTooltipSenseInitiatorShared.value = false;
-            animationSharedValue.value = 0;
+            isTooltipSenseInitiator.value = false;
+            animation.value = 0;
         }
         TooltipSense.deactivate();
         setIsVisible(false);
-    }, []);
+    }, [animation, isTooltipSenseInitiator]);
 
     const onPressOverlay = useCallback(() => {
         if (!shouldUseOverlay) {
@@ -164,7 +162,7 @@ function GenericTooltip({
             {isRendered && (
                 <BaseGenericTooltip
                     // eslint-disable-next-line react-compiler/react-compiler
-                    animation={animationSharedValue}
+                    animation={animation}
                     windowWidth={windowWidth}
                     xOffset={xOffset}
                     yOffset={yOffset}

diff --git a/src/styles/utils/generators/TooltipStyleUtils/index.ts b/src/styles/utils/generators/TooltipStyleUtils/index.ts
@@ -22,7 +22,6 @@ const POINTER_HEIGHT = 4;
 const POINTER_WIDTH = 12;
 
 type TooltipStyles = {
-    animationStyle: ViewStyle;
     rootWrapperStyle: ViewStyle;
     textStyle: TextStyle;
     pointerWrapperStyle: ViewStyle;
@@ -31,7 +30,6 @@ type TooltipStyles = {
 
 type TooltipParams = {
     tooltip: View | HTMLDivElement | null;
-    currentSize: number;
     windowWidth: number;
     xOffset: number;
     yOffset: number;
@@ -83,7 +81,6 @@ type GetTooltipStylesStyleUtil = {getTooltipStyles: (props: TooltipParams) => To
 const createTooltipStyleUtils: StyleUtilGenerator<GetTooltipStylesStyleUtil> = ({theme, styles}) => ({
     getTooltipStyles: ({
         tooltip,
-        currentSize,
         windowWidth,
         xOffset,
         yOffset,
@@ -114,8 +111,6 @@ const createTooltipStyleUtils: StyleUtilGenerator<GetTooltipStylesStyleUtil> = (
 
         const isTooltipSizeReady = tooltipWidth !== undefined && tooltipHeight !== undefined;
 
-        // Set the scale to 1 to be able to measure the tooltip size correctly when it's not ready yet.
-        let scale = 1;
         let shouldShowBelow = false;
         let horizontalShift = 0;
         let horizontalShiftPointer = 0;
@@ -137,9 +132,6 @@ const createTooltipStyleUtils: StyleUtilGenerator<GetTooltipStylesStyleUtil> = (
                 !!(tooltip && isOverlappingAtTop(tooltip, xOffset, yOffset, tooltipTargetWidth, tooltipTargetHeight)) ||
                 anchorAlignment.vertical === CONST.MODAL.ANCHOR_ORIGIN_VERTICAL.TOP;
 
-            // When the tooltip size is ready, we can start animating the scale.
-            scale = currentSize;
-
             // Determine if we need to shift the tooltip horizontally to prevent it
             // from displaying too near to the edge of the screen.
             horizontalShift = computeHorizontalShift(windowWidth, xOffset, tooltipTargetWidth, tooltipWidth, manualShiftHorizontal);
@@ -223,12 +215,6 @@ const createTooltipStyleUtils: StyleUtilGenerator<GetTooltipStylesStyleUtil> = (
         }
 
         return {
-            animationStyle: {
-                // remember Transform causes a new Local cordinate system
-                // https://drafts.csswg.org/css-transforms-1/#transform-rendering
-                // so Position fixed children will be relative to this new Local cordinate system
-                transform: [{scale}],
-            },
             rootWrapperStyle: {
                 ...tooltipPlatformStyle,
                 backgroundColor: theme.heading,
@@ -277,8 +263,7 @@ const createTooltipStyleUtils: StyleUtilGenerator<GetTooltipStylesStyleUtil> = (
         };
     },
 
-    // Utility function to create and manage scale animations with React Native Reanimated
-
+    /** Utility function to create and manage scale animations with React Native Reanimated */
     getTooltipAnimatedStyles: (props: TooltipAnimationProps) => {
         const tooltipHorizontalPadding = spacing.ph2.paddingHorizontal * 2;
         const tooltipWidth = props.tooltipContentWidth && props.tooltipContentWidth + tooltipHorizontalPadding + 1;