diff --git a/.changeset/clean-fireants-type.md b/.changeset/clean-fireants-type.md new file mode 100644 index 00000000000..b41d46a7d66 --- /dev/null +++ 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 index f7c74f0f750..e48eb4a15a7 100644 --- 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 index aa2238aafc7..3453017099d 100644 --- 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).current.getBoundingClientRect() setChildrenWidth({text, width: domRect.width}) }, [ref, setChildrenWidth]) - return + return }) const sizeToHeight = { diff --git a/packages/react/src/ActionList/TrailingAction.tsx b/packages/react/src/ActionList/TrailingAction.tsx index f258dae4411..1b9df53370f 100644 --- 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 index d7b06d39db0..e2ae3db8108 100644 --- 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 index 48c16d2222c..87b4e6b3b06 100644 --- 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() + it('should render tooltip on an icon button by default', () => { + const {getByRole, getByText} = render() 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 index 301c8ded983..6b33ad5ab29 100644 --- 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 = () => ( - - + {/* We can remove these unsafe props after we resolve https://github.com/primer/react/issues/4129 */} + + ) diff --git a/packages/react/src/Dialog.tsx b/packages/react/src/Dialog.tsx index 50bb47d11cb..2139792b545 100644 --- 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 index 497a2148021..df439ad91c8 100644 --- 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 ( +
+ + setIsOpen(false)} + aria-labelledby="header" + > +
+ Title + + Some content + + +
+
+
+ ) +} + 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() + const {getByTestId, queryByTestId} = HTMLRender() + 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 index 8d4b3d7dd25..8b138d747d8 100644 --- a/packages/react/src/drafts/MarkdownEditor/_ToolbarButton.tsx +++ b/packages/react/src/drafts/MarkdownEditor/_ToolbarButton.tsx @@ -16,6 +16,8 @@ export const ToolbarButton = forwardRef((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 index 2160c1b8c01..b95183ebd26 100644 --- 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 void > {onBack ? ( - - onBack()} - /> - + onBack()} + /> ) : null} @@ -428,15 +425,16 @@ const SelectPanelHeader: React.FC void - {/* Will not need tooltip after https://github.com/primer/react/issues/2008 */} {onClearSelection ? ( - - - + ) : null} - - onCancel()} /> - + onCancel()} />