Add Storybook to store components #1012
Conversation
…nother package since it breaks Storybook
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
📝 WalkthroughWalkthroughThis pull request introduces Storybook to the UI package, creating a comprehensive set of stories for various components. The changes include configuring Storybook in the UI workspace, adding story files for multiple components like Button, Input, Dialog, and others, and updating import statements to centralize the SpinnerIcon. The implementation provides a robust documentation and testing environment for the UI components. Changes
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (31)
ui/src/logos/spinner-icon.tsx (1)
14-14: Consider making animation duration configurable.The animation speed is hardcoded via Tailwind's
animate-spin. Consider making it configurable through props for different use cases.-className={twMerge('h-5 w-5 animate-spin', className)} +className={twMerge('h-5 w-5', animationSpeed === 'fast' ? 'animate-spin-fast' : 'animate-spin', className)}ui/src/stories/alert.stories.tsx (1)
14-86: Consider enhancing stories with additional features.The stories provide good coverage of component variants. Consider these enhancements:
- Add args configuration for interactive controls
- Include accessibility examples (ARIA labels, keyboard navigation)
- Add responsive design examples
- Demonstrate error handling scenarios
Example args configuration:
export const Primary: Story = { args: { variant: 'primary', title: 'Primary Alert', description: 'This is a primary alert with an icon.', showIcon: true, }, render: (args) => ( <Alert variant={args.variant}> {args.showIcon && <InformationCircleIcon className="h-4 w-4" />} <AlertTitle>{args.title}</AlertTitle> <AlertDescription>{args.description}</AlertDescription> </Alert> ), };ui/.storybook/preview.js (1)
4-13: Consider enhancing preview configuration with additional parameters.Add viewport and theme configurations to improve story development experience.
const preview = { parameters: { + viewport: { + viewports: { + mobile: { name: 'Mobile', styles: { width: '360px', height: '640px' } }, + tablet: { name: 'Tablet', styles: { width: '768px', height: '1024px' } }, + desktop: { name: 'Desktop', styles: { width: '1280px', height: '800px' } }, + }, + }, + themes: { + default: 'light', + list: [ + { name: 'light', class: 'light-theme', color: '#ffffff' }, + { name: 'dark', class: 'dark-theme', color: '#000000' }, + ], + }, controls: {ui/.storybook/main.js (1)
14-18: Add useful Storybook addons for enhanced development.Consider adding a11y and viewport addons for better component testing.
addons: [ getAbsolutePath('@storybook/addon-onboarding'), getAbsolutePath('@storybook/addon-essentials'), getAbsolutePath('@storybook/addon-interactions'), + getAbsolutePath('@storybook/addon-a11y'), + getAbsolutePath('@storybook/addon-viewport'), ],ui/src/stories/avatar.stories.tsx (1)
4-8: Add component description and usage guidelines.Enhance documentation with component description and usage examples.
const meta = { title: 'Components/Avatar', component: Avatar, tags: ['autodocs'], + parameters: { + docs: { + description: { + component: 'Avatar component for displaying user profile images with fallback support.', + }, + }, + }, } satisfies Meta<typeof Avatar>;ui/src/stories/accordion.stories.tsx (1)
20-20: Make accordion width responsive.Replace hardcoded width with responsive classes.
- className: 'w-[400px]', + className: 'w-full max-w-md', - <Accordion type="single" collapsible className="w-[400px]"> + <Accordion type="single" collapsible className="w-full max-w-md"> - className: 'w-[400px]', + className: 'w-full max-w-md', - <Accordion type="multiple" className="w-[400px]"> + <Accordion type="multiple" className="w-full max-w-md">Also applies to: 23-23, 47-47, 50-50
ui/src/stories/badge.stories.tsx (1)
64-74: Consider using CSS variables for colors.Using direct Tailwind color classes like
bg-blue-500makes theme customization harder. Consider using CSS variables for better maintainability.-<Badge className="bg-blue-500 hover:bg-blue-600">Custom Blue</Badge> +<Badge className="bg-[var(--badge-color)] hover:bg-[var(--badge-hover-color)]">Custom Blue</Badge>ui/src/stories/progress.stories.tsx (1)
46-67: Consider making animation speed configurable.The animation interval is hardcoded to 1000ms. Consider making it configurable through story controls.
+export const Animated: Story = { + args: { + interval: 1000, + }, render: ({ interval }) => { const [progress, setProgress] = React.useState(0); React.useEffect(() => { - const timer = setInterval(() => { + const timer = setInterval(() => { setProgress((prevProgress) => (prevProgress >= 100 ? 0 : prevProgress + 10)); - }, 1000); + }, interval);ui/src/stories/checkbox.stories.tsx (1)
91-102: Simplify the checkbox state handling.The Boolean type casting and callback can be simplified.
-<Checkbox checked={checked} onCheckedChange={(checkedState) => setChecked(Boolean(checkedState))} /> +<Checkbox checked={checked} onCheckedChange={setChecked} />ui/src/stories/switch.stories.tsx (2)
54-57: Add form submission handler implementation.The form submission handler is currently empty. Consider implementing the handler or adding a comment explaining that it's intentionally left empty for demonstration purposes.
onSubmit={(e) => { e.preventDefault(); - // Handle form submission + // This is a demo form submission handler + console.log('Form submitted'); }}
63-69: Add aria-label to notification settings section.The notification settings section should be labeled for better accessibility.
-<div className="space-y-2"> +<div className="space-y-2" role="group" aria-label="Notification Settings">ui/src/stories/popover.stories.tsx (2)
48-58: Add input validation and error handling to form fields.The form fields lack validation and error handling. Consider adding required fields and validation messages.
<input id="email" defaultValue="john@example.com" + required + pattern="[a-z0-9._%+-]+@[a-z0-9.-]+\.[a-z]{2,}$" className="border-input col-span-2 h-8 rounded-md border px-3" + aria-describedby="email-error" /> +<span id="email-error" className="text-destructive text-sm" />
69-70: Consider adding loading states for delayed popovers.The custom delay implementation could benefit from a loading indicator during the delay period.
ui/src/stories/dialog.stories.tsx (1)
72-90: Add form validation and aria-required attributes.The profile form fields should include proper validation attributes and aria labels.
<input id="name" + required + aria-required="true" className="col-span-3 h-10 rounded-md border px-3" placeholder="Enter your name" />ui/src/stories/carousel.stories.tsx (1)
108-116: Add pause on hover for autoplay carousel.Consider adding a pause feature when users hover over the carousel for better user experience.
options={{ autoPlay: { enabled: true, delay: 3000, + pauseOnHover: true }, }}ui/src/stories/radio-group.stories.tsx (1)
119-132: Use design system tokens for custom styling.Instead of using direct color classes like
border-blue-500, consider using design system tokens for consistent theming.-<RadioGroupItem value="option-1" id="custom-1" className="border-blue-500 text-blue-500" /> +<RadioGroupItem value="option-1" id="custom-1" className="border-primary text-primary" /> -<RadioGroupItem value="option-2" id="custom-2" className="border-green-500 text-green-500" /> +<RadioGroupItem value="option-2" id="custom-2" className="border-secondary text-secondary" />ui/src/stories/toggle.stories.tsx (1)
17-31: Extract SVG icons into separate components.The SVG icons are duplicated across multiple stories. Consider extracting them into reusable components.
Create an icons directory and move the SVGs there:
// src/components/icons/italic-icon.tsx export const ItalicIcon = () => ( <svg xmlns="http://www.w3.org/2000/svg" width="16" height="16" viewBox="0 0 24 24" fill="none" stroke="currentColor" strokeWidth="2" strokeLinecap="round" strokeLinejoin="round" > <line x1="19" y1="4" x2="10" y2="4" /> <line x1="14" y1="20" x2="5" y2="20" /> <line x1="15" y1="4" x2="9" y2="20" /> </svg> );Also applies to: 40-54, 110-123, 126-140, 143-156
ui/src/stories/typography.stories.tsx (1)
6-23: Import color tokens from design system.Instead of maintaining a separate FONT_COLORS array, consider importing these values from your design system tokens.
import { colorTokens } from '../theme/tokens'; const FONT_COLORS: FontColor[] = Object.keys(colorTokens) as FontColor[];ui/src/stories/tooltip.stories.tsx (2)
22-139: Add ARIA labels for better accessibility.Consider adding aria-label attributes to buttons in the stories, especially for icon-only buttons.
-<button className="flex h-8 w-8 items-center justify-center rounded-full border"> +<button + className="flex h-8 w-8 items-center justify-center rounded-full border" + aria-label="Information">
140-187: Document delay duration constraints.Consider adding a comment about minimum/maximum recommended delay durations for optimal user experience.
ui/src/stories/collapsible.stories.tsx (2)
44-50: Optimize state management for multiple sections.Consider using a Set instead of an array for better performance with includes/filter operations.
-const [openSections, setOpenSections] = React.useState<number[]>([]); +const [openSections, setOpenSections] = React.useState<Set<number>>(new Set()); const toggleSection = (index: number) => { - setOpenSections((current) => - current.includes(index) ? current.filter((i) => i !== index) : [...current, index], - ); + setOpenSections((current) => { + const newSet = new Set(current); + current.has(index) ? newSet.delete(index) : newSet.add(index); + return newSet; + }); };
111-138: Make animation duration configurable.Consider making the animation duration customizable through props for more flexibility.
ui/src/stories/menubar.stories.tsx (1)
1-171: Document keyboard navigation patterns.Add comments or stories demonstrating keyboard navigation patterns (e.g., arrow keys, Enter, Escape) for better accessibility documentation.
ui/src/stories/hover-card.stories.tsx (1)
1-161: Add hover delay configuration.Consider adding a delay prop to prevent accidental triggering, especially for interactive content.
-<HoverCard> +<HoverCard openDelay={200} closeDelay={150}>ui/src/stories/tabs.stories.tsx (2)
16-24: Add aria-label to Basic tabs example.Enhance accessibility by adding descriptive aria-labels to the tabs component.
-<Tabs defaultValue="account"> +<Tabs defaultValue="account" aria-label="Account settings">
85-97: Extract SVG icons and enhance accessibility.Consider extracting SVG icons into separate components and adding proper accessibility attributes.
-<svg xmlns="http://www.w3.org/2000/svg" width="16" height="16" viewBox="0 0 24 24" fill="none" stroke="currentColor" strokeWidth="2" strokeLinecap="round" strokeLinejoin="round"> +<svg aria-hidden="true" xmlns="http://www.w3.org/2000/svg" width="16" height="16" viewBox="0 0 24 24" fill="none" stroke="currentColor" strokeWidth="2" strokeLinecap="round" strokeLinejoin="round">Also applies to: 101-114
ui/src/stories/dropdown-menu.stories.tsx (1)
58-69: Add cross-platform keyboard shortcuts.Consider showing different shortcuts for Windows/Linux users.
-New Tab <DropdownMenuShortcut>⌘T</DropdownMenuShortcut> +New Tab <DropdownMenuShortcut>{isMac ? '⌘T' : 'Ctrl+T'}</DropdownMenuShortcut>ui/src/stories/form.stories.tsx (1)
161-215: Add error boundary to form examples.Wrap form examples with error boundaries to handle runtime errors gracefully.
+import { ErrorBoundary } from 'react-error-boundary'; export const DifferentStates: Story = { render: () => ( + <ErrorBoundary fallback={<div>Something went wrong</div>}> <div className="space-y-8"> // ... existing code ... </div> + </ErrorBoundary> ), };ui/src/stories/navigation-menu.stories.tsx (3)
23-44: Add accessibility improvements to ListItem componentConsider enhancing the component with aria-label and prop types validation.
const ListItem = React.forwardRef<React.ElementRef<'a'>, React.ComponentPropsWithoutRef<'a'>>( ({ className, title, children, ...props }, ref) => { return ( <li> <NavigationMenuLink asChild> <a ref={ref} + aria-label={title} className={cn( 'hover:bg-accent hover:text-accent-foreground focus:bg-accent focus:text-accent-foreground block select-none space-y-1 rounded-md p-3 leading-none no-underline outline-none transition-colors', className, )} {...props} >
46-70: Add aria-label to navigation menuEnhance accessibility by adding an aria-label to the navigation menu.
- <NavigationMenu> + <NavigationMenu aria-label="Main navigation">
160-180: Add documentation for custom styling approachConsider adding comments to explain the custom styling methodology for better maintainability.
// Custom Styled +// This story demonstrates how to override default styles using Tailwind classes +// primary and primary-foreground colors are defined in the theme configuration export const CustomStyled: Story = {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (35)
package.json(1 hunks)ui/.gitignore(1 hunks)ui/.storybook/main.js(1 hunks)ui/.storybook/preview.js(1 hunks)ui/package.json(2 hunks)ui/src/components/button.tsx(1 hunks)ui/src/logos/spinner-icon.tsx(1 hunks)ui/src/stories/accordion.stories.tsx(1 hunks)ui/src/stories/alert.stories.tsx(1 hunks)ui/src/stories/avatar.stories.tsx(1 hunks)ui/src/stories/badge.stories.tsx(1 hunks)ui/src/stories/button.stories.tsx(1 hunks)ui/src/stories/card.stories.tsx(1 hunks)ui/src/stories/carousel.stories.tsx(1 hunks)ui/src/stories/checkbox.stories.tsx(1 hunks)ui/src/stories/collapsible.stories.tsx(1 hunks)ui/src/stories/dialog.stories.tsx(1 hunks)ui/src/stories/dropdown-menu.stories.tsx(1 hunks)ui/src/stories/form.stories.tsx(1 hunks)ui/src/stories/hover-card.stories.tsx(1 hunks)ui/src/stories/input.stories.tsx(1 hunks)ui/src/stories/label.stories.tsx(1 hunks)ui/src/stories/menubar.stories.tsx(1 hunks)ui/src/stories/navigation-menu.stories.tsx(1 hunks)ui/src/stories/popover.stories.tsx(1 hunks)ui/src/stories/progress.stories.tsx(1 hunks)ui/src/stories/radio-group.stories.tsx(1 hunks)ui/src/stories/select.stories.tsx(1 hunks)ui/src/stories/separator.stories.tsx(1 hunks)ui/src/stories/switch.stories.tsx(1 hunks)ui/src/stories/table.stories.tsx(1 hunks)ui/src/stories/tabs.stories.tsx(1 hunks)ui/src/stories/toggle.stories.tsx(1 hunks)ui/src/stories/tooltip.stories.tsx(1 hunks)ui/src/stories/typography.stories.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- ui/.gitignore
- ui/src/components/button.tsx
🔇 Additional comments (14)
ui/src/logos/spinner-icon.tsx (3)
1-2: LGTM! Imports are clean and necessary.
4-4: LGTM! Component signature is well-typed.
17-22: Verify licensing requirements for copied SVG.Since this SVG was copied from another site, please verify and document any licensing or attribution requirements.
ui/src/stories/alert.stories.tsx (2)
1-13: LGTM! Well-structured Storybook configuration.The meta configuration and imports are properly set up following Storybook best practices.
3-3: Verify Alert component exports.Let's ensure the Alert component and its subcomponents are properly exported.
✅ Verification successful
Alert component imports are correct
The Alert components are properly exported from
ui/src/components/alert.tsxand match the imports in the stories file.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if Alert components are properly exported ast-grep --pattern 'export { Alert, AlertDescription, AlertTitle }' ui/src/components/alert/index.tsLength of output: 182
Script:
#!/bin/bash # Find alert-related files fd alert.ts -t f fd alert/index.ts -t fLength of output: 71
Script:
#!/bin/bash # Check exports in alert.tsx ast-grep --pattern 'export function Alert' ui/src/components/alert.tsx ast-grep --pattern 'export function AlertDescription' ui/src/components/alert.tsx ast-grep --pattern 'export function AlertTitle' ui/src/components/alert.tsxLength of output: 234
Script:
#!/bin/bash # View the content of alert.tsx to understand export pattern cat ui/src/components/alert.tsxLength of output: 1709
ui/src/stories/badge.stories.tsx (1)
1-12: LGTM! Proper Storybook configuration.The metadata setup and type definitions follow best practices.
ui/src/stories/label.stories.tsx (1)
1-116: Well-structured stories with good accessibility practices!The Label stories effectively demonstrate various use cases while maintaining proper accessibility patterns.
ui/src/stories/radio-group.stories.tsx (1)
85-88: Implement form submission handler.The form submission handler is currently empty.
Do you want me to generate an implementation for the form submission handler?
ui/src/stories/select.stories.tsx (1)
89-92: Implement form submission handler.The form submission handler is currently empty.
Do you want me to generate an implementation for the form submission handler?
ui/src/stories/tooltip.stories.tsx (1)
1-21: LGTM! Well-structured imports and meta configuration.ui/src/stories/navigation-menu.stories.tsx (2)
1-20: Well-structured Storybook configuration!The meta configuration and imports are properly set up, with autodocs enabled for automatic documentation generation.
72-158: Well-implemented navigation menu variations!The MultipleSections and WithActiveStates stories effectively demonstrate different use cases and states.
ui/package.json (1)
46-52: Verify Storybook version compatibilityThe Storybook version 8.4.7 is quite recent. Please verify compatibility with the current React version (^18.2.0).
Also applies to: 64-64, 69-70
✅ Verification successful
Storybook 8.4.7 is compatible with React ^18.2.0
The peer dependencies explicitly support React versions ^18.0.0, which includes React ^18.2.0.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any compatibility issues between Storybook and React versions npm info @storybook/react@8.4.7 peerDependenciesLength of output: 254
package.json (1)
46-47: Well-structured workspace scripts!The ui:serve and ui:build scripts are properly configured using workspace references.
| export const WithImage: Story = { | ||
| render: () => ( | ||
| <Avatar> | ||
| <AvatarImage src="https://github.com/shadcn.png" alt="@shadcn" /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace external image URL with local asset.
Using external URLs can make stories unreliable if the resource becomes unavailable.
- <AvatarImage src="https://github.com/shadcn.png" alt="@shadcn" />
+ <AvatarImage src="/assets/avatar-placeholder.png" alt="Avatar" />Committable suggestion skipped: line range outside the PR's diff.
| // In Card Layout | ||
| export const InCardLayout: Story = { | ||
| render: () => ( | ||
| <div className="space-y-4 rounded-lg border p-4"> | ||
| <div className="flex items-center justify-between"> | ||
| <h3 className="font-semibold">Account Settings</h3> | ||
| <button className="text-sm text-blue-500">Edit</button> | ||
| </div> | ||
| <Separator /> | ||
| <div className="space-y-4"> | ||
| <div> | ||
| <h4 className="text-sm font-medium">Email</h4> | ||
| <p className="text-muted-foreground text-sm">john@example.com</p> | ||
| </div> | ||
| <Separator /> | ||
| <div> | ||
| <h4 className="text-sm font-medium">Password</h4> | ||
| <p className="text-muted-foreground text-sm">Last changed 3 months ago</p> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| ), | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add aria-label to the edit button.
The edit button needs an aria-label for better accessibility.
-<button className="text-sm text-blue-500">Edit</button>
+<button className="text-sm text-blue-500" aria-label="Edit account settings">Edit</button>📝 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.
| // In Card Layout | |
| export const InCardLayout: Story = { | |
| render: () => ( | |
| <div className="space-y-4 rounded-lg border p-4"> | |
| <div className="flex items-center justify-between"> | |
| <h3 className="font-semibold">Account Settings</h3> | |
| <button className="text-sm text-blue-500">Edit</button> | |
| </div> | |
| <Separator /> | |
| <div className="space-y-4"> | |
| <div> | |
| <h4 className="text-sm font-medium">Email</h4> | |
| <p className="text-muted-foreground text-sm">john@example.com</p> | |
| </div> | |
| <Separator /> | |
| <div> | |
| <h4 className="text-sm font-medium">Password</h4> | |
| <p className="text-muted-foreground text-sm">Last changed 3 months ago</p> | |
| </div> | |
| </div> | |
| </div> | |
| ), | |
| }; | |
| // In Card Layout | |
| export const InCardLayout: Story = { | |
| render: () => ( | |
| <div className="space-y-4 rounded-lg border p-4"> | |
| <div className="flex items-center justify-between"> | |
| <h3 className="font-semibold">Account Settings</h3> | |
| <button className="text-sm text-blue-500" aria-label="Edit account settings">Edit</button> | |
| </div> | |
| <Separator /> | |
| <div className="space-y-4"> | |
| <div> | |
| <h4 className="text-sm font-medium">Email</h4> | |
| <p className="text-muted-foreground text-sm">john@example.com</p> | |
| </div> | |
| <Separator /> | |
| <div> | |
| <h4 className="text-sm font-medium">Password</h4> | |
| <p className="text-muted-foreground text-sm">Last changed 3 months ago</p> | |
| </div> | |
| </div> | |
| </div> | |
| ), | |
| }; |
| <Button size="icon" variant="default"> | ||
| <PlusIcon className="h-4 w-4" /> | ||
| </Button> | ||
| <Button size="icon" variant="secondary"> | ||
| <TrashIcon className="h-4 w-4" /> | ||
| </Button> | ||
| <Button size="icon" variant="outline"> | ||
| <ArrowRightIcon className="h-4 w-4" /> | ||
| </Button> | ||
| <Button size="icon" variant="ghost"> | ||
| <ChevronRightIcon className="h-4 w-4" /> | ||
| </Button> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add aria-labels to icon-only buttons for accessibility.
Icon-only buttons need aria-labels to be accessible to screen readers.
<Button size="icon" variant="default">
+ <span className="sr-only">Add item</span>
<PlusIcon className="h-4 w-4" />
</Button>
<Button size="icon" variant="secondary">
+ <span className="sr-only">Delete item</span>
<TrashIcon className="h-4 w-4" />
</Button>
<Button size="icon" variant="outline">
+ <span className="sr-only">Next item</span>
<ArrowRightIcon className="h-4 w-4" />
</Button>
<Button size="icon" variant="ghost">
+ <span className="sr-only">Show more</span>
<ChevronRightIcon className="h-4 w-4" />
</Button>📝 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.
| <Button size="icon" variant="default"> | |
| <PlusIcon className="h-4 w-4" /> | |
| </Button> | |
| <Button size="icon" variant="secondary"> | |
| <TrashIcon className="h-4 w-4" /> | |
| </Button> | |
| <Button size="icon" variant="outline"> | |
| <ArrowRightIcon className="h-4 w-4" /> | |
| </Button> | |
| <Button size="icon" variant="ghost"> | |
| <ChevronRightIcon className="h-4 w-4" /> | |
| </Button> | |
| <Button size="icon" variant="default"> | |
| <span className="sr-only">Add item</span> | |
| <PlusIcon className="h-4 w-4" /> | |
| </Button> | |
| <Button size="icon" variant="secondary"> | |
| <span className="sr-only">Delete item</span> | |
| <TrashIcon className="h-4 w-4" /> | |
| </Button> | |
| <Button size="icon" variant="outline"> | |
| <span className="sr-only">Next item</span> | |
| <ArrowRightIcon className="h-4 w-4" /> | |
| </Button> | |
| <Button size="icon" variant="ghost"> | |
| <span className="sr-only">Show more</span> | |
| <ChevronRightIcon className="h-4 w-4" /> | |
| </Button> |
| <label htmlFor="email" className="text-sm font-medium"> | ||
| </label> | ||
| <Input id="email" type="email" placeholder="Enter email" className="border-destructive" /> | ||
| <p className="text-destructive text-sm">Please enter a valid email address</p> | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error state accessibility.
Add ARIA attributes to properly communicate the error state to screen readers.
<label htmlFor="email" className="text-sm font-medium">
Email
</label>
-<Input id="email" type="email" placeholder="Enter email" className="border-destructive" />
-<p className="text-destructive text-sm">Please enter a valid email address</p>
+<Input
+ id="email"
+ type="email"
+ placeholder="Enter email"
+ className="border-destructive"
+ aria-invalid="true"
+ aria-describedby="email-error"
+/>
+<p id="email-error" className="text-destructive text-sm">Please enter a valid email address</p>📝 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.
| <label htmlFor="email" className="text-sm font-medium"> | |
| </label> | |
| <Input id="email" type="email" placeholder="Enter email" className="border-destructive" /> | |
| <p className="text-destructive text-sm">Please enter a valid email address</p> | |
| </div> | |
| <label htmlFor="email" className="text-sm font-medium"> | |
| </label> | |
| <Input | |
| id="email" | |
| type="email" | |
| placeholder="Enter email" | |
| className="border-destructive" | |
| aria-invalid="true" | |
| aria-describedby="email-error" | |
| /> | |
| <p id="email-error" className="text-destructive text-sm">Please enter a valid email address</p> | |
| </div> |
| export const Interactive: Story = { | ||
| render: () => ( | ||
| <Card className="w-[350px] cursor-pointer transition-shadow hover:shadow-lg"> | ||
| <CardHeader> | ||
| <CardTitle>Interactive Card</CardTitle> | ||
| <CardDescription>This card has hover and click effects</CardDescription> | ||
| </CardHeader> | ||
| <CardContent> | ||
| <p>Click or hover over this card to see the effects.</p> | ||
| </CardContent> | ||
| </Card> | ||
| ), | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add keyboard interaction support to Interactive card.
The card has hover effects but should also support keyboard interactions for accessibility.
-<Card className="w-[350px] cursor-pointer transition-shadow hover:shadow-lg">
+<Card
+ className="w-[350px] cursor-pointer transition-shadow hover:shadow-lg focus-visible:shadow-lg focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-ring"
+ tabIndex={0}
+ role="button"
+ onKeyDown={(e) => {
+ if (e.key === 'Enter' || e.key === ' ') {
+ // Handle interaction
+ e.preventDefault();
+ }
+ }}
+>📝 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.
| export const Interactive: Story = { | |
| render: () => ( | |
| <Card className="w-[350px] cursor-pointer transition-shadow hover:shadow-lg"> | |
| <CardHeader> | |
| <CardTitle>Interactive Card</CardTitle> | |
| <CardDescription>This card has hover and click effects</CardDescription> | |
| </CardHeader> | |
| <CardContent> | |
| <p>Click or hover over this card to see the effects.</p> | |
| </CardContent> | |
| </Card> | |
| ), | |
| }; | |
| export const Interactive: Story = { | |
| render: () => ( | |
| <Card | |
| className="w-[350px] cursor-pointer transition-shadow hover:shadow-lg focus-visible:shadow-lg focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-ring" | |
| tabIndex={0} | |
| role="button" | |
| onKeyDown={(e) => { | |
| if (e.key === 'Enter' || e.key === ' ') { | |
| // Handle interaction | |
| e.preventDefault(); | |
| } | |
| }} | |
| > | |
| <CardHeader> | |
| <CardTitle>Interactive Card</CardTitle> | |
| <CardDescription>This card has hover and click effects</CardDescription> | |
| </CardHeader> | |
| <CardContent> | |
| <p>Click or hover over this card to see the effects.</p> | |
| </CardContent> | |
| </Card> | |
| ), | |
| }; |
| <DialogFooter className="gap-2"> | ||
| <Button variant="outline">Cancel</Button> | ||
| <Button variant="destructive">Yes, delete my account</Button> | ||
| </DialogFooter> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add confirmation step for destructive actions.
The delete account action should require a confirmation step, such as typing "delete" to confirm.
| const DemoSlide = ({ children }: { children: React.ReactNode }) => ( | ||
| <Card className="m-1"> | ||
| <CardContent className="flex aspect-square items-center justify-center p-6"> | ||
| <span className="text-4xl font-semibold">{children}</span> | ||
| </CardContent> | ||
| </Card> | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add keyboard navigation support to DemoSlide component.
The DemoSlide component should support keyboard navigation for better accessibility.
const DemoSlide = ({ children }: { children: React.ReactNode }) => (
<Card className="m-1">
<CardContent className="flex aspect-square items-center justify-center p-6">
- <span className="text-4xl font-semibold">{children}</span>
+ <span className="text-4xl font-semibold" tabIndex={0} role="region">
+ {children}
+ </span>
</CardContent>
</Card>
);📝 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.
| const DemoSlide = ({ children }: { children: React.ReactNode }) => ( | |
| <Card className="m-1"> | |
| <CardContent className="flex aspect-square items-center justify-center p-6"> | |
| <span className="text-4xl font-semibold">{children}</span> | |
| </CardContent> | |
| </Card> | |
| ); | |
| const DemoSlide = ({ children }: { children: React.ReactNode }) => ( | |
| <Card className="m-1"> | |
| <CardContent className="flex aspect-square items-center justify-center p-6"> | |
| <span className="text-4xl font-semibold" tabIndex={0} role="region"> | |
| {children} | |
| </span> | |
| </CardContent> | |
| </Card> | |
| ); |
| <span className="inline-flex items-center rounded-full bg-green-100 px-2.5 py-0.5 text-xs font-medium text-green-800"> | ||
| Completed | ||
| </span> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance status indicators for accessibility.
Add aria-labels and icons to status indicators for better accessibility.
-<span className="inline-flex items-center rounded-full bg-green-100 px-2.5 py-0.5 text-xs font-medium text-green-800">
+<span role="status" aria-label="Order completed" className="inline-flex items-center rounded-full bg-green-100 px-2.5 py-0.5 text-xs font-medium text-green-800">
Completed
</span>Also applies to: 169-171, 179-181
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
ui/src/components/button.tsx(1 hunks)ui/src/index.ts(1 hunks)website/src/app/[lang]/[region]/(website)/me/contributions/contributions-table.tsx(1 hunks)website/src/app/[lang]/[region]/(website)/me/donation-certificates/donation-certificates-table.tsx(1 hunks)website/src/app/[lang]/[region]/(website)/me/subscriptions/subscriptions-client.tsx(1 hunks)website/src/app/[lang]/[region]/(website)/me/work-info/employers-list.tsx(1 hunks)website/src/app/[lang]/[region]/(website)/newsletter/subscription-info-form.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (5)
- website/src/app/[lang]/[region]/(website)/newsletter/subscription-info-form.tsx
- website/src/app/[lang]/[region]/(website)/me/donation-certificates/donation-certificates-table.tsx
- website/src/app/[lang]/[region]/(website)/me/contributions/contributions-table.tsx
- website/src/app/[lang]/[region]/(website)/me/work-info/employers-list.tsx
- website/src/app/[lang]/[region]/(website)/me/subscriptions/subscriptions-client.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- ui/src/components/button.tsx
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Test website
- GitHub Check: Prettify
- GitHub Check: Security checks (typescript)
🔇 Additional comments (1)
ui/src/index.ts (1)
27-27: LGTM! Good organization.The empty line helps separate component exports from icon exports.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
ui/src/icons/donate.tsx (1)
Line range hint
17-18: Consider removing commented color values.The commented-out
fill="#FAC800"values are redundant since the icon usescurrentColor.- // fill="#FAC800"Also applies to: 21-22
ui/package.json (1)
70-71: Consider adding documentation for Storybook scripts.Add a README section explaining how to use these scripts, especially since PR objectives mention using
npm run ui:serveinstead ofnpm run storybook.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
ui/package.json(2 hunks)ui/src/icons/donate.tsx(1 hunks)ui/src/index.ts(1 hunks)website/src/components/navbar/navbar-client.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- website/src/components/navbar/navbar-client.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- ui/src/index.ts
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Test website
- GitHub Check: Prettify
- GitHub Check: Security checks (typescript)
🔇 Additional comments (3)
ui/src/icons/donate.tsx (1)
13-13: LGTM! Props spreading enhances component flexibility.The addition of props spreading allows the component to be more versatile by supporting event handlers and accessibility attributes.
ui/package.json (2)
43-43: Verify the tailwind-merge version upgrade.The upgrade from v2.4.0 to v2.5.3 needs validation for breaking changes.
✅ Verification successful
The tailwind-merge upgrade is safe to proceed
The changes from v2.4.0 to v2.5.3 contain only performance improvements and bug fixes, with no breaking changes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for breaking changes in tailwind-merge changelog curl -s https://raw.githubusercontent.com/dcastil/tailwind-merge/v2.5.3/CHANGELOG.md | grep -A 5 "## \[2.5"Length of output: 109
Script:
#!/bin/bash # Try GitHub API for release info gh api repos/dcastil/tailwind-merge/releases | jq -r '.[] | select(.tag_name | startswith("v2.5")) | {tag_name, body}' # Fetch NPM package info as backup npm view tailwind-merge@2.5.3 repository.url changelogLength of output: 3477
47-53: Verify Storybook version compatibility.The Storybook v8.4.7 dependencies are in beta/preview. This version might be unstable.
Also applies to: 65-65
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
.github/workflows/deploy-storybook.yml (3)
6-6: Remove unnecessary comment about default branch.The comment
# Or your default branch nameis redundant sincemainis explicitly specified.- - main # Or your default branch name + - main
57-57: Add newline at end of file.Add a newline character at the end of the file to comply with POSIX standards.
uses: actions/deploy-pages@v4 +🧰 Tools
🪛 yamllint (1.35.1)
[error] 57-57: no new line character at the end of file
(new-line-at-end-of-file)
23-25: Document the expected URL pattern.Consider adding a comment explaining the expected format of the Storybook URL (e.g.,
https://{org}.github.io/{repo}/).environment: name: github-pages + # URL format: https://{org}.github.io/{repo}/ url: ${{ steps.deployment.outputs.page_url }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/deploy-storybook.yml(1 hunks)
🧰 Additional context used
🪛 yamllint (1.35.1)
.github/workflows/deploy-storybook.yml
[error] 57-57: no new line character at the end of file
(new-line-at-end-of-file)
🪛 actionlint (1.7.4)
.github/workflows/deploy-storybook.yml
33-33: shellcheck reported issue in this script: SC2086:info:1:31: Double quote to prevent globbing and word splitting
(shellcheck)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Test website
- GitHub Check: Prettify
- GitHub Check: Security checks (typescript)
🔇 Additional comments (1)
.github/workflows/deploy-storybook.yml (1)
11-19: Well-configured permissions and concurrency settings!The configuration follows security best practices by using minimal permissions and preventing deployment conflicts.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.github/workflows/deploy-storybook.yml (1)
31-34:⚠️ Potential issueFix shell script quoting in .nvmrc reading step.
The command needs proper quoting to prevent potential issues.
- run: echo "NVMRC=$(cat .nvmrc)" >> $GITHUB_OUTPUT + run: echo "NVMRC=$(cat '.nvmrc')" >> "$GITHUB_OUTPUT"🧰 Tools
🪛 actionlint (1.7.4)
33-33: shellcheck reported issue in this script: SC2086:info:1:31: Double quote to prevent globbing and word splitting
(shellcheck)
🧹 Nitpick comments (1)
.github/workflows/deploy-storybook.yml (1)
44-46: Add error handling to the build step.Consider adding error handling and workspace validation to make the build step more robust.
- name: Build Storybook - run: npm run ui:build + run: | + if [ ! -d "ui" ]; then + echo "Error: ui directory not found" + exit 1 + fi + npm run ui:build || { + echo "Error: Storybook build failed" + exit 1 + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/deploy-storybook.yml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/deploy-storybook.yml
33-33: shellcheck reported issue in this script: SC2086:info:1:31: Double quote to prevent globbing and word splitting
(shellcheck)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Test website
- GitHub Check: Prettify
- GitHub Check: Security checks (typescript)
🔇 Additional comments (2)
.github/workflows/deploy-storybook.yml (2)
1-9: LGTM! Well-configured workflow triggers.The workflow name is descriptive and the triggers are properly set up for both automated (main branch) and manual execution.
11-20: LGTM! Secure permissions and proper concurrency handling.The configuration follows security best practices with minimal required permissions and prevents deployment conflicts.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.github/workflows/deploy-storybook.yml (1)
29-41:⚠️ Potential issueFix shell script quoting in .nvmrc reading step.
The command needs proper quoting to prevent globbing and word splitting issues.
- run: echo "NVMRC=$(cat '.nvmrc')" >> $GITHUB_OUTPUT + run: echo "NVMRC=$(cat '.nvmrc')" >> "$GITHUB_OUTPUT"🧰 Tools
🪛 actionlint (1.7.4)
31-31: shellcheck reported issue in this script: SC2086:info:1:33: Double quote to prevent globbing and word splitting
(shellcheck)
🧹 Nitpick comments (1)
.github/workflows/deploy-storybook.yml (1)
42-55: Add build output verification step.Consider adding a step to verify the Storybook build output path exists before upload.
- name: Build Storybook run: npm run ui:build + - name: Verify build output + run: | + if [ ! -d "ui/storybook-static" ]; then + echo "Error: Storybook build output directory not found" + exit 1 + fi + - name: Setup Pages
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/deploy-storybook.yml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/deploy-storybook.yml
31-31: shellcheck reported issue in this script: SC2086:info:1:33: Double quote to prevent globbing and word splitting
(shellcheck)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Prettify
🔇 Additional comments (4)
.github/workflows/deploy-storybook.yml (4)
1-13: LGTM! Well-defined workflow with proper permissions.The workflow name is descriptive and the permissions follow the principle of least privilege.
14-18: LGTM! Good concurrency configuration.Prevents deployment conflicts and optimizes CI resources.
19-24: LGTM! Proper environment configuration.Correctly sets up GitHub Pages environment with dynamic URL.
26-28: LGTM! Using latest checkout action.Good practice to use the latest v4 version.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
ui/.storybook/main.js (5)
3-9: Add error handling to getAbsolutePath function.The function should handle cases where the package.json cannot be resolved.
function getAbsolutePath(value) { + try { return dirname(require.resolve(join(value, 'package.json'))); + } catch (error) { + throw new Error(`Could not resolve package path for ${value}: ${error.message}`); + } }
15-19: Consider adding more Storybook addons for enhanced development experience.Consider adding these useful addons:
@storybook/addon-a11yfor accessibility testing@storybook/addon-viewportfor responsive design testingaddons: [ getAbsolutePath('@storybook/addon-onboarding'), getAbsolutePath('@storybook/addon-essentials'), getAbsolutePath('@storybook/addon-interactions'), + getAbsolutePath('@storybook/addon-a11y'), + getAbsolutePath('@storybook/addon-viewport'), ],
21-24: Document framework options for future reference.Add a comment explaining available framework options for future customization needs.
framework: { name: getAbsolutePath('@storybook/react-vite'), - options: {}, + options: { + // Available options: + // builder: {} // Configure the Vite builder + // fastRefresh: true // Enable/disable fast refresh + }, },
26-32: Consider additional Vite optimizations.The current configuration only enables PostCSS. Consider adding other useful Vite configurations for better development experience.
viteFinal: async (config) => { // Use PostCSS config from postcss.config.js config.css = { postcss: true, }; + // Add optimizations for development + if (process.env.NODE_ENV === 'development') { + config.optimizeDeps = { + include: ['@storybook/react'], + }; + } return config; },
34-38: Add documentation configuration options.The docs configuration is empty. Consider adding explicit options for better documentation generation.
-docs: {}, +docs: { + autodocs: true, + defaultName: 'Documentation', + docsMode: true, +},ui/src/stories/icons.stories.tsx (3)
33-42: Add JSDoc comment to document available sizes.Document the supported size variations for better developer experience.
+/** + * Demonstrates available icon sizes: + * - Small: h-4 w-4 (16px) + * - Medium: h-6 w-6 (24px) + * - Large: h-8 w-8 (32px) + * - Extra Large: h-12 w-12 (48px) + */ export const Sizes: Story = {
44-53: Add JSDoc comment to document color usage.Document when to use each color variant.
+/** + * Demonstrates themed color variants: + * - accent: Used for highlighting or emphasis + * - primary: Main brand color + * - secondary: Supporting elements + * - destructive: Warning or delete actions + */ export const Colors: Story = {
55-64: Reuse size variations and document animation.Consider extracting size variations into a shared constant and document the animation behavior.
+const ICON_SIZES = [ + { className: "h-4 w-4", label: "Small" }, + { className: "h-6 w-6", label: "Medium" }, + { className: "h-8 w-8", label: "Large" }, + { className: "h-12 w-12", label: "Extra Large" }, +] as const; + +/** + * Demonstrates the loading spinner with built-in animation. + * The spinner rotates continuously using CSS animation. + */ export const AnimatedSpinner: Story = { render: () => ( <div className="flex flex-wrap items-center gap-4"> - <SpinnerIcon className="h-4 w-4" /> - <SpinnerIcon className="h-6 w-6" /> - <SpinnerIcon className="h-8 w-8" /> - <SpinnerIcon className="h-12 w-12" /> + {ICON_SIZES.map(({ className }) => ( + <SpinnerIcon key={className} className={className} /> + ))} </div> ), };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
ui/.storybook/main.js(1 hunks)ui/.storybook/preview.js(1 hunks)ui/package.json(1 hunks)ui/src/stories/icons.stories.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- ui/.storybook/preview.js
- ui/package.json
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Prettify
🔇 Additional comments (2)
ui/.storybook/main.js (1)
11-13: LGTM! Well-structured stories configuration.The stories pattern covers all necessary file types and follows Storybook's recommended structure.
ui/src/stories/icons.stories.tsx (1)
1-13: LGTM! Clean imports and proper Storybook configuration.The metadata setup with autodocs will help generate automatic documentation.
| export const AllIcons: Story = { | ||
| render: () => ( | ||
| <div className="flex flex-wrap gap-8"> | ||
| <div className="flex flex-col items-center gap-2"> | ||
| <DonateIcon className="h-8 w-8" /> | ||
| <span className="text-sm">DonateIcon</span> | ||
| </div> | ||
| <div className="flex flex-col items-center gap-2"> | ||
| <SIIcon className="h-8 w-8" /> | ||
| <span className="text-sm">SIIcon</span> | ||
| </div> | ||
| <div className="flex flex-col items-center gap-2"> | ||
| <SpinnerIcon className="h-8 w-8" /> | ||
| <span className="text-sm">SpinnerIcon</span> | ||
| </div> | ||
| </div> | ||
| ), | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add aria-labels for accessibility.
The icons should have aria-labels for screen readers.
- <DonateIcon className="h-8 w-8" />
+ <DonateIcon className="h-8 w-8" aria-label="Donate icon" />
- <SIIcon className="h-8 w-8" />
+ <SIIcon className="h-8 w-8" aria-label="SI icon" />
- <SpinnerIcon className="h-8 w-8" />
+ <SpinnerIcon className="h-8 w-8" aria-label="Loading spinner" />📝 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.
| export const AllIcons: Story = { | |
| render: () => ( | |
| <div className="flex flex-wrap gap-8"> | |
| <div className="flex flex-col items-center gap-2"> | |
| <DonateIcon className="h-8 w-8" /> | |
| <span className="text-sm">DonateIcon</span> | |
| </div> | |
| <div className="flex flex-col items-center gap-2"> | |
| <SIIcon className="h-8 w-8" /> | |
| <span className="text-sm">SIIcon</span> | |
| </div> | |
| <div className="flex flex-col items-center gap-2"> | |
| <SpinnerIcon className="h-8 w-8" /> | |
| <span className="text-sm">SpinnerIcon</span> | |
| </div> | |
| </div> | |
| ), | |
| }; | |
| export const AllIcons: Story = { | |
| render: () => ( | |
| <div className="flex flex-wrap gap-8"> | |
| <div className="flex flex-col items-center gap-2"> | |
| <DonateIcon className="h-8 w-8" aria-label="Donate icon" /> | |
| <span className="text-sm">DonateIcon</span> | |
| </div> | |
| <div className="flex flex-col items-center gap-2"> | |
| <SIIcon className="h-8 w-8" aria-label="SI icon" /> | |
| <span className="text-sm">SIIcon</span> | |
| </div> | |
| <div className="flex flex-col items-center gap-2"> | |
| <SpinnerIcon className="h-8 w-8" aria-label="Loading spinner" /> | |
| <span className="text-sm">SpinnerIcon</span> | |
| </div> | |
| </div> | |
| ), | |
| }; |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
ui/.storybook/main.js(1 hunks)ui/.storybook/preview.js(1 hunks)ui/package.json(2 hunks)ui/src/stories/icons.stories.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- ui/.storybook/preview.js
- ui/src/stories/icons.stories.tsx
- ui/.storybook/main.js
🔇 Additional comments (2)
ui/package.json (2)
70-71: LGTM: Storybook scripts are correctly configured.The scripts for running and building Storybook are properly set up.
43-43: Verify the tailwind-merge version upgrade.The update from ^2.4.0 to ^2.5.3 should be checked for breaking changes.
✅ Verification successful
The tailwind-merge upgrade is safe ✅
The update to v2.5.3 contains only non-breaking changes: a bug fix for border colors, documentation updates, and internal test improvements.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for breaking changes in tailwind-merge changelog curl -s https://raw.githubusercontent.com/dcastil/tailwind-merge/v2.5.3/CHANGELOG.md | grep -A 5 "## \[2.5"Length of output: 109
Script:
#!/bin/bash # Query npm registry for tailwind-merge package information curl -s https://registry.npmjs.org/tailwind-merge | jq -r '.versions["2.5.3"].dist.tarball' | xargs curl -s | tar -xzO package/CHANGELOG.md | grep -A 10 "## \[2.5"Length of output: 269
Script:
#!/bin/bash # Query GitHub API for release information curl -s "https://api.github.com/repos/dcastil/tailwind-merge/releases/tags/v2.5.3" | jq -r '.body'Length of output: 702
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
ui/src/icons/spinner.tsx (1)
14-14: Consider users who prefer reduced motion.The spinner uses
animate-spinclass but doesn't respect user's motion preferences.Add a media query to pause animation for users who prefer reduced motion:
- className={twMerge('h-5 w-5 animate-spin', className)} + className={twMerge('h-5 w-5 animate-spin motion-reduce:animate-none', className)}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
ui/src/icons/donate.tsx(1 hunks)ui/src/icons/si.tsx(1 hunks)ui/src/icons/spinner.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ui/src/icons/donate.tsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Prettify
| @@ -9,12 +9,12 @@ export function SIIcon({ className, ...props }: HTMLAttributes<SVGElement>) { | |||
| xmlSpace="preserve" | |||
| fill="currentColor" | |||
| aria-hidden={true} | |||
| aria-labelledby="si-icon" | |||
| aria-labelledby="si-icon-title" | |||
There was a problem hiding this comment.
Fix conflicting ARIA attributes.
While the matching of aria-labelledby with id is good, having both aria-hidden="true" and aria-labelledby is contradictory. aria-hidden removes the element from the accessibility tree, preventing screen readers from announcing the title.
Choose one approach:
<svg
version="1.1"
xmlns="http://www.w3.org/2000/svg"
xmlSpace="preserve"
fill="currentColor"
- aria-hidden={true}
aria-labelledby="si-icon-title"
viewBox="0 0 816 815.8"
className={twMerge('text-accent', className)}
{...props}
>
<title id="si-icon-title">Social Income Icon</title>Or if the icon should be decorative:
<svg
version="1.1"
xmlns="http://www.w3.org/2000/svg"
xmlSpace="preserve"
fill="currentColor"
aria-hidden={true}
- aria-labelledby="si-icon-title"
viewBox="0 0 816 815.8"
className={twMerge('text-accent', className)}
{...props}
>
- <title id="si-icon-title">Social Income Icon</title>Also applies to: 17-17
| xmlSpace="preserve" | ||
| fill="none" | ||
| aria-hidden={true} | ||
| aria-labelledby="si-logo-title" | ||
| aria-labelledby="spinner-icon-title" |
There was a problem hiding this comment.
Fix contradicting accessibility attributes.
The SVG has both aria-hidden="true" and aria-labelledby which contradict each other. aria-hidden="true" hides the element from screen readers while aria-labelledby tries to label it for screen readers.
Apply this diff to resolve the contradiction:
- aria-hidden={true}
- aria-labelledby="spinner-icon-title"
+ aria-hidden={false}
+ aria-labelledby="spinner-icon-title"
+ role="status"📝 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.
| xmlSpace="preserve" | |
| fill="none" | |
| aria-hidden={true} | |
| aria-labelledby="si-logo-title" | |
| aria-labelledby="spinner-icon-title" | |
| xmlSpace="preserve" | |
| fill="none" | |
| aria-hidden={false} | |
| aria-labelledby="spinner-icon-title" | |
| role="status" |
Closes #969
This PR introduces the initial integration of Storybook and some AI-generated stories for components. You can run it locally using the command
npm run ui:serve.To simplify the setup, I copied the spinner icon directly from the site. This was necessary because Storybook could not handle imports from another workspace, and copying the asset was the simplest solution for now🙂
Let's discuss the next steps to determine what should be included in this PR:
@socialincome/ui, with the main site importing them from there?Summary by CodeRabbit
Based on the comprehensive changes, here are the updated release notes:
New Features
DonateIcon,SIIcon, andSpinnerIcon.Chores
SpinnerIconimport across multiple files.dropdown-menu,menubar, andnavigation-menucomponents from main exports.DonateIcon,SIIcon,SpinnerIcon) to the main exports.Documentation