Feat/#251 select component #267
Conversation
WalkthroughThe pull request introduces a new React component named Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant P as Page Component
participant S as Select Dropdown
participant I as Input Field
U->>P: Request page render
P->>S: Render Select with release options
P->>I: Render Input with label/helper text
U->>S: Open dropdown and select option
U->>I: Enter text into Input field
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 ESLint
Scope: all 2 workspace projects This error happened while installing a direct dependency of /tmp/eslint @repo/eslint-config is not in the npm registry, or you have no permission to fetch it. No authorization header was set for the request. 📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
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: 2
🔭 Outside diff range comments (1)
apps/core/app/test/page.tsx (1)
1-34: 💡 Verification agent🧩 Analysis chain
Add proper testing setup
Since this is a test page, consider adding proper test cases using React Testing Library or similar testing framework.
🏁 Script executed:
#!/bin/bash # Check if testing libraries are installed npm list @testing-library/react @testing-library/jest-dom # Check for existing test files find . -name "*.test.tsx" -o -name "*.spec.tsx"Length of output: 167
Action Required: Set Up Testing for the Page Component
There are no testing libraries installed and no test files detected, so please add a proper testing setup. Consider the following:
- Install Testing Libraries: Add packages like
@testing-library/reactand@testing-library/jest-domto your project dependencies.- Write Test Cases: Create test files (e.g.,
page.test.tsx) for the component, covering key interactions and UI rendering.- Setup Test Runner: Configure your project to run tests (using Jest or a similar framework) to ensure future changes don’t break expected functionality.
🧹 Nitpick comments (4)
packages/ui/src/components/atoms/base-input.tsx (2)
6-10: Add TypeScript documentationConsider adding JSDoc comments to improve code documentation and IDE support.
+/** + * Base input props interface extending HTML input attributes + * @property {boolean} error - Indicates if the input is in an error state + */ export interface BaseInputProps extends Omit<React.InputHTMLAttributes<HTMLInputElement>, "size">, VariantProps<typeof baseInputVariants> { error?: boolean; }
12-29: Optimize focus transition and error state visibility
- The transition duration of 500ms for focus state might feel slow
- Consider adding a subtle background color change for error state
export const baseInputVariants = cva( - "flex h-10 w-full bg-card rounded-md border border-border ring-offset-background file:border-0 file:bg-transparent transition-shadow duration-500 focus-visible:outline-none disabled:cursor-not-allowed disabled:opacity-50 text-sm file:text-sm placeholder:text-muted-foreground font-normal file:font-medium focus:ring-2 focus:ring-primary focus:ring-offset-2", + "flex h-10 w-full bg-card rounded-md border border-border ring-offset-background file:border-0 file:bg-transparent transition-shadow duration-200 focus-visible:outline-none disabled:cursor-not-allowed disabled:opacity-50 text-sm file:text-sm placeholder:text-muted-foreground font-normal file:font-medium focus:ring-2 focus:ring-primary focus:ring-offset-2", { variants: { size: { sm: "px-4 py-2.5 h-9", md: "px-4 py-2.5 h-[52px]", lg: "px-4 py-4 h-14", }, error: { - true: "ring-2 ring-error focus-visible:ring-error focus-visible:ring-offset-2", + true: "ring-2 ring-error focus-visible:ring-error focus-visible:ring-offset-2 bg-error/5", }, }, defaultVariants: { size: "md", }, }, );packages/ui/src/components/atoms/select.tsx (2)
23-26: Improve hover states and touch targetsThe SelectTrigger could benefit from:
- Hover state styling
- Larger touch targets for mobile users (minimum 44px as per WCAG)
className={cn( - "flex h-10 w-full items-center justify-between rounded-lg border border-input px-3 py-2 text-sm placeholder:text-muted-foreground focus:outline-none disabled:cursor-not-allowed disabled:opacity-50 [&>span]:line-clamp-1", + "flex h-11 w-full items-center justify-between rounded-lg border border-input px-3 py-2 text-sm placeholder:text-muted-foreground focus:outline-none disabled:cursor-not-allowed disabled:opacity-50 [&>span]:line-clamp-1 hover:bg-muted/50 transition-colors", baseInputVariants({ size: "sm" }), className, )}
80-81: Ensure consistent border radiusThe SelectContent uses a different border radius than the trigger. Consider using the same radius for visual consistency.
className={cn( - "relative z-50 max-h-96 min-w-[8rem] overflow-hidden rounded-lg border bg-card text-popover-foreground shadow-md data-[state=open]:animate-in data-[state=closed]:animate-out data-[state=closed]:fade-out-0 data-[state=open]:fade-in-0 data-[state=closed]:zoom-out-95 data-[state=open]:zoom-in-95 data-[side=bottom]:slide-in-from-top-2 data-[side=left]:slide-in-from-right-2 data-[side=right]:slide-in-from-left-2 data-[side=top]:slide-in-from-bottom-2", + "relative z-50 max-h-96 min-w-[8rem] overflow-hidden rounded-lg border bg-card text-popover-foreground shadow-md data-[state=open]:animate-in data-[state=closed]:animate-out data-[state=closed]:fade-out-0 data-[state=open]:fade-in-0 data-[state=closed]:zoom-out-95 data-[state=open]:zoom-in-95 data-[side=bottom]:slide-in-from-top-2 data-[side=left]:slide-in-from-right-2 data-[side=right]:slide-in-from-left-2 data-[side=top]:slide-in-from-bottom-2",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/core/app/test/page.tsx(1 hunks)packages/ui/src/components/atoms/base-input.tsx(1 hunks)packages/ui/src/components/atoms/select.tsx(11 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: merge-pr
mrbadri
left a comment
mrbadri
left a comment
There was a problem hiding this comment.
@ErfanKashef
Please Add Select Component in Storybook
same Button Component
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/storybook/src/stories/select.stories.tsx (2)
51-63: Consider adding more customization examples.The custom styling example demonstrates Tailwind class customization well, but consider expanding it to showcase more advanced customization scenarios such as different sizes, states (disabled, error), or custom dropdown positioning.
1-64: Missing accessibility testing scenarios.The stories should include examples demonstrating keyboard navigation, screen reader support, and ARIA attributes handling. This ensures the component meets accessibility standards.
Consider adding examples like:
export const WithAccessibilityFeatures: Story = { render: () => ( <div> <label id="fruit-label">Select a fruit</label> <Select aria-labelledby="fruit-label"> <SelectTrigger> <SelectValue placeholder="Choose a fruit" /> </SelectTrigger> <SelectContent> <SelectItem value="apple">Apple</SelectItem> <SelectItem value="banana">Banana</SelectItem> <SelectItem value="orange">Orange</SelectItem> </SelectContent> </Select> <p className="text-sm text-gray-500 mt-1"> Press Tab to focus, Space to open, and Arrow keys to navigate </p> </div> ), };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockbis excluded by!**/bun.lockb
📒 Files selected for processing (1)
apps/storybook/src/stories/select.stories.tsx(1 hunks)
🔇 Additional comments (3)
apps/storybook/src/stories/select.stories.tsx (3)
1-12: Good component organization.The imports are well-structured with appropriate grouping - React core, Storybook utilities, and the Select component with its subcomponents. This follows best practices for import organization.
14-21: Proper Storybook metadata setup.The metadata configuration is well-structured with appropriate title, tags, and control parameters. The use of
autodocstag will automatically generate documentation for this component, which is excellent for maintainability.
27-49: Well-structured Select component demonstration.The Default story showcases a complete implementation with proper component hierarchy, including groups, labels, items, and separators. This provides a good reference for developers who will use this component.
ErfanKashef
force-pushed
the
feat/#251-Select-Component
branch
from
6d77152 to
dea5840
Compare
|
Closes #251 |
|
Close #251 |
close
Summary by CodeRabbit
Selectcomponent, showcasing standard and custom styling variations.