-
Notifications
You must be signed in to change notification settings - Fork 650
feat(SkeletonBox): add customizable delay #7448
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.
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
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 33e64c1 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 |
|
👋 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 |
Change version type for @primer/react from patch to feat.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds a customizable delay feature to the SkeletonBox component, allowing developers to control when skeleton loading states appear. The delay can be set to a boolean for a default 1000ms delay, or a custom number of milliseconds.
Changes:
- Added
delayprop toSkeletonBoxwith support for boolean or numeric values - Implemented delay logic using
useStateanduseEffectwithsetTimeout(following the existing pattern from theSpinnercomponent) - Added comprehensive test coverage for delay behavior with proper timer mocking
- Added documentation and Storybook story for the new feature
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/react/src/Skeleton/SkeletonBox.tsx | Implements delay prop with useState/useEffect pattern, supporting both boolean and numeric delay values |
| packages/react/src/Skeleton/tests/SkeletonBox.test.tsx | Adds test suite for delay behavior covering immediate render, delayed render, timeout cleanup, and timer advancement |
| packages/react/src/Skeleton/SkeletonBox.features.stories.tsx | Adds WithDelay story to demonstrate the delay feature in Storybook |
| packages/react/src/Skeleton/SkeletonBox.docs.json | Documents the new delay prop with type, description, and default value |
| .changeset/sweet-pets-breathe.md | Adds changeset entry for the feature release |
| "props": [ | ||
| { | ||
| "name": "delay", | ||
| "type": "boolean | 'number'", |
Copilot
AI
Jan 14, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The type should be "boolean | number" without quotes around "number". The quotes make it a string literal type instead of the number type.
| "type": "boolean | 'number'", | |
| "type": "boolean | number", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because this is a new prop, should we try 'short' | 'long' | number? instead of boolean?
Bonus points if we can align it with Spinner delay
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any thoughts on what short and long would translate to? 👀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at motion primitives: https://primer.style/primitives/storybook/?path=/story/motion-base--base
long is what we have now, 1000ms. I imagine short could one of 200, 300, 400 or 500ms. Not sure if @lukasoppermann already has a preference. I personally would pick 300 and run with it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
implemented!
|
@francinelucca I've opened a new pull request, #7450, to work on those changes. Once the pull request is ready, I'll request review from you. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved because it matches Spinner.
Left a comment about my ideal API, but approving in advance in case you choose not to take the suggestion
Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: francinelucca <[email protected]>
|
👋 Hi from github/github-ui! Your integration PR is ready: https://github.com/github/github-ui/pull/10805 |
|
Integration test results from github/github-ui:
CI check runs linting, type checking, and unit tests. Check the workflow logs for specific failures. VRT check ensures that when visual differences are detected, the PR cannot proceed until someone acknowledges the changes by adding the "visual difference acknowledged" label. Need help? If you believe this failure is unrelated to your changes, please reach out to the Primer team for assistance. |
Closes https://github.com/github/primer/issues/6292
Changelog
New
delayprop inSkeletonBox, along with docs, story and testsRollout strategy
Testing & Reviewing
Merge checklist