feat(Spinner): Adds a delay prop to the Spinner component that delays rendering by 1000ms. #7059
Conversation
🦋 Changeset detectedLatest commit: c3e91bd The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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
bot
added
staff
|
👋 Hi, this pull request contains changes to the source code that github/github depends on. If you are GitHub staff, we recommend testing these changes with github/github using the integration workflow. Thanks! |
Changed the version of @primer/react from patch to minor and added a delay prop to the Spinner component.
github-actions
bot
requested a deployment
to
storybook-preview-7059
| /** Number of milliseconds to delay the spinner before rendering. */ | ||
| delay?: number |
There was a problem hiding this comment.
One idea for this with our guidelines could be to have it be a boolean that uses our 1000ms value by default instead of everyone needing to supply that value manually.
jonrohan
changed the title
github-actions
bot
requested a deployment
to
storybook-preview-7059
There was a problem hiding this comment.
Pull Request Overview
This PR adds a delay prop to the Spinner component that delays rendering by 1000ms, helping to prevent spinner flash for quick loading states and improving user experience.
Key Changes
- Added
delayboolean prop (defaults tofalse) that delays spinner rendering by 1000ms using React hooks - Implemented proper timeout cleanup on component unmount
- Added comprehensive test coverage with 4 new test cases covering immediate rendering, delayed rendering, and cleanup behavior
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/react/src/Spinner/Spinner.tsx | Added delay logic using useState and useEffect to conditionally render spinner after 1000ms timeout |
| packages/react/src/Spinner/Spinner.test.tsx | Added fake timer setup and 4 test cases to verify delay behavior and cleanup |
| packages/react/src/Spinner/Spinner.features.stories.tsx | Added WithDelay story to demonstrate the delay functionality |
| packages/react/src/Spinner/Spinner.docs.json | Updated documentation to include delay prop definition and new story reference |
| .changeset/shaggy-pants-remain.md | Added changeset marking this as a minor release |
| beforeEach(() => { | ||
| vi.useFakeTimers() | ||
| }) | ||
|
|
||
| afterEach(() => { | ||
| vi.restoreAllMocks() | ||
| vi.useRealTimers() | ||
| }) |
There was a problem hiding this comment.
The fake timers setup in beforeEach affects all tests in the file, including existing tests that don't need fake timers. This could cause issues with tests that rely on real timers or async behavior (e.g., tests that use waitFor or other time-dependent operations).
Consider scoping the fake timer setup to only the delay-related tests by moving it inside those specific test cases or creating a separate describe block for delay tests:
describe('delay behavior', () => {
beforeEach(() => {
vi.useFakeTimers()
})
afterEach(() => {
vi.restoreAllMocks()
vi.useRealTimers()
})
it('should render immediately when delay is false', () => {
// test code
})
// other delay tests...
})| { | ||
| "a11yReviewed": "2025-01-08", | ||
| "id": "spinner", | ||
| "importPath": "@primer/react", | ||
| "name": "Spinner", | ||
| "status": "alpha", | ||
| "a11yReviewed": "2025-01-08", | ||
| "stories": [ | ||
| "props": [ | ||
| { | ||
| "id": "components-spinner--default" | ||
| "description": "Sets the width and height of the spinner.", | ||
| "name": "size", | ||
| "type": "'small' | 'medium' | 'large'" | ||
| }, | ||
| { | ||
| "id": "components-spinner-features--small" | ||
| "defaultValue": "Loading", | ||
| "description": "Sets the text conveyed by assistive technologies such as screen readers. Set to `null` if the loading state is displayed in a text node somewhere else on the page.", | ||
| "name": "srText", | ||
| "type": "string | null" | ||
| }, | ||
| { | ||
| "id": "components-spinner-features--large" | ||
| "deprecated": true, | ||
| "description": "Sets the text conveyed by assistive technologies such as screen readers.", | ||
| "name": "aria-label", | ||
| "type": "string" | ||
| }, | ||
| { | ||
| "id": "components-spinner-features--suppress-screen-reader-text" | ||
| "defaultValue": "", | ||
| "description": "", | ||
| "name": "className", | ||
| "type": "string" | ||
| }, | ||
| { | ||
| "name": "data-*", | ||
| "type": "string" | ||
| }, | ||
| { | ||
| "defaultValue": "false", | ||
| "description": "Whether to delay the spinner before rendering by the defined 1000ms.", | ||
| "name": "delay", | ||
| "type": "boolean" | ||
| } | ||
| ], | ||
| "importPath": "@primer/react", | ||
| "props": [ | ||
| "status": "alpha", | ||
| "stories": [ | ||
| { | ||
| "name": "size", | ||
| "type": "'small' | 'medium' | 'large'", | ||
| "description": "Sets the width and height of the spinner." | ||
| "id": "components-spinner--default" | ||
| }, | ||
| { | ||
| "name": "srText", | ||
| "type": "string | null", | ||
| "defaultValue": "Loading", | ||
| "description": "Sets the text conveyed by assistive technologies such as screen readers. Set to `null` if the loading state is displayed in a text node somewhere else on the page." | ||
| "id": "components-spinner-features--small" | ||
| }, | ||
| { | ||
| "name": "aria-label", | ||
| "type": "string", | ||
| "description": "Sets the text conveyed by assistive technologies such as screen readers.", | ||
| "deprecated": true | ||
| "id": "components-spinner-features--large" | ||
| }, | ||
| { | ||
| "name": "className", | ||
| "type": "string", | ||
| "description": "", | ||
| "defaultValue": "" | ||
| "id": "components-spinner-features--suppress-screen-reader-text" | ||
| }, | ||
| { | ||
| "name": "data-*", | ||
| "type": "string" | ||
| "id": "components-spinner-features--with-delay" | ||
| } | ||
| ], | ||
| "subcomponents": [] | ||
| } | ||
| } |
There was a problem hiding this comment.
The entire file has been reformatted with fields reordered (e.g., a11yReviewed and importPath moved before their original positions). This appears to be unintentional reformatting unrelated to the delay feature.
Based on other .docs.json files in the codebase (e.g., Button.docs.json, Stack.docs.json), the standard field order is: id, name, status, a11yReviewed, stories, importPath, props, subcomponents.
Consider reverting the file to its original structure and only adding the new delay prop entry and the new story reference, without changing the order of existing fields.
| const timeoutId = setTimeout(() => { | ||
| setIsVisible(true) | ||
| }, 1000) | ||
|
|
||
| return () => clearTimeout(timeoutId) |
There was a problem hiding this comment.
The useEffect dependency array only includes [delay], but if the delay prop changes from false to true or vice versa after the component has mounted, the behavior might be unexpected:
- If
delaychanges fromfalsetotrueafter mount, the spinner will remain visible (sinceisVisibleis alreadytrue) - If
delaychanges fromtruetofalseafter the timeout has started, a new timeout won't be cleared, but the component will start a new timeout
While changing the delay prop after mount is likely an edge case, consider adding a comment documenting that the delay prop should not be changed after mount, or add logic to handle prop changes:
useEffect(() => {
if (delay) {
setIsVisible(false) // Reset visibility when delay prop changes
const timeoutId = setTimeout(() => {
setIsVisible(true)
}, 1000)
return () => clearTimeout(timeoutId)
} else {
setIsVisible(true) // Immediately show when delay is removed
}
}, [delay])| const timeoutId = setTimeout(() => { | |
| setIsVisible(true) | |
| }, 1000) | |
| return () => clearTimeout(timeoutId) | |
| setIsVisible(false) // Reset visibility when delay prop changes | |
| const timeoutId = setTimeout(() => { | |
| setIsVisible(true) | |
| }, 1000) | |
| return () => clearTimeout(timeoutId) | |
| } else { | |
| setIsVisible(true) // Immediately show when delay is removed |
| if (delay) { | ||
| const timeoutId = setTimeout(() => { | ||
| setIsVisible(true) | ||
| }, 1000) |
There was a problem hiding this comment.
[nitpick] The delay duration of 1000 milliseconds is hardcoded in multiple places (here and in the JSDoc comment on line 24). Consider extracting this to a named constant at the top of the file to improve maintainability:
const SPINNER_DELAY_MS = 1000;This makes it easier to adjust the delay in the future and ensures consistency between the implementation and documentation.
Closes https://github.com/github/primer/issues/5907
Adds a
delayprop to the Spinner component that delays rendering by 1000ms. This helps prevent spinner flash for quick loading states, improving the user experience by avoiding jarring visual changes when content loads quickly.Changelog
New
delayprop (boolean, defaults tofalse) to Spinner component that delays rendering by 1000msWithDelaystory to demonstrate the delay functionalityChanged
Removed
N/A
Rollout strategy
This is a new feature that adds an optional prop with a safe default value (
false), making it backwards compatible.Testing & Reviewing
Components/Spinner/Featuresto see the delay in actiondelay={false}(default behavior)delay={true}delay={true}Merge checklist