Improve tabs in UI kit #1022
Improve tabs in UI kit #1022
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
📝 WalkthroughWalkthroughThis pull request introduces updates to documentation and UI component styling. The Changes
Possibly related PRs
Suggested Reviewers
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: 0
🧹 Nitpick comments (2)
ui/src/stories/tabs.stories.tsx (2)
99-106: Flexible rendering approach.
Using a destructured render function for story-specific props keeps the code well-structured. Consider adding quick comment docs for future maintainers.
208-239: Interactive icons within tabs.
Including HeroIcons with customizable size demonstrates real-world usage. Inline styles are fine for small demos but consider using a common utility class for icon styling as a next step.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
README.md(1 hunks)ui/src/components/tabs.tsx(1 hunks)ui/src/stories/tabs.stories.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- README.md
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Prettify
- GitHub Check: Security checks (typescript)
🔇 Additional comments (13)
ui/src/components/tabs.tsx (1)
31-31: Check accessibility for color changes.
Using aprimarybackground with white text can reduce readability if contrast is insufficient. Ensure that the updated theme maintains adequate contrast, especially for users with impaired vision.ui/src/stories/tabs.stories.tsx (12)
3-3: Good import usage for icons.
Importing icons from@heroicons/react/24/outlineis an effective way to keep them consistent and well-maintained.
9-25: Clear documentation structure.
The expanded docs parameters and story description provide better user guidance. This helps in Storybook’s discoverability and clarifies the component’s usage.
26-55: Comprehensive argTypes definition.
Defining controls and describing props ensure an improved developer experience. Hiding irrelevant props likedir,asChild, andorientationkeeps the Storybook interface uncluttered.
60-66: Smart separation of interfaces.
DefiningBasicStoryPropsclarifies what custom props the story supports. This is a tidy approach that enhances code readability and organization.
68-98: Appropriate usage example.
Showcasing theBasicstory with dynamic labels and content is a great introduction to the component’s core functionality.
111-115: Interface for styling customization.
Enabling style props at story level promotes a clear demonstration of theming and extends usage possibilities.
118-162: Clear demonstration of optional classes.
Exhibiting how to pass custom classes via props helps teams adopt consistent styling practices across different UI scenarios.
167-169: Nice approach to layout props.
Accepting agapprop for spacing between tabs is a flexible solution for design variations.
171-199: Effective example for full-width usage.
Explicitly rendering tabs in a full-width layout with optional gap showcases advanced alignment patterns.
204-206: Icon sizing prop.
Providing a dedicatediconSizeprop is a convenient approach, ensuring consistent sizing across various stories.
244-244: Rich content story is well separated.
The separate export forWithRichContentkeeps the code organized.
246-280: Demonstrating advanced content.
Showcasing form elements and textual content in tabs is a strong plus, illustrating the component’s flexibility and user flow.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
ui/src/stories/tabs.stories.tsx (2)
60-113: Consider using array-based implementation for better scalability.The current implementation with tab1/tab2 props could be refactored to use an array-based approach for better maintainability.
-interface BasicStoryProps extends React.ComponentProps<typeof Tabs> { - tab1Label?: string; - tab2Label?: string; - tab1Content?: string; - tab2Content?: string; +interface Tab { + label: string; + content: string; + value: string; +} + +interface BasicStoryProps extends React.ComponentProps<typeof Tabs> { + tabs?: Tab[]; } export const Basic: StoryObj<BasicStoryProps> = { args: { defaultValue: 'tab1', - tab1Label: 'Tab 1', - tab2Label: 'Tab 2', + tabs: [ + { label: 'Tab 1', content: 'Content for Tab 1', value: 'tab1' }, + { label: 'Tab 2', content: 'Content for Tab 2', value: 'tab2' }, + ], }, // ... render: ({ - tab1Label = 'Tab 1', - tab2Label = 'Tab 2', - tab1Content = 'Content for Tab 1', - tab2Content = 'Content for Tab 2', + tabs = [ + { label: 'Tab 1', content: 'Content for Tab 1', value: 'tab1' }, + { label: 'Tab 2', content: 'Content for Tab 2', value: 'tab2' }, + ], ...args }) => ( <Tabs {...args}> <TabsList> - <TabsTrigger value="tab1">{tab1Label}</TabsTrigger> - <TabsTrigger value="tab2">{tab2Label}</TabsTrigger> + {tabs.map(tab => ( + <TabsTrigger key={tab.value} value={tab.value}> + {tab.label} + </TabsTrigger> + ))} </TabsList> - <TabsContent value="tab1">{tab1Content}</TabsContent> - <TabsContent value="tab2">{tab2Content}</TabsContent> + {tabs.map(tab => ( + <TabsContent key={tab.value} value={tab.value}> + {tab.content} + </TabsContent> + ))} </Tabs> ),
117-174: Consider using a class name utility for better class management.The custom class names could benefit from using a utility like
clsxortailwind-mergeto handle conditional and merged classes.+import { cn } from '../lib/utils'; // Add utility import render: ({ listClassName, triggerClassName, contentClassName, ...args }) => ( <Tabs {...args}> - <TabsList className={listClassName}> + <TabsList className={cn('bg-gray-100 p-2 rounded-lg', listClassName)}>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
README.md(1 hunks)ui/src/components/tabs.tsx(1 hunks)ui/src/stories/tabs.stories.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- ui/src/components/tabs.tsx
- README.md
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Prettify
🔇 Additional comments (2)
ui/src/stories/tabs.stories.tsx (2)
1-57: Well-structured meta configuration and documentation!The meta configuration provides clear documentation and appropriate controls configuration.
220-257: Well-implemented icon integration!The HeroIcons integration and dynamic sizing are implemented correctly.
| <TabsContent value="profile" className={`space-y-4`}> | ||
| <h3 className="text-lg font-medium">Profile Settings</h3> | ||
| <p className="text-muted-foreground text-sm">Update your profile information and preferences.</p> | ||
| <input type="text" placeholder="Display Name" className="flex h-10 w-full rounded-md border px-3" /> | ||
| </TabsContent> | ||
| <TabsContent value="tab2" className="space-y-4"> | ||
| <div className="space-y-2"> | ||
| <h3 className="text-lg font-medium">Preferences</h3> | ||
| <p className="text-muted-foreground text-sm">Customize your application preferences.</p> | ||
| </div> | ||
| <div className="grid gap-2"> | ||
| <label className="flex items-center space-x-2"> | ||
| <input type="checkbox" /> | ||
| <span>Enable notifications</span> | ||
| </label> | ||
| <TabsContent value="notifications" className={`space-y-4`}> | ||
| <h3 className="text-lg font-medium">Notification Settings</h3> | ||
| <p className="text-muted-foreground text-sm">Choose how you want to be notified.</p> | ||
| <div className="flex items-center space-x-2"> | ||
| <input type="checkbox" id="emailNotifications" /> | ||
| <label htmlFor="emailNotifications">Email notifications</label> | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add accessibility attributes to form elements.
The form elements should include proper accessibility attributes.
-<input type="text" placeholder="Display Name" className="flex h-10 w-full rounded-md border px-3" />
+<input
+ type="text"
+ placeholder="Display Name"
+ className="flex h-10 w-full rounded-md border px-3"
+ aria-label="Display Name"
+ required
+/>
-<input type="checkbox" id="emailNotifications" />
+<input
+ type="checkbox"
+ id="emailNotifications"
+ aria-describedby="emailNotifications-description"
+/>
<label htmlFor="emailNotifications">Email notifications</label>
+<p id="emailNotifications-description" className="sr-only">
+ Toggle email notifications for your account
+</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.
| <TabsContent value="profile" className={`space-y-4`}> | |
| <h3 className="text-lg font-medium">Profile Settings</h3> | |
| <p className="text-muted-foreground text-sm">Update your profile information and preferences.</p> | |
| <input type="text" placeholder="Display Name" className="flex h-10 w-full rounded-md border px-3" /> | |
| </TabsContent> | |
| <TabsContent value="tab2" className="space-y-4"> | |
| <div className="space-y-2"> | |
| <h3 className="text-lg font-medium">Preferences</h3> | |
| <p className="text-muted-foreground text-sm">Customize your application preferences.</p> | |
| </div> | |
| <div className="grid gap-2"> | |
| <label className="flex items-center space-x-2"> | |
| <input type="checkbox" /> | |
| <span>Enable notifications</span> | |
| </label> | |
| <TabsContent value="notifications" className={`space-y-4`}> | |
| <h3 className="text-lg font-medium">Notification Settings</h3> | |
| <p className="text-muted-foreground text-sm">Choose how you want to be notified.</p> | |
| <div className="flex items-center space-x-2"> | |
| <input type="checkbox" id="emailNotifications" /> | |
| <label htmlFor="emailNotifications">Email notifications</label> | |
| </div> | |
| <TabsContent value="profile" className={`space-y-4`}> | |
| <h3 className="text-lg font-medium">Profile Settings</h3> | |
| <p className="text-muted-foreground text-sm">Update your profile information and preferences.</p> | |
| <input | |
| type="text" | |
| placeholder="Display Name" | |
| className="flex h-10 w-full rounded-md border px-3" | |
| aria-label="Display Name" | |
| required | |
| /> | |
| </TabsContent> | |
| <TabsContent value="notifications" className={`space-y-4`}> | |
| <h3 className="text-lg font-medium">Notification Settings</h3> | |
| <p className="text-muted-foreground text-sm">Choose how you want to be notified.</p> | |
| <div className="flex items-center space-x-2"> | |
| <input | |
| type="checkbox" | |
| id="emailNotifications" | |
| aria-describedby="emailNotifications-description" | |
| /> | |
| <label htmlFor="emailNotifications">Email notifications</label> | |
| <p id="emailNotifications-description" className="sr-only"> | |
| Toggle email notifications for your account | |
| </p> | |
| </div> |
| }, | ||
| render: ({ gap = 2, ...args }) => ( | ||
| <Tabs {...args}> | ||
| <TabsList className={`flex w-full justify-between gap-${gap}`}> |
There was a problem hiding this comment.
Fix dynamic Tailwind class generation.
Using template literals for Tailwind classes (gap-${gap}) won't work in production as Tailwind needs to statically analyze class names.
-<TabsList className={`flex w-full justify-between gap-${gap}`}>
+<TabsList className={cn('flex w-full justify-between', {
+ 'gap-0': gap === 0,
+ 'gap-1': gap === 1,
+ 'gap-2': gap === 2,
+ 'gap-3': gap === 3,
+ 'gap-4': gap === 4,
+ 'gap-5': gap === 5,
+ 'gap-6': gap === 6,
+ 'gap-7': gap === 7,
+ 'gap-8': gap === 8,
+})}>📝 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.
| <TabsList className={`flex w-full justify-between gap-${gap}`}> | |
| <TabsList className={cn('flex w-full justify-between', { | |
| 'gap-0': gap === 0, | |
| 'gap-1': gap === 1, | |
| 'gap-2': gap === 2, | |
| 'gap-3': gap === 3, | |
| 'gap-4': gap === 4, | |
| 'gap-5': gap === 5, | |
| 'gap-6': gap === 6, | |
| 'gap-7': gap === 7, | |
| 'gap-8': gap === 8, | |
| })}> |
|
Visit the preview URL for this PR (updated for commit 75259e3): https://si-admin-staging--pr1022-almsh-impove-tabs-brkeosuw.web.app (expires Fri, 31 Jan 2025 18:01:23 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: b7b0969384059dce6ea8fad1ee1d1737e54e6676 |
Summary by CodeRabbit
Release Notes
Documentation
UI Improvements
Storybook Enhancement