Skip to content

Fix Details flickering #7020

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
merged 7 commits into from
Oct 21, 2025
Merged

Fix Details flickering #7020

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e22bc37
WIP
hectahertz Oct 16, 2025
2338ef4
Improve message
hectahertz Oct 17, 2025
529952c
Remove outdated tests
hectahertz Oct 17, 2025
5610063
Add changeset
hectahertz Oct 17, 2025
45b56b2
Merge branch 'main' into hectahertz/fix-details-flickering
hectahertz Oct 17, 2025
75a4d5c
Separate the story to a new file
hectahertz Oct 20, 2025
488ab49
Merge branch 'main' into hectahertz/fix-details-flickering
hectahertz Oct 20, 2025
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
5 changes: 5 additions & 0 deletions .changeset/dark-birds-dont.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@primer/react': patch
---

Fixes `Details` flickering, prevents re-renders
18 changes: 18 additions & 0 deletions packages/react/src/Details/Details.features.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import type {StoryFn, Meta} from '@storybook/react-vite'
import Details from './Details'
import useDetails from '../hooks/useDetails'

export default {
title: 'Components/Details/Features',
component: Details,
} as Meta<typeof Details>

export const WithCustomSummary: StoryFn<typeof Details> = () => {
const {getDetailsProps} = useDetails({closeOnOutsideClick: true})
return (
<Details {...getDetailsProps()}>
<summary>Custom see Details</summary>
This is some content
</Details>
)
}
36 changes: 11 additions & 25 deletions packages/react/src/Details/Details.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import React, {useEffect, useState, type ComponentPropsWithoutRef, type ReactElement} from 'react'
import React, {useEffect, type ComponentPropsWithoutRef, type ReactElement} from 'react'
import {warning} from '../utils/warning'
import {clsx} from 'clsx'
import classes from './Details.module.css'
import {useMergedRefs} from '../internal/hooks/useMergedRefs'
Expand All @@ -7,40 +8,25 @@ const Root = React.forwardRef<HTMLDetailsElement, DetailsProps>(
({className, children, ...rest}, forwardRef): ReactElement => {
const detailsRef = React.useRef<HTMLDetailsElement>(null)
const ref = useMergedRefs(forwardRef, detailsRef)
const [hasSummary, setHasSummary] = useState(false)

useEffect(() => {
const {current: details} = detailsRef
if (!details) {
if (!__DEV__) {
return
}

const updateSummary = () => {
const summary = details.querySelector('summary:not([data-default-summary])')
setHasSummary(!!summary)
}

// Update summary on mount
updateSummary()

const observer = new MutationObserver(() => {
updateSummary()
})

observer.observe(details, {
childList: true,
subtree: true,
})

return () => {
observer.disconnect()
const {current: details} = detailsRef
if (!details) {
return
}
const summary = details.querySelector('summary:not([data-default-summary])')
warning(
summary === null,
'The <Details> component must have a <summary> child component. You can either use <Details.Summary> or a native <summary> element.',
)
Comment on lines +22 to +25
Copy link

Copilot AI Oct 17, 2025

Choose a reason for hiding this comment

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

The warning condition is inverted. It currently warns when a summary is present (summary === null evaluates to true only when summary is null). It should warn when summary is not present. Change to summary !== null or !summary.

Copilot uses AI. Check for mistakes.

}, [])

return (
<details className={clsx(className, classes.Details)} {...rest} ref={ref}>
{/* Include default summary if summary is not provided */}
{!hasSummary && <Details.Summary data-default-summary>{'See Details'}</Details.Summary>}
{children}
</details>
)
Expand Down
33 changes: 0 additions & 33 deletions packages/react/src/Details/__tests__/Details.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,39 +82,6 @@ describe('Details', () => {
expect(getByTestId('summary')).toHaveTextContent('Open')
})

it('Adds default summary if no summary supplied', async () => {
const {getByText} = render(<Details data-testid="details">content</Details>)

expect(getByText('See Details')).toBeInTheDocument()
expect(getByText('See Details').tagName).toBe('SUMMARY')
})

it('Does not add default summary if summary supplied', async () => {
const {findByTestId, findByText} = render(
<Details data-testid="details">
<Details.Summary data-testid="summary">summary</Details.Summary>
content
</Details>,
)

await expect(findByText('See Details')).rejects.toThrow()
expect(await findByTestId('summary')).toBeInTheDocument()
expect((await findByTestId('summary')).tagName).toBe('SUMMARY')
})

it('Does not add default summary if supplied as different element', async () => {
const {findByTestId, findByText} = render(
<Details data-testid="details">
<summary data-testid="summary">custom summary</summary>
content
</Details>,
)

await expect(findByText('See Details')).rejects.toThrow()
expect(await findByTestId('summary')).toBeInTheDocument()
expect((await findByTestId('summary')).tagName).toBe('SUMMARY')
})

describe('Details.Summary', () => {
it('should support a custom `className` on the container element', () => {
render(<Details.Summary className="custom-class">test summary</Details.Summary>)
Expand Down
Loading