You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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')}.
📄 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Gitar
LoadingSkeletoncomponent to provide a visual loading state with multiple varieties:Text,Rectangle,Disk, andParagraph.@emotion/reactkeyframes withprefers-reduced-motionsupport.src/components/index.tsfor library consumption.LoadingSkeleton-stories.tsxdemonstrating various usage patterns including text, wrappers, and integration withinCardcomponents.This will update automatically on new commits.