Skip to content

Update the default value of unsafeDisableTooltip to false and remove redundant props and wrappers#4702

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
broccolinisoup merged 9 commits into from
Jul 9, 2024
Merged

Update the default value of unsafeDisableTooltip to false and remove redundant props and wrappers #4702

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
af79a10
Update the default value of unsafeDisableTooltip to false and remove …
broccolinisoup Jun 27, 2024
fdb2dae
test and lint fix
broccolinisoup Jun 27, 2024
71bfea3
Create clean-fireants-type.md
broccolinisoup Jun 27, 2024
9186382
update actionbar e2e test selector
broccolinisoup Jun 28, 2024
6f6482a
disable tooltips in button group stories
broccolinisoup Jun 28, 2024
f5ffdc4
update Dialog tests
broccolinisoup Jun 28, 2024
51ee65f
fix linting
broccolinisoup Jun 28, 2024
a659cc9
Merge branch 'main' into default-tooltip-icon-button
broccolinisoup Jul 3, 2024
fd6bd57
update tests
broccolinisoup Jul 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions .changeset/clean-fireants-type.md
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -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.

4 changes: 2 additions & 2 deletions e2e/components/drafts/ActionBar.test.ts
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -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)
})
})
Expand Down
2 changes: 1 addition & 1 deletion packages/react/src/ActionBar/ActionBar.tsx
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -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 = {
Expand Down
1 change: 0 additions & 1 deletion packages/react/src/ActionList/TrailingAction.tsx
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -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
Expand Down
2 changes: 1 addition & 1 deletion packages/react/src/Button/IconButton.tsx
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -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,
Expand Down
9 changes: 5 additions & 4 deletions packages/react/src/Button/__tests__/Button.test.tsx
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -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')
})
})
5 changes: 3 additions & 2 deletions packages/react/src/ButtonGroup/ButtonGroup.features.stories.tsx
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -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 */}

@siddharthkpsiddharthkpJul 8, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Good to know!

<IconButton unsafeDisableTooltip={true} icon={PlusIcon} aria-label="Add" />
<IconButton unsafeDisableTooltip={true} icon={DashIcon} aria-label="Subtract" />
</ButtonGroup>
)
1 change: 1 addition & 0 deletions packages/react/src/Dialog.tsx
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -11,6 +11,7 @@ import type {ComponentProps} from './utils/types'
import {useRefObjectAsForwardedRef} from './hooks/useRefObjectAsForwardedRef'
import {XIcon} from '@primer/octicons-react'

// Dialog v1

@siddharthkpsiddharthkpJul 8, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Classic! 😅

const noop = () => null

type StyledDialogBaseProps = {
Expand Down
37 changes: 34 additions & 3 deletions packages/react/src/__tests__/Dialog.test.tsx
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -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'
Expand DownExpand Up@@ -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({
Expand DownExpand Up@@ -140,7 +169,9 @@ describe('Dialog', () => {
})

it('Returns focus to returnFocusRef on escape', async () => {
const {getByTestId, queryByTestId} = HTMLRender(<Component />)

@broccolinisoupbroccolinisoupJul 3, 2024

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had an issue with this existing component. It was supposed to go back to the return ref after pressing the ESC twice (First closing the tooltip, second closing the dialog) but for some reason it didn't work, even though I waited for the previous keyboard event to be completed. This test wasn't about the tooltip itself; it is about the Dialog so I updated to use a different dialog but let me know if I am misinterpreting the issue I encountered.

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'})
Expand Down
2 changes: 2 additions & 0 deletions packages/react/src/drafts/MarkdownEditor/_ToolbarButton.tsx
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -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
/>
)
})
Expand Down
32 changes: 15 additions & 17 deletions packages/react/src/drafts/SelectPanel2/SelectPanel.tsx
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -7,7 +7,6 @@ import {
IconButton,
Heading,
Box,
Tooltip,
TextInput,
Spinner,
Text,
Expand DownExpand Up@@ -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}}>
Expand All@@ -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>

Expand Down