Skip to content

ECHOES-1335 LoadingSkeleton component#691

Open
jeremy-davis-sonarsource wants to merge 1 commit into
mainfrom
jay/loading-skeleton
Open

ECHOES-1335 LoadingSkeleton component#691
jeremy-davis-sonarsource wants to merge 1 commit into
mainfrom
jay/loading-skeleton

Conversation

@jeremy-davis-sonarsource
Copy link
Copy Markdown
Contributor

@jeremy-davis-sonarsource jeremy-davis-sonarsource commented Jun 3, 2026


Summary by Gitar

  • New Component:
    • Added LoadingSkeleton component to provide a visual loading state with multiple varieties: Text, Rectangle, Disk, and Paragraph.
    • Implemented a shimmer animation effect using @emotion/react keyframes with prefers-reduced-motion support.
    • Added component exports to src/components/index.ts for library consumption.
  • Documentation:
    • Added LoadingSkeleton-stories.tsx demonstrating various usage patterns including text, wrappers, and integration within Card components.

This will update automatically on new commits.

@netlify
Copy link
Copy Markdown

netlify Bot commented Jun 3, 2026

Deploy Preview for echoes-react ready!

Name Link
🔨 Latest commit c324d36
🔍 Latest deploy log https://app.netlify.com/projects/echoes-react/deploys/6a227df277f7850008a0d0f5
😎 Deploy Preview https://deploy-preview-691--echoes-react.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
🤖 Make changes Run an agent on this branch

To edit notification comments on pull requests, go to your Netlify project configuration.

@hashicorp-vault-sonar-prod
Copy link
Copy Markdown

hashicorp-vault-sonar-prod Bot commented Jun 3, 2026

ECHOES-1335

Comment thread src/components/loading-skeleton/index.ts
Comment thread src/components/loading-skeleton/LoadingSkeleton.tsx
Comment thread src/components/loading-skeleton/LoadingSkeleton.tsx Outdated
Comment thread src/components/loading-skeleton/LoadingSkeleton.tsx
Comment thread src/components/loading-skeleton/LoadingSkeleton.tsx Outdated
Comment thread src/components/loading-skeleton/LoadingSkeleton.tsx Outdated
Comment thread src/components/loading-skeleton/LoadingSkeletonStyles.tsx Outdated
Comment thread src/components/loading-skeleton/LoadingSkeleton.tsx
@sonarqube-next
Copy link
Copy Markdown

sonarqube-next Bot commented Jun 5, 2026

@gitar-bot
Copy link
Copy Markdown

gitar-bot Bot commented Jun 5, 2026

Code Review ✅ Approved 8 resolved / 8 findings

Introduces the LoadingSkeleton component with multiple varieties and shimmer animations. Resolved issues regarding unit tests, DOM prop forwarding, and invalid CSS width handling.

✅ 8 resolved
Quality: New LoadingSkeleton component has no unit tests

📄 src/components/loading-skeleton/LoadingSkeleton.tsx:53-67
src/components/loading-skeleton/LoadingSkeleton.tsx is a new public component but ships without a __tests__ directory. The repo convention is that components have unit tests (e.g. src/components/tooltip/__tests__, src/components/buttons/__tests__). Tests should at minimum cover the isLoading=false branch (renders children) and each variety (Text, Paragraph, Disk, Rectangle) rendering the expected skeleton structure. Note this PR is still a draft, so this may be planned.

Quality: width style prop is forwarded to the DOM as an attribute

📄 src/components/loading-skeleton/LoadingSkeleton.tsx:128-132 📄 src/components/loading-skeleton/LoadingSkeleton.tsx:71
StyledLoadingSkeletonText is typed with Pick<CSSProperties, 'width'> and consumes width only inside the styled template (line 129). Because width is a valid HTML attribute, Emotion's default prop filtering forwards it to the underlying <div>, emitting an unwanted width="100px" attribute. Other styled components in this repo avoid this by using a custom prop name destructured in the template (see SpinnerStyles.tsx). Consider renaming the prop (e.g. a transient/custom name like skeletonWidth) so it isn't passed through to the DOM element.

Bug: skeletonWidth number value produces invalid CSS width

📄 src/components/loading-skeleton/LoadingSkeletonStyles.tsx:65-73
StyledLoadingSkeletonText is typed to accept skeletonWidth?: string | number, and the interpolation ${(props) => props.skeletonWidth && \width: ${props.skeletonWidth}`}emits the value verbatim. When a number such as40is passed, this produceswidth: 40(no unit), which is invalid CSS and will be ignored by the browser (only0is valid unit-less). Internally only the stringPARAGRAPH_LAST_WIDTH('40%') is used today, but since the component is exported and the prop type advertisesnumber, a numeric caller will silently get no width. Either drop numberfrom the type, or appendpxwhen a number is provided, e.g.typeof props.skeletonWidth === 'number' ? `${props.skeletonWidth}px` : props.skeletonWidth`.

Quality: Use cssVar() helper instead of raw var() in gradient

📄 src/components/loading-skeleton/LoadingSkeleton.tsx:100-105
The shimmer gradient hardcodes var(--echoes-color-surface-default) (line 103) while every other color/dimension in this file uses the cssVar(...) design-token helper (e.g. cssVar('color-surface-hover'), cssVar('border-radius-200')). Using the raw CSS variable bypasses the type-safe token helper and is inconsistent; if the token name changes there is no compile-time safety. Replace with ${cssVar('color-surface-default')}.

Quality: Accessibility: skeleton lacks aria-hidden / reduced-motion handling

📄 src/components/loading-skeleton/LoadingSkeleton.tsx:48-62 📄 src/components/loading-skeleton/LoadingSkeleton.tsx:76-90
The skeleton renders a perpetually animating shimmer with no accessibility semantics. Unlike the existing Spinner (which uses aria-live and a screen-reader label, see src/components/spinner/Spinner.tsx:63,82), LoadingSkeleton exposes purely decorative shimmer elements to the accessibility tree. Consider marking the skeleton elements aria-hidden (and/or conveying busy state via aria-busy/role="status" on the loading region) so screen readers don't announce empty placeholders. Additionally, the infinite shimmer animation has no prefers-reduced-motion: reduce opt-out, which can be uncomfortable for motion-sensitive users.

...and 3 more resolved from earlier reviews

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change:

Auto-apply Compact
gitar auto-apply:on         
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Gitar

@jeremy-davis-sonarsource jeremy-davis-sonarsource marked this pull request as ready for review June 5, 2026 07:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant