Update the default value of unsafeDisableTooltip to false and rem…

…ove redundant props and wrappers (#4702) * Update the default value of unsafeDisableTooltip to false and remove unncessary props and wrappers * test and lint fix * Create clean-fireants-type.md * update actionbar e2e test selector * disable tooltips in button group stories * update Dialog tests * fix linting * update tests
primer · Jul 9, 2024 · 2536b49 · 2536b49
1 parent 1976ee1
commit 2536b49
Show file tree

Hide file tree

Showing 11 changed files with 72 additions and 31 deletions.
diff --git a/.changeset/clean-fireants-type.md b/.changeset/clean-fireants-type.md
@@ -0,0 +1,8 @@
+---
+"@primer/react": minor
+---
+
+IconButton: Enable tooltips by default in icon buttons by updating the default value of `unsafeDisableTooltip` to `false`.
+
+This is a behaviour change in icon buttons, please upgrade with a caution.  
+
diff --git a/e2e/components/drafts/ActionBar.test.ts b/e2e/components/drafts/ActionBar.test.ts
@@ -44,8 +44,8 @@ test.describe('ActionBar', () => {
           await expect(page.locator(toolbarButtonSelector)).toHaveCount(10)
           await page.setViewportSize({width: viewports['primer.breakpoint.xs'], height: 768})
           await expect(page.locator(toolbarButtonSelector)).toHaveCount(6)
-          const moreButtonSelector = `button[aria-label="More Comment box toolbar items"]`
-          await page.locator(moreButtonSelector).click()
+          const moreButtonSelector = page.getByLabel('More Comment box toolbar items')
+          await moreButtonSelector.click()
           await expect(page.locator('ul[role="menu"]>li')).toHaveCount(5)
         })
       })

diff --git a/packages/react/src/ActionBar/ActionBar.tsx b/packages/react/src/ActionBar/ActionBar.tsx
@@ -299,7 +299,7 @@ export const ActionBarIconButton = forwardRef((props: ActionBarIconButtonProps,
     const domRect = (ref as MutableRefObject<HTMLElement>).current.getBoundingClientRect()
     setChildrenWidth({text, width: domRect.width})
   }, [ref, setChildrenWidth])
-  return <IconButton ref={ref} size={size} {...props} variant="invisible" unsafeDisableTooltip={false} />
+  return <IconButton ref={ref} size={size} {...props} variant="invisible" />
 })
 
 const sizeToHeight = {

diff --git a/packages/react/src/ActionList/TrailingAction.tsx b/packages/react/src/ActionList/TrailingAction.tsx
@@ -48,7 +48,6 @@ export const TrailingAction = forwardRef(({as = 'button', icon, label, href = nu
           aria-label={label}
           icon={icon}
           variant="invisible"
-          unsafeDisableTooltip={false}
           tooltipDirection="w"
           href={href}
           // @ts-expect-error StyledButton wants both Anchor and Button refs

diff --git a/packages/react/src/Button/IconButton.tsx b/packages/react/src/Button/IconButton.tsx
@@ -17,7 +17,7 @@ const IconButton = forwardRef(
       disabled,
       tooltipDirection,
       // This is planned to be a temporary prop until the default tooltip on icon buttons are fully rolled out.
-      unsafeDisableTooltip = true,
+      unsafeDisableTooltip = false,
       ...props
     },
     forwardedRef,

diff --git a/packages/react/src/Button/__tests__/Button.test.tsx b/packages/react/src/Button/__tests__/Button.test.tsx
@@ -131,10 +131,11 @@ describe('Button', () => {
     const tooltipEl = getByText('Love is all around')
     expect(triggerEL).toHaveAttribute('aria-describedby', tooltipEl.id)
   })
-  it('should not render tooltip on an icon button by default', () => {
-    const {getByRole} = render(<IconButton icon={HeartIcon} aria-label="Heart" />)
+  it('should render tooltip on an icon button by default', () => {
+    const {getByRole, getByText} = render(<IconButton icon={HeartIcon} aria-label="Heart" />)
     const triggerEl = getByRole('button')
-    expect(triggerEl).not.toHaveAttribute('aria-labelledby')
-    expect(triggerEl).toHaveAttribute('aria-label', 'Heart')
+    const tooltipEl = getByText('Heart')
+    expect(triggerEl).toHaveAttribute('aria-labelledby', tooltipEl.id)
+    expect(triggerEl).not.toHaveAttribute('aria-label')
   })
 })
diff --git a/packages/react/src/ButtonGroup/ButtonGroup.features.stories.tsx b/packages/react/src/ButtonGroup/ButtonGroup.features.stories.tsx
@@ -11,7 +11,8 @@ export default {
 
 export const IconButtons = () => (
   <ButtonGroup>
-    <IconButton icon={PlusIcon} aria-label="Add" />
-    <IconButton icon={DashIcon} aria-label="Subtract" />
+    {/* We can remove these unsafe props after we resolve https://github.com/primer/react/issues/4129 */}
+    <IconButton unsafeDisableTooltip={true} icon={PlusIcon} aria-label="Add" />
+    <IconButton unsafeDisableTooltip={true} icon={DashIcon} aria-label="Subtract" />
   </ButtonGroup>
 )
diff --git a/packages/react/src/Dialog.tsx b/packages/react/src/Dialog.tsx
@@ -11,6 +11,7 @@ import type {ComponentProps} from './utils/types'
 import {useRefObjectAsForwardedRef} from './hooks/useRefObjectAsForwardedRef'
 import {XIcon} from '@primer/octicons-react'
 
+// Dialog v1
 const noop = () => null
 
 type StyledDialogBaseProps = {

diff --git a/packages/react/src/__tests__/Dialog.test.tsx b/packages/react/src/__tests__/Dialog.test.tsx
@@ -1,6 +1,5 @@
 import React, {useState, useRef} from 'react'
-import {Dialog, Box, Text} from '..'
-import {Button} from '../deprecated'
+import {Dialog, Box, Text, Button} from '..'
 import {render as HTMLRender, fireEvent} from '@testing-library/react'
 import axe from 'axe-core'
 import {behavesAsComponent, checkExports} from '../utils/testing'
@@ -71,6 +70,36 @@ const DialogWithCustomFocusRef = () => {
   )
 }
 
+const DialogWithCustomFocusRefAndReturnFocusRef = () => {
+  const [isOpen, setIsOpen] = useState(true)
+  const returnFocusRef = useRef(null)
+  const buttonRef = useRef(null)
+  return (
+    <div>
+      <Button data-testid="trigger-button" ref={returnFocusRef} onClick={() => setIsOpen(true)}>
+        Show Dialog
+      </Button>
+      <Dialog
+        returnFocusRef={returnFocusRef}
+        isOpen={isOpen}
+        initialFocusRef={buttonRef}
+        onDismiss={() => setIsOpen(false)}
+        aria-labelledby="header"
+      >
+        <div data-testid="inner">
+          <Dialog.Header id="header">Title</Dialog.Header>
+          <Box p={3}>
+            <Text fontFamily="sans-serif">Some content</Text>
+            <button data-testid="inner-button" ref={buttonRef}>
+              hi
+            </button>
+          </Box>
+        </div>
+      </Dialog>
+    </div>
+  )
+}
+
 describe('Dialog', () => {
   // because Dialog returns a React fragment the as and sx tests fail always, so they are skipped
   behavesAsComponent({
@@ -140,7 +169,9 @@ describe('Dialog', () => {
   })
 
   it('Returns focus to returnFocusRef on escape', async () => {
-    const {getByTestId, queryByTestId} = HTMLRender(<Component />)
+    const {getByTestId, queryByTestId} = HTMLRender(<DialogWithCustomFocusRefAndReturnFocusRef />)
+    const innerButton = getByTestId('inner-button')
+    expect(document.activeElement).toEqual(innerButton)
 
     expect(getByTestId('inner')).toBeTruthy()
     fireEvent.keyDown(document.body, {key: 'Escape'})

diff --git a/packages/react/src/drafts/MarkdownEditor/_ToolbarButton.tsx b/packages/react/src/drafts/MarkdownEditor/_ToolbarButton.tsx
@@ -16,6 +16,8 @@ export const ToolbarButton = forwardRef<HTMLButtonElement, IconButtonProps>((pro
       onMouseDown={(e: React.MouseEvent) => e.preventDefault()}
       {...props}
       sx={{color: 'fg.muted', ...props.sx}}
+      // Keeping the tooltip disable since it is not maintained anymore and its tests were failing.
+      unsafeDisableTooltip
     />
   )
 })

diff --git a/packages/react/src/drafts/SelectPanel2/SelectPanel.tsx b/packages/react/src/drafts/SelectPanel2/SelectPanel.tsx
@@ -7,7 +7,6 @@ import {
   IconButton,
   Heading,
   Box,
-  Tooltip,
   TextInput,
   Spinner,
   Text,
@@ -401,15 +400,13 @@ const SelectPanelHeader: React.FC<React.PropsWithChildren & {onBack?: () => void
       >
         <Box sx={{display: 'flex'}}>
           {onBack ? (
-            <Tooltip text="Back" direction="s">
-              <IconButton
-                type="button"
-                variant="invisible"
-                icon={ArrowLeftIcon}
-                aria-label="Back"
-                onClick={() => onBack()}
-              />
-            </Tooltip>
+            <IconButton
+              type="button"
+              variant="invisible"
+              icon={ArrowLeftIcon}
+              aria-label="Back"
+              onClick={() => onBack()}
+            />
           ) : null}
 
           <Box sx={{marginLeft: onBack ? 1 : 2, marginTop: description ? '2px' : 0}}>
@@ -428,15 +425,16 @@ const SelectPanelHeader: React.FC<React.PropsWithChildren & {onBack?: () => void
         </Box>
 
         <Box>
-          {/* Will not need tooltip after https://github.com/primer/react/issues/2008 */}
           {onClearSelection ? (
-            <Tooltip text="Clear selection" direction="s" onClick={onClearSelection}>
-              <IconButton type="button" variant="invisible" icon={FilterRemoveIcon} aria-label="Clear selection" />
-            </Tooltip>
+            <IconButton
+              type="button"
+              variant="invisible"
+              icon={FilterRemoveIcon}
+              aria-label="Clear selection"
+              onClick={onClearSelection}
+            />
           ) : null}
-          <Tooltip text="Close" direction="s">
-            <IconButton type="button" variant="invisible" icon={XIcon} aria-label="Close" onClick={() => onCancel()} />
-          </Tooltip>
+          <IconButton type="button" variant="invisible" icon={XIcon} aria-label="Close" onClick={() => onCancel()} />
         </Box>
       </Box>