Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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/rotten-mugs-tan.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@primer/react': minor
---

feat: make Spinner's delay customizable
8 changes: 4 additions & 4 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

56 changes: 56 additions & 0 deletions packages/react/src/Spinner/Spinner.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -94,5 +94,61 @@ describe('Spinner', () => {
// No errors should occur
expect(true).toBe(true)
})

it('should render after 1000ms when delay is default', () => {
const {container} = render(<Spinner delay="default" />)

// Not visible initially
expect(container.querySelector('svg')).not.toBeInTheDocument()

// Advance timers by 1000ms
act(() => {
vi.advanceTimersByTime(1000)
})

// Now it should be visible
expect(container.querySelector('svg')).toBeInTheDocument()
})

it('should cleanup timeout on unmount when delay is default', () => {
const {unmount} = render(<Spinner delay={'default'} />)

// Unmount before the delay completes
unmount()

// Advance timers to see if there are any side effects
vi.advanceTimersByTime(1000)

// No errors should occur
expect(true).toBe(true)
})

it('should render after custom ms when delay is a number', () => {
const {container} = render(<Spinner delay={500} />)

// Not visible initially
expect(container.querySelector('svg')).not.toBeInTheDocument()

// Advance timers by 500ms
act(() => {
vi.advanceTimersByTime(500)
})

// Now it should be visible
expect(container.querySelector('svg')).toBeInTheDocument()
})

it('should cleanup timeout on unmount when delay is a number', () => {
const {unmount} = render(<Spinner delay={500} />)

// Unmount before the delay completes
unmount()

// Advance timers to see if there are any side effects
vi.advanceTimersByTime(500)

// No errors should occur
expect(true).toBe(true)
})
})
})
5 changes: 3 additions & 2 deletions packages/react/src/Spinner/Spinner.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ export type SpinnerProps = {
className?: string
style?: React.CSSProperties
/** Whether to delay the spinner before rendering by the defined 1000ms. */
delay?: boolean
delay?: boolean | 'default' | number
Copy link
Member Author

Choose a reason for hiding this comment

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

@siddharthkp curious on your thoughts here since this implementation was your idea. I didn't feel confident removing the boolean type initially because that felt like a major change, so I just added the other two. I'm thinking we can go back and cleanup the boolean type on v39?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is the 'default' option even needed? Could we just use true for 1s or a number for a custom value?

Copy link
Member

Choose a reason for hiding this comment

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

Agree with this ^, true = default

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, def can. I was just thinking what the API would look like if we were building it from scratch with the requirements we have now ('default', number), but happy to defer to boolean, number instead!

Copy link
Member

@siddharthkp siddharthkp Jan 13, 2026

Choose a reason for hiding this comment

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

Thinking from scratch, I would have said delay = short | long and not number.

But, seeing how we don't have strong opinions on what those numbers should be and there is only 1 instance of delay in dotcom, I'm happy to open it up to being a number and then codifying a few values based on usage in the future.

Optional side note: Does delayMs: number sound better? Works if we silently remove delay: boolean 😅

} & HTMLDataAttributes

function Spinner({
Expand All @@ -46,9 +46,10 @@ function Spinner({

useEffect(() => {
if (delay) {
const delayDuration = typeof delay === 'number' ? delay : 1000
const timeoutId = setTimeout(() => {
setIsVisible(true)
}, 1000)
}, delayDuration)

return () => clearTimeout(timeoutId)
}
Expand Down
Loading