-
Notifications
You must be signed in to change notification settings - Fork 0
feat(Spinner): add accessible SVG spinner component #63
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
## Description Implemented a reusable `Spinner` atom component to indicate loading states or ongoing processes within the application. This component renders a lightweight, accessible SVG animation and supports customization for size and color to fit various UI contexts.
📝 WalkthroughWalkthroughA new Spinner component is introduced with type definitions and Storybook stories. The component renders an animated SVG circle with configurable size ("sm", "md", "lg"), color, and className. It dynamically computes SVG dimensions and stroke width, includes accessibility attributes, and demonstrates usage through four story variations. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/components/atoms/spinner/Spinner.tsx`:
- Around line 46-52: The SVG in the Spinner component is missing the
accessibility attributes promised in the docs; update the <svg> element in
Spinner.tsx (inside the Spinner component where width/height are set using
pixelSize and className) to include role="status" and aria-label="Loading" (keep
focusable="false") so the spinner is announced correctly by assistive
technologies.
🧹 Nitpick comments (1)
src/components/atoms/spinner/Spinner.stories.tsx (1)
16-20: Prefer Tailwind utility classes over inline styles/hex colors in stories.
Inline styles and hex values bypass the project’s Tailwind/DaisyUI styling rule; the stories can demonstratecurrentColorvia text color utilities. As per coding guidelines, ...♻️ Suggested refactor
export const Default: Story = { args: { size: "md", - color: "#2563eb", + className: "text-blue-600", }, }; export const Sizes: Story = { render: () => ( - <div style={{display: "flex", gap: 16, alignItems: "center"}}> - <Spinner size="sm" color="#2563eb"/> - <Spinner size="md" color="#2563eb"/> - <Spinner size="lg" color="#2563eb"/> + <div className="flex items-center gap-4 text-blue-600"> + <Spinner size="sm"/> + <Spinner size="md"/> + <Spinner size="lg"/> </div> ), }; export const Colors: Story = { render: () => ( - <div style={{display: "flex", gap: 16, alignItems: "center"}}> - <Spinner color="#ef4444"/> {/* red */} - <Spinner color="#22c55e"/> {/* green */} - <Spinner color="#f59e0b"/> {/* amber */} - <Spinner color="#0ea5e9"/> {/* sky */} + <div className="flex items-center gap-4"> + <Spinner className="text-red-500"/> {/* red */} + <Spinner className="text-green-500"/> {/* green */} + <Spinner className="text-amber-500"/> {/* amber */} + <Spinner className="text-sky-500"/> {/* sky */} </div> ), }; export const InheritColor: Story = { render: () => ( - <div style={{color: "#7c3aed"}}> + <div className="text-violet-600"> <Spinner/> </div> ), };Also applies to: 27-33, 41-46, 55-56
| <svg | ||
| width={pixelSize} | ||
| height={pixelSize} | ||
| viewBox="0 0 50 50" | ||
| className={className} | ||
| focusable="false" | ||
| > |
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.
Add the advertised accessibility attributes to the SVG.
The docs state role="status" and aria-label="Loading", but the SVG omits them, weakening accessibility and contradicting the comment.
🔧 Proposed fix
<svg
width={pixelSize}
height={pixelSize}
viewBox="0 0 50 50"
className={className}
+ role="status"
+ aria-label="Loading"
focusable="false"
>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <svg | |
| width={pixelSize} | |
| height={pixelSize} | |
| viewBox="0 0 50 50" | |
| className={className} | |
| focusable="false" | |
| > | |
| <svg | |
| width={pixelSize} | |
| height={pixelSize} | |
| viewBox="0 0 50 50" | |
| className={className} | |
| role="status" | |
| aria-label="Loading" | |
| focusable="false" | |
| > |
🤖 Prompt for AI Agents
In `@src/components/atoms/spinner/Spinner.tsx` around lines 46 - 52, The SVG in
the Spinner component is missing the accessibility attributes promised in the
docs; update the <svg> element in Spinner.tsx (inside the Spinner component
where width/height are set using pixelSize and className) to include
role="status" and aria-label="Loading" (keep focusable="false") so the spinner
is announced correctly by assistive technologies.
Description
This PR introduces comprehensive TSDoc comments to the
Spinnercomponent. The goal is to improve code readability, maintainability, and provide better IDE support for developers using the component. The documentation now clearly outlines the component's purpose, its props, their types, default values, and provides usage examples.Type of Change
Changes Made
Spinnerfunctional component.props.sizeproperty, including its accepted values ("sm", "md", "lg") and their corresponding pixel dimensions.props.colorproperty, explaining its usage and default value.@component,@returns, and@exampletags for rich documentation generation and IDE hints.Testing Done
Screenshots (if applicable)
N/A (Documentation change, no visual impact on the component's rendering beyond what was already there).
Related Issues
Closes #issue_number (Please replace with actual issue number if applicable)
Checklist
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.