fix(tooltip): don't pass in a ref as it's not actually needed (#82)

zendeskgarden · Aug 2, 2019 · 75c77bc · 75c77bc
1 parent 5cbf0f0
commit 75c77bc
Show file tree

Hide file tree

Showing 4 changed files with 22 additions and 22 deletions.
diff --git a/packages/tooltip/README.md b/packages/tooltip/README.md
@@ -16,14 +16,10 @@ For live examples check out our [storybook](https://zendeskgarden.github.io/reac
 ### useTooltip
 
 ```jsx static
-import { useRef } from 'react';
 import { useTooltip } from '@zendeskgarden/container-tooltip';
 
 const Tooltip = () => {
-  const tooltipRef = useRef(null);
-
   const { isVisible, getTooltipProps, getTriggerProps } = useTooltip({
-    tooltipRef,
     isVisible: false,
     delayMilliseconds: 500
   });
@@ -38,8 +34,8 @@ const Tooltip = () => {
 
   return (
     <>
-      <div {...getTooltipProps({ ref: tooltipRef, style: styles })}>Tooltip</div>
-      <button {...getTriggerProps({ ref: triggerRef })}>Trigger</button>
+      <div {...getTooltipProps({ style: styles })}>Tooltip</div>
+      <button {...getTriggerProps()}>Trigger</button>
     </>
   );
 };
@@ -48,14 +44,11 @@ const Tooltip = () => {
 ### TooltipContainer
 
 ```jsx static
-import { useRef } from 'react';
 import { TooltipContainer } from '@zendeskgarden/container-tooltip';
 
 const Tooltip = () => {
-  const tooltipRef = useRef(null);
-
   return (
-    <TooltipContainer tooltipRef={tooltipRef} isVisible={false} delayMilliseconds={500}>
+    <TooltipContainer isVisible={false} delayMilliseconds={500}>
       {({ isVisible, getTooltipProps, getTriggerProps }) => {
         const styles = {
           visibility: isVisible ? 'visible' : 'hidden',
@@ -69,7 +62,6 @@ const Tooltip = () => {
           <>
             <div
               {...getTooltipProps({
-                ref: tooltipRef,
                 style: styles
               })}
             >

diff --git a/packages/tooltip/src/TooltipContainer.js b/packages/tooltip/src/TooltipContainer.js
@@ -16,7 +16,6 @@ export function TooltipContainer({ children, render = children, ...options }) {
 TooltipContainer.propTypes = {
   children: PropTypes.func,
   render: PropTypes.func,
-  tooltipRef: PropTypes.shape({ current: PropTypes.instanceOf(Element) }).isRequired,
   delayMilliseconds: PropTypes.number,
   isVisible: PropTypes.bool
 };
diff --git a/packages/tooltip/src/TooltipContainer.spec.js b/packages/tooltip/src/TooltipContainer.spec.js
@@ -5,7 +5,7 @@
  * found at http://www.apache.org/licenses/LICENSE-2.0.
  */
 
-import React, { useRef } from 'react';
+import React from 'react';
 import { render, fireEvent } from '@testing-library/react';
 import { act } from 'react-dom/test-utils';
 
@@ -17,17 +17,14 @@ describe('TooltipContainer', () => {
   const TOOLTIP_ID = 'test';
 
   const BasicExample = props => {
-    const tooltipRef = useRef(null);
-
     return (
-      <TooltipContainer tooltipRef={tooltipRef} id={TOOLTIP_ID} {...props}>
+      <TooltipContainer id={TOOLTIP_ID} {...props}>
         {({ getTooltipProps, getTriggerProps }) => (
           <>
             <div {...getTriggerProps({ 'data-test-id': 'trigger' })}>trigger</div>
             <div
               {...getTooltipProps({
-                'data-test-id': 'tooltip',
-                ref: tooltipRef
+                'data-test-id': 'tooltip'
               })}
             >
               tooltip

diff --git a/packages/tooltip/src/useTooltip.js b/packages/tooltip/src/useTooltip.js
@@ -5,12 +5,13 @@
  * found at http://www.apache.org/licenses/LICENSE-2.0.
  */
 
-import { useState, useEffect } from 'react';
+import { useState, useEffect, useRef } from 'react';
 import { composeEventHandlers, generateId, KEY_CODES } from '@zendeskgarden/container-utilities';
 
-export function useTooltip({ tooltipRef, delayMilliseconds = 500, id, isVisible } = {}) {
+export function useTooltip({ delayMilliseconds = 500, id, isVisible } = {}) {
   const [visibility, setVisibility] = useState(isVisible);
   const [_id] = useState(id || generateId('garden-tooltip-container'));
+  const isMounted = useRef(false);
 
   let openTooltipTimeout;
   let closeTooltipTimeout;
@@ -19,7 +20,7 @@ export function useTooltip({ tooltipRef, delayMilliseconds = 500, id, isVisible
     clearTimeout(closeTooltipTimeout);
 
     openTooltipTimeout = setTimeout(() => {
-      if (tooltipRef.current) {
+      if (isMounted.current) {
         setVisibility(true);
       }
     }, delayMs);
@@ -29,12 +30,23 @@ export function useTooltip({ tooltipRef, delayMilliseconds = 500, id, isVisible
     clearTimeout(openTooltipTimeout);
 
     closeTooltipTimeout = setTimeout(() => {
-      if (tooltipRef.current) {
+      if (isMounted.current) {
         setVisibility(false);
       }
     }, delayMs);
   };
 
+  // Sometimes the timeout will call setVisibility even after unmount and cleanup
+  // reproducable when running tests, happens when fast switching in storybook.
+  // May be related https://github.com/facebook/react/pull/15650
+  useEffect(() => {
+    isMounted.current = true;
+
+    return () => {
+      isMounted.current = false;
+    };
+  }, []);
+
   // Clean up stray timeouts if tooltip unmounts
   useEffect(() => {
     return () => {