feat(Spinner): make delay customizable

[francinelucca]

Closes https://github.com/github/primer/issues/6288

Changelog

Changed

• broadened Spinner's delay prop to accept a number for further customization

Rollout strategy

• Patch release
• Minor release
• Major release; if selected, include a written rollout or migration plan
• None; if selected, include a brief description as to why

Testing & Reviewing

Merge checklist

• Added/updated tests
• Added/updated documentation
• Added/updated previews (Storybook)
• Changes are SSR compatible
• Tested in Chrome
• Tested in Firefox
• Tested in Safari
• Tested in Edge
• (GitHub staff only) Integration tests pass at github/github (Learn more about how to run integration tests)

[changeset-bot]

🦋 Changeset detected

Latest commit: be45a92

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@primer/react	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[github-actions]

👋 Hi, this pull request contains changes to the source code that github/github-ui depends on. If you are GitHub staff, test these changes with github/github-ui using the integration workflow. Or, apply the integration-tests: skipped manually label to skip these checks.

[francinelucca]

@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?

[adierkens]

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

[siddharthkp]

Agree with this ^, true = default

[francinelucca]

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!

[siddharthkp]

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 😅

[Copilot]

Pull request overview

This PR enhances the Spinner component's delay prop to support custom delay durations. Previously, the delay prop only accepted a boolean value (true for 1000ms delay, false for no delay). Now it accepts boolean, the string literal 'default' for 1000ms, or a number for custom millisecond delays.

Key Changes:

• Extended the delay prop type from boolean to boolean | 'default' | number
• Updated delay calculation logic to handle numeric values
• Added comprehensive test coverage for the new 'default' and numeric delay options

Reviewed changes

Copilot reviewed 3 out of 4 changed files in this pull request and generated 1 comment.

File	Description
packages/react/src/Spinner/Spinner.tsx	Extended delay prop type definition and implementation to support 'default' string and custom numeric delays
packages/react/src/Spinner/Spinner.test.tsx	Added test cases for delay='default' and delay={number} scenarios including render timing and cleanup behavior
package-lock.json	Version bump from 38.6.2 to 38.7.0 for minor release

Comments suppressed due to low confidence (2)

packages/react/src/Spinner/Spinner.tsx:56

• The truthy check for delay will cause unexpected behavior when delay is set to 0. A value of 0 is falsy in JavaScript, so delay={0} will be treated the same as delay={false}, causing immediate rendering instead of running the setTimeout with 0ms delay. Consider using an explicit type check instead of a truthy check, such as: if (delay !== false && delay !== undefined)

This issue also appears in the following locations of the same file:

• line 47

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

      return () => clearTimeout(timeoutId)
    }
  }, [delay])

packages/react/src/Spinner/Spinner.tsx:56

• When the delay prop changes from a truthy value (true, 'default', or a number) to false while the component is mounted, the spinner will never become visible. The timeout gets cleared but isVisible remains false. Consider adding logic to set isVisible to true when delay becomes false: if (!delay) { setIsVisible(true) }

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

      return () => clearTimeout(timeoutId)
    }
  }, [delay])