feat(blade-svelte): Skeleton component#3362
Conversation
Made-with: Cursor
🦋 Changeset detectedLatest commit: 0a408f1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
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 |
|
✅ PR title follows Conventional Commits specification. |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 0a408f1:
|
| opacity: 0; | ||
| background-color: var(--interactive-background-gray-default); | ||
| animation-name: skeleton-fade-in, skeleton-pulse; | ||
| animation-duration: var(--duration-2xgentle), 1320ms; |
There was a problem hiding this comment.
Magic number for animation duration
The 1320ms here is the sum of --duration-xmoderate (360ms) + --duration-2xgentle (960ms), matching the React PulseAnimation logic. However, this is a magic number that will silently drift if those token values ever change.
Consider using calc(var(--duration-xmoderate) + var(--duration-2xgentle)) instead, which stays in sync with the design tokens automatically:
animation-duration: var(--duration-2xgentle), calc(var(--duration-xmoderate) + var(--duration-2xgentle));There was a problem hiding this comment.
Tracking — out of scope for this must-fix pass. Will switch to calc(var(--duration-xmoderate) + var(--duration-2xgentle)) in a follow-up so the duration stays in sync with token changes.
| animation-timing-function: var(--easing-standard), var(--easing-standard); | ||
| animation-iteration-count: 1, infinite; | ||
| animation-direction: normal, alternate; | ||
| animation-fill-mode: forwards; |
There was a problem hiding this comment.
Missing prefers-reduced-motion media query
The Skeleton animation runs infinitely with no respect for users who have reduced motion enabled. Consider adding:
@media (prefers-reduced-motion: reduce) {
.skeleton {
animation: none;
opacity: 1;
background-color: var(--interactive-background-gray-default);
}
}This is an accessibility best practice for any infinite animation. The React version also lacks this, but the Svelte migration is a good opportunity to add it since this CSS lives in blade-core and would benefit both implementations.
There was a problem hiding this comment.
Tracking — out of scope for this must-fix pass. Good a11y suggestion; will add the reduced-motion media query (and likely backport to React Skeleton CSS too) in a follow-up.
| const finalStyle = $derived(combineStyleStrings(ownInlineStyle, styledPropsInlineStyle) || undefined); | ||
|
|
||
| const a11yAttrs = makeAccessible({ hidden: true }); | ||
| const metaAttrs = metaAttribute({ name: MetaConstants.Skeleton, testID }); |
There was a problem hiding this comment.
metaAttrs not reactive to testID changes
metaAttrs is computed at initialization time (no $derived), so if testID changes at runtime, the data-testid attribute will not update. Same issue with a11yAttrs, though that one uses a static { hidden: true } so it's fine in practice.
For consistency with the reactive pattern used throughout the rest of this component, consider wrapping in $derived:
const metaAttrs = $derived(metaAttribute({ name: MetaConstants.Skeleton, testID }));This ensures the component behaves correctly if consumers conditionally toggle testID.
There was a problem hiding this comment.
Acknowledged. Matches the existing pattern across blade-svelte components; will revisit holistically rather than per-component.
|
|
||
| export const skeletonClass = styles.skeleton; | ||
|
|
||
| export type SkeletonFlexProps = { |
There was a problem hiding this comment.
Unused export: skeletonClass
skeletonClass is exported from skeleton.ts and re-exported from the barrel index.ts, but it's never consumed anywhere in this PR (nor anywhere else in the codebase). The getSkeletonClasses() function already returns the base class via the CVA call internally.
If this is intended as a public escape hatch for consumers who need the raw class name, that's fine -- but consider documenting the use case or removing it to keep the public API surface minimal.
There was a problem hiding this comment.
Tracking — out of scope for this must-fix pass. Will remove (or document an external consumer for) the export in a follow-up.
| const styledPropsInlineStyle = $derived.by(() => { | ||
| const entries = Object.entries(styledProps.inlineStyles ?? {}); | ||
| if (entries.length === 0) return undefined; | ||
| return entries.map(([key, value]) => `${kebabCase(key)}: ${value}`).join('; '); |
There was a problem hiding this comment.
Duplicated styledPropsInlineStyle logic -- consider consolidating
This manual kebab-case conversion of styledProps.inlineStyles into a CSS string is likely duplicated across other Svelte Blade components. If combineStyleStrings already exists as a shared util, consider also adding a shared inlineStylesToString() helper in blade-core to avoid reimplementing this pattern in every component.
Not blocking, but worth tracking if this pattern keeps appearing in the Svelte migration.
There was a problem hiding this comment.
Tracking — out of scope for this must-fix pass. Will extract a shared util in blade-core in a follow-up once two more components hit the same pattern.
| return entries.map(([key, value]) => `${kebabCase(key)}: ${value}`).join('; '); | ||
| }); | ||
|
|
||
| const finalStyle = $derived(combineStyleStrings(ownInlineStyle, styledPropsInlineStyle) || undefined); |
There was a problem hiding this comment.
Line length exceeds 100 chars
Nit: this line is quite long. Consider breaking it for readability:
const finalStyle = $derived(
combineStyleStrings(ownInlineStyle, styledPropsInlineStyle) || undefined,
);There was a problem hiding this comment.
Tracking — nit, out of scope for this must-fix pass.
| <Skeleton | ||
| height="65px" | ||
| borderRadius="medium" | ||
| marginBottom="spacing.3" |
There was a problem hiding this comment.
Stories: consider extracting duplicated skeleton card layout
The Complex and CardExample stories share a large amount of duplicated layout markup (the two-card loading state pattern in Complex is copy-pasted between left and right panels). This makes the stories harder to maintain.
Consider extracting the shared skeleton card pattern into a small helper snippet or a SkeletonCard fragment within the stories file to reduce duplication and improve readability. Not blocking -- stories are not production code -- but it would make future story updates easier.
There was a problem hiding this comment.
Tracking — nit, out of scope for this must-fix pass.
rohankokane-dev
left a comment
rohankokane-dev
left a comment
There was a problem hiding this comment.
Skeleton Svelte 5 Migration -- Code Review Summary
Overall: Good migration. The component is well-structured, follows Svelte 5 idioms ($props(), $derived, $derived.by), and correctly mirrors the React Skeleton's behavior. The blade-core extraction of styles is clean and will benefit both React and Svelte consumers.
What looks good
- Svelte 5 patterns: Correct use of
$props()with destructuring + rest,$derivedfor reactive computations, and$derived.byfor the more complex styled props inline style logic. - Architecture: Clean separation between blade-core (styles, CVA, utility mappings) and blade-svelte (component). The
getSkeletonClasses/getSkeletonInlineStylesplit is well thought out. - Accessibility:
aria-hidden="true"on each skeleton node matches the React implementation and the documented decision (consumers should usearia-busyon the container). Stories demonstrate thearia-busypattern correctly. - Type safety: Props interface is well-documented with JSDoc comments and correctly extends
StyledPropsBlade. - Animation fidelity: The CSS keyframes match the React styled-components implementation (fade-in then infinite pulse with the same timing curve and delays).
Issues to address
-
[suggestion]
prefers-reduced-motion: The infinite pulse animation has no reduced-motion fallback. This is an accessibility gap. See inline comment for the recommended CSS. -
[suggestion] Magic number
1320ms: The pulse duration is hardcoded as1320msinstead of usingcalc(var(--duration-xmoderate) + var(--duration-2xgentle)). If token values change, this will silently break. -
[nit]
metaAttrsreactivity:metaAttribute(...)is called once at init time, not wrapped in$derived. IftestIDever changes at runtime, the DOM won't update. Low impact since most consumers set testID statically, but worth fixing for correctness.
Minor / Non-blocking
- Unused export
skeletonClass: Exported but never consumed. Consider removing or documenting the intended use case. - Duplicated
styledPropsInlineStylepattern: The kebab-case + join logic will likely repeat across every Svelte Blade component. Worth extracting into a shared util at some point. - Stories duplication: The Complex story has copy-pasted layout markup between left/right panels.
Checklist items from PR description
The PR notes several unchecked items (Component Status Page, manual testing in other browsers, KitchenSink story). Please ensure those are tracked before merge.
No blocking issues. The component is correct and ready to ship after addressing the suggestions above.
3 participants
Description
Migrates the React Skeleton component to Svelte 5.
Changes
packages/blade-svelte/src/components/Skeleton/packages/blade-core/src/styles/Skeleton/packages/blade-svelte/src/components/index.tspackages/blade-core/src/styles/index.tsAdditional Information
Artifacts (in worktree):
.cursor/artifacts/Skeleton/discovery-report.md.cursor/artifacts/Skeleton/migration-plan.md.cursor/artifacts/Skeleton/verification-report.md.cursor/artifacts/Skeleton/screenshots/See the verification report for the full validation summary.
Component Checklist
Made with Cursor