From af79a10d001bc1376439e7194be4099d1d38ff5c Mon Sep 17 00:00:00 2001 From: Armagan Ersoz Date: Thu, 27 Jun 2024 15:07:47 +1000 Subject: [PATCH 1/4] Update the default value of unsafeDisableTooltip to false and remove unncessary props and wrappers --- packages/react/src/ActionBar/ActionBar.tsx | 2 +- .../react/src/ActionList/TrailingAction.tsx | 1 - packages/react/src/Button/IconButton.tsx | 2 +- .../src/drafts/SelectPanel2/SelectPanel.tsx | 31 +++++++++---------- 4 files changed, 17 insertions(+), 19 deletions(-) diff --git a/packages/react/src/ActionBar/ActionBar.tsx b/packages/react/src/ActionBar/ActionBar.tsx index aa2238aafc7c..3453017099d2 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 f258dae44117..1b9df53370f7 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 d7b06d39db08..e2ae3db81089 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/drafts/SelectPanel2/SelectPanel.tsx b/packages/react/src/drafts/SelectPanel2/SelectPanel.tsx index 48853b46e096..029628f0ffae 100644 --- a/packages/react/src/drafts/SelectPanel2/SelectPanel.tsx +++ b/packages/react/src/drafts/SelectPanel2/SelectPanel.tsx @@ -401,15 +401,13 @@ const SelectPanelHeader: React.FC void > {onBack ? ( - - onBack()} - /> - + onBack()} + /> ) : null} @@ -428,15 +426,16 @@ const SelectPanelHeader: React.FC void - {/* Will not need tooltip after https://github.com/primer/react/issues/2008 */} {onClearSelection ? ( - - - + ) : null} - - onCancel()} /> - + onCancel()} /> From fdb2daef89fd2a46a75ff99f54c09c32400255c4 Mon Sep 17 00:00:00 2001 From: Armagan Ersoz Date: Thu, 27 Jun 2024 16:18:44 +1000 Subject: [PATCH 2/4] test and lint fix --- packages/react/src/Button/__tests__/Button.test.tsx | 9 +++++---- .../react/src/drafts/MarkdownEditor/_ToolbarButton.tsx | 2 ++ packages/react/src/drafts/SelectPanel2/SelectPanel.tsx | 1 - 3 files changed, 7 insertions(+), 5 deletions(-) diff --git a/packages/react/src/Button/__tests__/Button.test.tsx b/packages/react/src/Button/__tests__/Button.test.tsx index 48c16d2222c1..87b4e6b3b068 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/drafts/MarkdownEditor/_ToolbarButton.tsx b/packages/react/src/drafts/MarkdownEditor/_ToolbarButton.tsx index 8d4b3d7dd251..8b138d747d82 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 029628f0ffae..a5e798fdd72f 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, From 71bfea397cfd4aa47c49457406f14a878eb5ea61 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Arma=C4=9Fan?= Date: Thu, 27 Jun 2024 16:45:40 +1000 Subject: [PATCH 3/4] Create clean-fireants-type.md --- .changeset/clean-fireants-type.md | 8 ++++++++ 1 file changed, 8 insertions(+) create mode 100644 .changeset/clean-fireants-type.md diff --git a/.changeset/clean-fireants-type.md b/.changeset/clean-fireants-type.md new file mode 100644 index 000000000000..b41d46a7d66c --- /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. + From 91863825ca7e2f31445225a34b2b70a27ad72f66 Mon Sep 17 00:00:00 2001 From: Armagan Ersoz Date: Fri, 28 Jun 2024 12:53:38 +1000 Subject: [PATCH 4/4] update actionbar e2e test selector --- e2e/components/drafts/ActionBar.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/e2e/components/drafts/ActionBar.test.ts b/e2e/components/drafts/ActionBar.test.ts index f7c74f0f750a..e48eb4a15a76 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) }) })