code review comments

primer · Mar 1, 2024 · 290e33e · 290e33e
1 parent 480b6aa
commit 290e33e
Show file tree

Hide file tree

Showing 4 changed files with 23 additions and 11 deletions.
diff --git a/packages/react/src/ActionMenu/ActionMenu.tsx b/packages/react/src/ActionMenu/ActionMenu.tsx
@@ -148,12 +148,12 @@ const Overlay: React.FC<React.PropsWithChildren<MenuOverlayProps>> = ({
   useMenuKeyboardNavigation(open, onClose, containerRef, anchorRef)
 
   // If the menu anchor is an icon button, we need to label the menu by tooltip that also labelled the anchor.
-  const [tooltipId, setTooltipId] = useState<null | string>(null)
+  const [anchorAriaLabelledby, setAnchorAriaLabelledby] = useState<null | string>(null)
   useEffect(() => {
     if (anchorRef.current) {
-      const anchorAriaLabelledby = anchorRef.current.getAttribute('aria-labelledby')
-      if (anchorAriaLabelledby) {
-        setTooltipId(anchorAriaLabelledby)
+      const ariaLabelledby = anchorRef.current.getAttribute('aria-labelledby')
+      if (ariaLabelledby) {
+        setAnchorAriaLabelledby(ariaLabelledby)
       }
     }
   }, [anchorRef])
@@ -176,8 +176,8 @@ const Overlay: React.FC<React.PropsWithChildren<MenuOverlayProps>> = ({
           value={{
             container: 'ActionMenu',
             listRole: 'menu',
-            // If there is a custom aria-labelledby, use that. Otherwise, use the tooltipId if exist. If not, use anchor id.
-            listLabelledBy: ariaLabelledby || tooltipId || anchorId,
+            // If there is a custom aria-labelledby, use that. Otherwise, if exists, use the id that labels the anchor such as tooltip. If none of them exist, use anchor id.
+            listLabelledBy: ariaLabelledby || anchorAriaLabelledby || anchorId,
             selectionAttribute: 'aria-checked', // Should this be here?
             afterSelect: onClose,
           }}

diff --git a/packages/react/src/Button/IconButton.features.stories.tsx b/packages/react/src/Button/IconButton.features.stories.tsx
@@ -4,6 +4,7 @@ import {IconButton} from '.'
 import {ActionMenu} from '../ActionMenu'
 import {ActionList} from '../ActionList'
 import {Tooltip} from '../TooltipV2'
+import {default as TooltipV1} from '../Tooltip'
 
 export default {
   title: 'Components/IconButton/Features',
@@ -37,10 +38,16 @@ export const ExternalTooltip = () => (
   </Tooltip>
 )
 
+export const ExternalTooltipVersion1 = () => (
+  <TooltipV1 text="this is a supportive description for icon button" direction="se">
+    <IconButton icon={HeartIcon} aria-label="HeartIcon" />
+  </TooltipV1>
+)
+
 export const AsAMenuAnchor = () => (
   <ActionMenu>
     <ActionMenu.Anchor>
-      <IconButton icon={ChevronDownIcon} aria-label="Open Menu" />
+      <IconButton icon={ChevronDownIcon} aria-label="Something" />
     </ActionMenu.Anchor>
 
     <ActionMenu.Overlay width="medium">

diff --git a/packages/react/src/Tooltip/Tooltip.tsx b/packages/react/src/Tooltip/Tooltip.tsx
@@ -1,5 +1,5 @@
 import clsx from 'clsx'
-import React from 'react'
+import React, {useMemo} from 'react'
 import styled from 'styled-components'
 import {get} from '../constants'
 import type {SxProp} from '../sx'
@@ -259,9 +259,11 @@ function Tooltip({direction = 'n', children, className, text, noDelay, align, wr
     noDelay && 'tooltipped-no-delay',
     wrap && 'tooltipped-multiline',
   )
+
+  const value = useMemo(() => ({tooltipId}), [tooltipId])
   return (
     // This provider is used to check if an icon button is wrapped with tooltip or not.
-    <TooltipContext.Provider value={{tooltipId}}>
+    <TooltipContext.Provider value={value}>
       <TooltipBase role="tooltip" aria-label={text} {...rest} className={classes}>
         {children}
       </TooltipBase>

diff --git a/packages/react/src/TooltipV2/Tooltip.tsx b/packages/react/src/TooltipV2/Tooltip.tsx
@@ -1,4 +1,4 @@
-import React, {Children, useEffect, useRef, useState} from 'react'
+import React, {Children, useEffect, useRef, useState, useMemo} from 'react'
 import type {SxProp} from '../sx'
 import sx from '../sx'
 import {useId, useProvidedRefOrCreate} from '../hooks'
@@ -205,6 +205,9 @@ export const Tooltip = React.forwardRef(
       }
     }
 
+    // context value
+    const value = useMemo(() => ({tooltipId}), [tooltipId])
+
     useEffect(() => {
       if (!tooltipElRef.current || !triggerRef.current) return
       /*
@@ -268,7 +271,7 @@ export const Tooltip = React.forwardRef(
     }, [tooltipElRef, triggerRef, direction, type])
 
     return (
-      <TooltipContext.Provider value={{tooltipId}}>
+      <TooltipContext.Provider value={value}>
         <>
           {React.isValidElement(child) &&
             React.cloneElement(child as React.ReactElement<TriggerPropsType>, {