Conversation
|
""" WalkthroughA new modular Select UI component system is introduced, including both a user-facing component and a primitive implementation with context, floating UI, and keyboard navigation. Supporting SCSS styles are added, and Storybook stories demonstrate usage and controlled/uncontrolled patterns. The styles are integrated into the default theme. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SelectRoot
participant SelectTrigger
participant SelectContent
participant SelectItem
User->>SelectRoot: Render with props (value, onValueChange, ...)
SelectRoot->>SelectTrigger: Render trigger button
User->>SelectTrigger: Clicks trigger
SelectTrigger->>SelectRoot: Toggle open state
SelectRoot->>SelectContent: Render dropdown content if open
SelectContent->>SelectItem: Render selectable items
User->>SelectItem: Clicks item
SelectItem->>SelectRoot: Calls onValueChange with selected value
SelectRoot->>SelectTrigger: Update trigger display with new value
Possibly related PRs
Suggested labels
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
npm error Exit handler never called! 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (7)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (6)
⏰ Context from checks skipped due to timeout of 90000ms (1)
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 17
🧹 Nitpick comments (25)
src/core/primitives/Select/fragments/SelectPrimitiveOverlay.tsx (3)
6-6: Remove unused import.The
clsxutility is imported but never used in this component.-import { clsx } from 'clsx';
8-10: Consider removing unused className prop or implement it.The
classNameprop is defined but not used since the className application is commented out on line 18.Either remove the unused prop:
-type SelectPrimitiveOverlayProps = { - className?: string; -}; +type SelectPrimitiveOverlayProps = Record<string, never>;Or implement it by uncommenting and fixing line 18:
- // className={clsx(`${rootClass}-overlay`, className)} + className={clsx(`${rootClass}-overlay`, className)}
12-25: Simplify JSX structure.The React fragments are unnecessary since you're only returning a single conditional element.
-function SelectPrimitiveOverlay({ className = '' }: SelectPrimitiveOverlayProps) { +function SelectPrimitiveOverlay() { const { isOpen, handleOverlayClick } = useContext(SelectPrimitiveContext); - return ( - <> - {isOpen && ( - <Floater.Overlay - // className={clsx(`${rootClass}-overlay`, className)} - onClick={handleOverlayClick} - > - </Floater.Overlay> - )} - </> - ); + return isOpen ? ( + <Floater.Overlay onClick={handleOverlayClick} /> + ) : null; }src/core/primitives/Select/fragments/SelectPrimitivePortal.tsx (1)
5-5: Remove unnecessary type assertion.The type assertion
as HTMLElement | nullis redundant sincedocument.bodyis already typed asHTMLElement | null.- const rootElement = document.querySelector('#rad-ui-theme-container') || document.body as HTMLElement | null; + const rootElement = document.querySelector('#rad-ui-theme-container') || document.body;src/components/ui/Select/fragments/SelectRoot.tsx (2)
1-1: Remove unused React hook imports.The
useStateanduseCallbackhooks are imported but never used in this component.-import React, { useState, useCallback } from 'react'; +import React from 'react';
5-5: Consider updating COMPONENT_NAME for clarity.The component name is
SelectRootbutCOMPONENT_NAMEis set to'Select'. Consider whether this should be more specific.-const COMPONENT_NAME = 'Select'; +const COMPONENT_NAME = 'SelectRoot';Or if the intention is to use the base component name for all Select fragments, consider adding a comment explaining this pattern.
src/components/ui/Select/fragments/SelectItem.tsx (1)
20-22: Consider extracting the SVG icon for reusability.The checkmark SVG is hardcoded, which makes it difficult to customize or reuse across the component system.
Consider creating a separate icon component or using an icon library:
+import CheckIcon from '~/components/icons/CheckIcon'; - <svg width="16" height="16" viewBox="0 0 16 16" fill="none" xmlns="http://www.w3.org/2000/svg"> - <path d="M13.3332 4L5.99984 11.3333L2.6665 8" stroke="currentColor" strokeWidth="2" strokeLinecap="round" strokeLinejoin="round"/> - </svg> + <CheckIcon />src/components/ui/Select/fragments/SelectTrigger.tsx (1)
16-18: Remove unnecessary empty lines for cleaner code formatting.The empty lines around the children don't add value and make the code less concise.
> - {children} - </SelectPrimitive.Trigger>src/core/primitives/Select/fragments/SelectPrimitiveItem.tsx (2)
9-9: Add missing semicolon for consistent interface syntax.The
disabledproperty is missing a semicolon, which is inconsistent with TypeScript interface conventions.- disabled?: boolean + disabled?: boolean;
18-21: Improve code formatting for better readability.The spacing and line breaks are inconsistent, making the code harder to read.
<ButtonPrimitive - disabled={disabled} data-value={value} - aria-selected= {value === selectedValue} - onClick={() => handleSelect(value)} {...props} + disabled={disabled} + data-value={value} + aria-selected={value === selectedValue} + onClick={() => handleSelect(value)} + {...props} {...getItemProps()}src/components/ui/Select/fragments/SelectContent.tsx (1)
1-1: Remove unused imports.The imports
useEffect,useRef, anduseContextare not used in this component and should be removed to keep the code clean.-import React, { useEffect, useRef, useContext } from 'react'; +import React from 'react';src/core/primitives/Select/stories/Select.stories.tsx (3)
5-5: Remove unused import.The
Floaterimport is not used in this file and should be removed.-import Floater from '~/core/primitives/Floater';
44-44: Fix typo in trigger text.There appears to be a typo in the trigger text - "helo" should likely be "hello" to match the BasicSelect story.
- helo + hello
41-53: Consider consistency in Portal usage.The
ControlledExampledoesn't useSelectPrimitive.PortalwhileBasicSelectdoes. This inconsistency might be intentional to demonstrate different usage patterns, but it could be confusing. Consider adding a comment explaining the difference or standardizing the approach.src/core/primitives/Select/contexts/SelectPrimitiveContext.tsx (1)
22-22: Consider safer context initialization.Initializing the context with an empty object cast to the type could cause runtime errors if components try to access properties before the provider is set up. Consider providing default values or adding runtime checks.
-export const SelectPrimitiveContext = createContext<SelectPrimitiveContextType>({} as SelectPrimitiveContextType); +export const SelectPrimitiveContext = createContext<SelectPrimitiveContextType | null>(null);This would require components to check for null context and provide better error messages if the context is not properly set up.
src/core/primitives/Select/fragments/SelectPrimitiveTrigger.tsx (1)
10-10: Remove commented out code.The commented line should be removed if it's no longer needed, or if it's for debugging, consider adding a proper comment explaining its purpose.
- // if (isOpen) return null;src/core/primitives/Select/fragments/SelectPrimitiveRoot.tsx (4)
17-17: Improve code formatting for better readability.-function SelectPrimitiveRoot({ children, className, value, defaultValue = '', onValueChange, onClickOutside = () => {}, ...props }: SelectPrimitiveRootProps) { +function SelectPrimitiveRoot({ + children, + className, + value, + defaultValue = '', + onValueChange, + onClickOutside = () => {}, + ...props +}: SelectPrimitiveRootProps) {
35-35: Remove commented code.- // const click = Floater.useClick(context);
42-44: Clean up commented code in interactions array.const { getReferenceProps, getFloatingProps, getItemProps } = Floater.useInteractions([ - // click, dismiss, role ]);
48-48: Break long line for better readability.- const values = { isOpen, setIsOpen, selectedValue, setSelectedValue, handleSelect, floatingContext, refs, getFloatingProps, getReferenceProps, floatingStyles, getItemProps }; + const values = { + isOpen, + setIsOpen, + selectedValue, + setSelectedValue, + handleSelect, + floatingContext, + refs, + getFloatingProps, + getReferenceProps, + floatingStyles, + getItemProps + };src/core/primitives/Select/Select.tsx (2)
8-11: Improve warning message with usage examples.const SelectPrimitive = () => { - console.warn('Direct usage of Select is not supported. Please use Select.Root, Select.Content, etc. instead.'); + console.warn('SelectPrimitive should not be used directly. Use its subcomponents instead:\n' + + 'Example: <SelectPrimitive.Root><SelectPrimitive.Trigger>...</SelectPrimitive.Trigger></SelectPrimitive.Root>'); return null; };
8-20: Add TypeScript types for better type safety.+type SelectPrimitiveType = { + (): null; + Root: typeof SelectPrimitiveRoot; + Content: typeof SelectPrimitiveContent; + Portal: typeof SelectPrimitivePortal; + Overlay: typeof SelectPrimitiveOverlay; + Item: typeof SelectPrimitiveItem; + Trigger: typeof SelectPrimitiveTrigger; +}; + -const SelectPrimitive = () => { +const SelectPrimitive: SelectPrimitiveType = () => { console.warn('SelectPrimitive should not be used directly. Use its subcomponents instead:\n' + 'Example: <SelectPrimitive.Root><SelectPrimitive.Trigger>...</SelectPrimitive.Trigger></SelectPrimitive.Root>'); return null; -}; +} as SelectPrimitiveType;styles/themes/components/select.scss (3)
24-31: Ensure arrow pseudo-element doesn’t block pointer events
The&::afterarrow will inherit pointer events, which may intercept clicks on the trigger. It’s best to explicitly disable pointer events on the pseudo-element..rad-ui-select-trigger { &::after { content: ''; border-style: solid; border-width: 5px 5px 0 5px; border-color: var(--rad-ui-color-accent-600) transparent transparent transparent; margin-left: 8px; transition: transform 0.2s ease; + pointer-events: none; } }
41-51: Consolidate focus styles to avoid duplication
Both:focus-visibleand:focusblocks apply identical outline and box-shadow. To reduce repetition and potential drift, consider merging into a single selector (e.g.&:focus-visible, &:focus) or rely solely on:focus-visiblefor keyboard focus indicators.
224-246: Support reduced-motion preferences for animations
The keyframe animations enhance the UX, but consider respecting users’prefers-reduced-motionsettings by disabling or simplifying these animations under a media query.@media (prefers-reduced-motion: reduce) { .rad-ui-select-content[data-state="open"], .rad-ui-select-content[data-state="closed"] { animation: none !important; transition: none !important; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
src/components/ui/Select/Select.tsx(1 hunks)src/components/ui/Select/fragments/SelectContent.tsx(1 hunks)src/components/ui/Select/fragments/SelectItem.tsx(1 hunks)src/components/ui/Select/fragments/SelectRoot.tsx(1 hunks)src/components/ui/Select/fragments/SelectTrigger.tsx(1 hunks)src/components/ui/Select/stories/Select.stories.tsx(1 hunks)src/core/primitives/Select/Select.tsx(1 hunks)src/core/primitives/Select/contexts/SelectPrimitiveContext.tsx(1 hunks)src/core/primitives/Select/fragments/SelectPrimitiveContent.tsx(1 hunks)src/core/primitives/Select/fragments/SelectPrimitiveItem.tsx(1 hunks)src/core/primitives/Select/fragments/SelectPrimitiveOverlay.tsx(1 hunks)src/core/primitives/Select/fragments/SelectPrimitivePortal.tsx(1 hunks)src/core/primitives/Select/fragments/SelectPrimitiveRoot.tsx(1 hunks)src/core/primitives/Select/fragments/SelectPrimitiveTrigger.tsx(1 hunks)src/core/primitives/Select/stories/Select.stories.tsx(1 hunks)styles/themes/components/select.scss(1 hunks)styles/themes/default.scss(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (5)
src/core/primitives/Select/fragments/SelectPrimitiveOverlay.tsx (1)
src/core/primitives/Select/contexts/SelectPrimitiveContext.tsx (1)
SelectPrimitiveContext(22-22)
src/core/primitives/Select/stories/Select.stories.tsx (1)
src/components/ui/Select/stories/Select.stories.tsx (1)
ControlledExample(132-154)
src/core/primitives/Select/fragments/SelectPrimitiveTrigger.tsx (1)
src/core/primitives/Select/contexts/SelectPrimitiveContext.tsx (1)
SelectPrimitiveContext(22-22)
src/core/primitives/Select/fragments/SelectPrimitiveContent.tsx (1)
src/core/primitives/Select/contexts/SelectPrimitiveContext.tsx (1)
SelectPrimitiveContext(22-22)
src/components/ui/Select/stories/Select.stories.tsx (1)
src/core/primitives/Select/stories/Select.stories.tsx (1)
ControlledExample(36-60)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (9)
styles/themes/default.scss (1)
32-32: LGTM! Import follows the established pattern.The select component styles import is correctly added and maintains alphabetical ordering with other component imports.
src/components/ui/Select/fragments/SelectItem.tsx (1)
15-16: Good accessibility implementation.The component properly implements ARIA attributes including
role="option"andaria-disabled, which ensures screen reader compatibility and follows accessibility best practices.src/core/primitives/Select/fragments/SelectPrimitiveItem.tsx (1)
12-27: Well-structured primitive component with proper context usage.The component correctly uses the SelectPrimitiveContext and implements proper accessibility with
aria-selected. The integration with RovingFocusGroup and ButtonPrimitive follows good composition patterns.src/components/ui/Select/Select.tsx (2)
8-11: Excellent pattern for enforcing modular component usage.The warning message and null return effectively prevent direct usage while clearly guiding developers to use the intended subcomponents. This promotes better component composition patterns.
13-16: Clean modular architecture with static property pattern.The static property assignment pattern provides an intuitive API for accessing subcomponents while maintaining proper module boundaries. This approach aligns well with popular component libraries like Radix UI.
styles/themes/components/select.scss (4)
1-6: Appropriate scoping and default sizing for the root element
The.rad-ui-select-rootblock is well scoped, usinginline-blockfor flexible placement and exposing a sensible default width via CSS variables. This sets a good foundation for theming and layout overrides.
114-194: Item styling is comprehensive and accessible
The.rad-ui-select-itemrules cover font sizing, spacing, disabled states, hover/focus/selected interactions, and ARIA roles. They follow best practices for keyboard navigation and accessibility.
196-202: Label styling is clear and semantically appropriate
The.rad-ui-select-labelblock uses an uppercase, small-font design that clearly distinguishes section labels. This is a solid, self-contained pattern.
207-221: Icon and text flex layout is well-structured
The.rad-ui-select-iconand.rad-ui-select-textblocks correctly use flex properties for alignment and ellipsis overflow on text, ensuring a polished trigger appearance.
|
|
||
| const COMPONENT_NAME = 'Select'; | ||
|
|
||
| function SelectRoot({ customRootClass, children, defaultValue, value, onValueChange, ...props }: any) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type safety by replacing 'any' with proper types.
Using any type reduces type safety and IDE support. Consider defining proper prop types.
+type SelectRootProps = {
+ customRootClass?: string;
+ children?: React.ReactNode;
+ defaultValue?: string;
+ value?: string;
+ onValueChange?: (value: string) => void;
+} & React.ComponentPropsWithoutRef<'div'>;
-function SelectRoot({ customRootClass, children, defaultValue, value, onValueChange, ...props }: any) {
+function SelectRoot({ customRootClass, children, defaultValue, value, onValueChange, ...props }: SelectRootProps) {🤖 Prompt for AI Agents
In src/components/ui/Select/fragments/SelectRoot.tsx at line 7, replace the use
of 'any' for the function props with a properly defined TypeScript interface or
type that explicitly lists all expected props and their types, such as
customRootClass as string or undefined, children as React.ReactNode,
defaultValue and value as appropriate types, and onValueChange as a function
type. This will improve type safety and enhance IDE support.
|
|
||
| const COMPONENT_NAME = 'Select'; | ||
|
|
||
| function SelectItem({ customRootClass, children, value, disabled, ...props }: any) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace any type with proper TypeScript interface for better type safety.
Using any eliminates TypeScript's type checking benefits and reduces IDE support for autocompletion and error detection.
-function SelectItem({ customRootClass, children, value, disabled, ...props }: any) {
+interface SelectItemProps {
+ customRootClass?: string;
+ children: React.ReactNode;
+ value: string;
+ disabled?: boolean;
+ [key: string]: any; // for additional props
+}
+
+function SelectItem({ customRootClass, children, value, disabled, ...props }: SelectItemProps) {📝 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.
| function SelectItem({ customRootClass, children, value, disabled, ...props }: any) { | |
| interface SelectItemProps { | |
| customRootClass?: string; | |
| children: React.ReactNode; | |
| value: string; | |
| disabled?: boolean; | |
| [key: string]: any; // for additional props | |
| } | |
| function SelectItem( | |
| { customRootClass, children, value, disabled, ...props }: SelectItemProps | |
| ) { | |
| // ... | |
| } |
🤖 Prompt for AI Agents
In src/components/ui/Select/fragments/SelectItem.tsx at line 7, replace the use
of the `any` type for the function parameter with a properly defined TypeScript
interface that specifies the expected props such as customRootClass, children,
value, disabled, and any other relevant properties. Define this interface above
the function or in a separate types file, then use it to type the props
parameter to improve type safety and enable better IDE support.
|
|
||
| const COMPONENT_NAME = 'Select'; | ||
|
|
||
| function SelectTrigger({ customRootClass, children, disabled, placeholder, ...props }: any) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace any type with proper TypeScript interface for consistency and type safety.
Similar to SelectItem, this component should use a proper TypeScript interface to maintain type safety across the Select component system.
-function SelectTrigger({ customRootClass, children, disabled, placeholder, ...props }: any) {
+interface SelectTriggerProps {
+ customRootClass?: string;
+ children: React.ReactNode;
+ disabled?: boolean;
+ placeholder?: string;
+ [key: string]: any;
+}
+
+function SelectTrigger({ customRootClass, children, disabled, placeholder, ...props }: SelectTriggerProps) {🤖 Prompt for AI Agents
In src/components/ui/Select/fragments/SelectTrigger.tsx at line 7, replace the
use of the `any` type in the function parameter with a properly defined
TypeScript interface that specifies the expected props such as customRootClass,
children, disabled, placeholder, and any other relevant properties. Define this
interface above the function or import it if it exists, then use it to type the
props parameter to ensure type safety and consistency with other Select
components like SelectItem.
|
|
||
| const COMPONENT_NAME = 'Select'; | ||
|
|
||
| function SelectContent({ customRootClass, children, position = 'popper', ...props }: any) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type safety by defining proper prop types.
The component uses any type for props, which eliminates type checking benefits. Consider defining a proper interface for the component props.
+interface SelectContentProps {
+ customRootClass?: string;
+ children: React.ReactNode;
+ position?: 'popper' | 'item-aligned';
+ [key: string]: any; // for additional props to spread
+}
+
-function SelectContent({ customRootClass, children, position = 'popper', ...props }: any) {
+function SelectContent({ customRootClass, children, position = 'popper', ...props }: SelectContentProps) {📝 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.
| function SelectContent({ customRootClass, children, position = 'popper', ...props }: any) { | |
| interface SelectContentProps { | |
| customRootClass?: string; | |
| children: React.ReactNode; | |
| position?: 'popper' | 'item-aligned'; | |
| [key: string]: any; // for additional props to spread | |
| } | |
| function SelectContent({ customRootClass, children, position = 'popper', ...props }: SelectContentProps) { | |
| // ...rest of the implementation | |
| } |
🤖 Prompt for AI Agents
In src/components/ui/Select/fragments/SelectContent.tsx at line 7, the
SelectContent component uses the 'any' type for its props, which disables type
checking. Define a proper TypeScript interface for the component props
specifying types for customRootClass, children, position, and any other props,
then replace 'any' with this interface to improve type safety.
| export const ControlledExample = () => { | ||
| const [value, setValue] = React.useState(''); | ||
| return ( | ||
| <SandboxEditor> | ||
| <div className="w-[240px]"> | ||
| <Select.Root defaultValue="option1" value={value} onValueChange={setValue}> | ||
| <Select.Trigger> | ||
| helo | ||
| </Select.Trigger> | ||
| <Select.Content> | ||
| <Select.Item value='option1'>Option 1</Select.Item> | ||
| <Select.Item value='option2'>Option 2</Select.Item> | ||
| <Select.Item value='option3'>Option 3</Select.Item> | ||
| </Select.Content> | ||
| </Select.Root> | ||
| </div> | ||
|
|
||
| <div className='mt-32'> | ||
| <p className='text-gray-950'>Value: {value}</p> | ||
| </div> | ||
| </SandboxEditor> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
Fix conflicting props and state in controlled example.
The controlled example has several issues:
- Both
defaultValueandvalueprops are used together, which can cause conflicts - State is initialized with empty string but
defaultValueis "option1"
export const ControlledExample = () => {
- const [value, setValue] = React.useState('');
+ const [value, setValue] = React.useState('option1');
return (
<SandboxEditor>
<div className="w-[240px]">
- <Select.Root defaultValue="option1" value={value} onValueChange={setValue}>
+ <Select.Root value={value} onValueChange={setValue}>
<Select.Trigger>
- helo
+ <span>Select an option</span>
</Select.Trigger>📝 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 ControlledExample = () => { | |
| const [value, setValue] = React.useState(''); | |
| return ( | |
| <SandboxEditor> | |
| <div className="w-[240px]"> | |
| <Select.Root defaultValue="option1" value={value} onValueChange={setValue}> | |
| <Select.Trigger> | |
| helo | |
| </Select.Trigger> | |
| <Select.Content> | |
| <Select.Item value='option1'>Option 1</Select.Item> | |
| <Select.Item value='option2'>Option 2</Select.Item> | |
| <Select.Item value='option3'>Option 3</Select.Item> | |
| </Select.Content> | |
| </Select.Root> | |
| </div> | |
| <div className='mt-32'> | |
| <p className='text-gray-950'>Value: {value}</p> | |
| </div> | |
| </SandboxEditor> | |
| ); | |
| }; | |
| export const ControlledExample = () => { | |
| const [value, setValue] = React.useState('option1'); | |
| return ( | |
| <SandboxEditor> | |
| <div className="w-[240px]"> | |
| <Select.Root value={value} onValueChange={setValue}> | |
| <Select.Trigger> | |
| <span>Select an option</span> | |
| </Select.Trigger> | |
| <Select.Content> | |
| <Select.Item value='option1'>Option 1</Select.Item> | |
| <Select.Item value='option2'>Option 2</Select.Item> | |
| <Select.Item value='option3'>Option 3</Select.Item> | |
| </Select.Content> | |
| </Select.Root> | |
| </div> | |
| <div className='mt-32'> | |
| <p className='text-gray-950'>Value: {value}</p> | |
| </div> | |
| </SandboxEditor> | |
| ); | |
| }; |
🤖 Prompt for AI Agents
In src/components/ui/Select/stories/Select.stories.tsx around lines 132 to 154,
the ControlledExample component incorrectly uses both defaultValue and value
props on Select.Root, causing conflicts, and initializes state with an empty
string while defaultValue is "option1". Remove the defaultValue prop from
Select.Root and initialize the state with "option1" to ensure the component is
properly controlled and consistent.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/components/ui/Select/fragments/SelectItem.tsx (1)
5-5: Replaceanytype with proper TypeScript interface for better type safety.Using
anyeliminates TypeScript's type checking benefits and reduces IDE support for autocompletion and error detection.The past review comment correctly identified this type safety issue. Please implement the suggested TypeScript interface to replace the
anytype.src/components/ui/Select/fragments/SelectTrigger.tsx (1)
6-6: Replaceanytype with proper TypeScript interface for consistency and type safety.Similar to SelectItem, this component should use a proper TypeScript interface to maintain type safety across the Select component system.
The past review comment correctly identified this type safety issue. Please implement the suggested TypeScript interface as recommended.
src/components/ui/Select/fragments/SelectContent.tsx (1)
5-5: Address the type safety issue identified in previous review.This is the same type safety concern raised in the previous review. The component still uses
anytype for props, which eliminates TypeScript's type checking benefits.
🧹 Nitpick comments (2)
src/components/ui/Select/fragments/SelectContent.tsx (2)
1-1: Remove unused imports.The
useEffectanduseRefhooks are imported but never used in this component. Clean up the imports to only include what's necessary.-import React, { useEffect, useRef, useContext } from 'react'; +import React, { useContext } from 'react';
7-18: Clean up JSX formatting.The empty lines within the JSX structure (lines 12 and 15) are unnecessary and make the component less compact.
return ( <SelectPrimitive.Content className={`${rootClass}-content`} position={position} data-position={position} - {...props} > - {children} </SelectPrimitive.Content> );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/components/ui/Select/Select.tsx(1 hunks)src/components/ui/Select/contexts/SelectRootContext.tsx(1 hunks)src/components/ui/Select/fragments/SelectContent.tsx(1 hunks)src/components/ui/Select/fragments/SelectIndicator.tsx(1 hunks)src/components/ui/Select/fragments/SelectItem.tsx(1 hunks)src/components/ui/Select/fragments/SelectRoot.tsx(1 hunks)src/components/ui/Select/fragments/SelectTrigger.tsx(1 hunks)src/components/ui/Select/stories/Select.stories.tsx(1 hunks)src/core/primitives/Select/fragments/SelectPrimitiveContent.tsx(1 hunks)src/core/primitives/Select/fragments/SelectPrimitiveRoot.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/components/ui/Select/contexts/SelectRootContext.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- src/core/primitives/Select/fragments/SelectPrimitiveRoot.tsx
- src/components/ui/Select/fragments/SelectRoot.tsx
- src/core/primitives/Select/fragments/SelectPrimitiveContent.tsx
- src/components/ui/Select/stories/Select.stories.tsx
🧰 Additional context used
🧬 Code Graph Analysis (3)
src/components/ui/Select/fragments/SelectIndicator.tsx (1)
src/components/ui/Select/contexts/SelectRootContext.tsx (1)
SelectRootContext(7-9)
src/components/ui/Select/fragments/SelectTrigger.tsx (1)
src/components/ui/Select/contexts/SelectRootContext.tsx (1)
SelectRootContext(7-9)
src/components/ui/Select/fragments/SelectContent.tsx (1)
src/components/ui/Select/contexts/SelectRootContext.tsx (1)
SelectRootContext(7-9)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (4)
src/components/ui/Select/fragments/SelectIndicator.tsx (1)
1-16: LGTM! Clean and well-structured indicator component.The implementation is excellent with proper context usage, accessible SVG markup, and clean separation of concerns. The use of
currentColorfor the stroke ensures proper theming integration.src/components/ui/Select/Select.tsx (1)
7-18: LGTM! Excellent modular component architecture.The wrapper pattern with warning for direct usage and static subcomponent properties is a great architectural choice. This enforces proper usage of the modular API while providing a clean namespace for all Select-related components.
src/components/ui/Select/fragments/SelectItem.tsx (1)
8-21: LGTM! Excellent accessibility and component structure.The implementation correctly uses accessibility attributes (
role="option",aria-disabled,data-disabled) and follows good patterns for wrapping the primitive component while maintaining proper styling context.src/components/ui/Select/fragments/SelectTrigger.tsx (1)
8-20: LGTM! Well-implemented trigger component with proper accessibility.The implementation correctly uses accessibility attributes and follows good patterns for wrapping the primitive trigger while maintaining styling context and prop forwarding.
| function SelectContent({ customRootClass, children, position = 'popper', ...props }: any) { | ||
| const { rootClass } = useContext(SelectRootContext); | ||
| return ( | ||
| <SelectPrimitive.Content | ||
| className={`${rootClass}-content`} | ||
| position={position} | ||
| data-position={position} | ||
|
|
||
| {...props} | ||
| > | ||
|
|
||
| {children} | ||
| </SelectPrimitive.Content> | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Clarify the purpose of the customRootClass prop.
The customRootClass prop is destructured from the component props but is never used in the implementation. Either implement its intended functionality or remove it from the prop destructuring to avoid confusion.
If customRootClass should override the context-provided rootClass, consider this implementation:
function SelectContent({ customRootClass, children, position = 'popper', ...props }: any) {
const { rootClass } = useContext(SelectRootContext);
+ const effectiveRootClass = customRootClass || rootClass;
return (
<SelectPrimitive.Content
- className={`${rootClass}-content`}
+ className={`${effectiveRootClass}-content`}
position={position}
data-position={position}
{...props}
>
{children}
</SelectPrimitive.Content>
);
}Alternatively, if it's not needed, remove it from the props:
-function SelectContent({ customRootClass, children, position = 'popper', ...props }: any) {
+function SelectContent({ children, position = 'popper', ...props }: any) {📝 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.
| function SelectContent({ customRootClass, children, position = 'popper', ...props }: any) { | |
| const { rootClass } = useContext(SelectRootContext); | |
| return ( | |
| <SelectPrimitive.Content | |
| className={`${rootClass}-content`} | |
| position={position} | |
| data-position={position} | |
| {...props} | |
| > | |
| {children} | |
| </SelectPrimitive.Content> | |
| ); | |
| } | |
| function SelectContent({ customRootClass, children, position = 'popper', ...props }: any) { | |
| const { rootClass } = useContext(SelectRootContext); | |
| const effectiveRootClass = customRootClass || rootClass; | |
| return ( | |
| <SelectPrimitive.Content | |
| className={`${effectiveRootClass}-content`} | |
| position={position} | |
| data-position={position} | |
| {...props} | |
| > | |
| {children} | |
| </SelectPrimitive.Content> | |
| ); | |
| } |
🤖 Prompt for AI Agents
In src/components/ui/Select/fragments/SelectContent.tsx around lines 5 to 19,
the customRootClass prop is destructured but never used, causing confusion.
Either remove customRootClass from the component's props if it is unnecessary,
or update the implementation to use customRootClass to override the rootClass
from context by applying customRootClass in the className instead of rootClass.
Ensure the className reflects the correct root class based on whether
customRootClass is provided.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
src/components/ui/AlertDialog/fragments/AlertDialogPortal.tsx (1)
13-13: Remove debug console.log statement.This debug statement should be removed from production code.
src/components/ui/Select/stories/Select.stories.tsx (3)
116-116: Fix conflicting props in controlled example.Using both
defaultValueandvalueprops together can cause conflicts in controlled components. The state should be initialized to match the default value or the default value should be removed.
118-118: Fix typo in trigger content.The text "helo" appears to be a typo and should be corrected to meaningful placeholder text.
112-112: Fix inconsistent state initialization.The state is initialized with an empty string while
defaultValueis set to "option1", creating inconsistency.
🧹 Nitpick comments (10)
src/components/ui/Select/fragments/SelectPortal.tsx (2)
2-2: Remove unused import.The
useContextimport is not used in this component.-import React, { useContext } from 'react'; +import React from 'react';
10-10: Remove unused variable.The
rootElementvariable is calculated but not used sinceSelectPrimitive.Portalhandles its own root resolution.- const rootElement = document.querySelector('#rad-ui-theme-container') || document.body as HTMLElement | null;styles/themes/components/select.scss (8)
22-32: Add smooth transition to the dropdown arrow
The arrow::aftercontent rotates when the dropdown opens, but there’s no transition defined. Adding a transition (e.g.,transition: transform 0.2s ease) to the&::afterblock will make the rotation smoother..rad-ui-select-trigger { &::after { content: ''; border-style: solid; border-width: 5px 5px 0 5px; border-color: var(--rad-ui-color-accent-600) transparent transparent transparent; margin-left: 8px; + transition: transform 0.2s ease; } &[data-state="open"]::after { transform: rotate(180deg); } }
38-46: Combine duplicate focus and focus-visible styles
The&:focus-visibleand&:focusblocks repeat the same outline and box-shadow rules. You can merge them to reduce duplication:.rad-ui-select-trigger { - &:focus-visible { - outline: none; - box-shadow: 0 0 0 2px var(--rad-ui-color-accent-600); - } - - &:focus { - outline: none; - box-shadow: 0 0 0 2px var(--rad-ui-color-accent-600); - } + &:focus, &:focus-visible { + outline: none; + box-shadow: 0 0 0 2px var(--rad-ui-color-accent-600); + } }
66-83: Refine overflow declarations for clarity
Usingoverflow: hidden;followed byoverflow-y: auto;can be misleading. Replace the generaloverflowrule withoverflow-x: hidden;to explicitly target horizontal overflow:.rad-ui-select-content { - overflow: hidden; - /* ... */ - overflow-y: auto; + overflow-x: hidden; + /* ... */ + overflow-y: auto; }
116-125: Merge duplicate focus states for items
In.rad-ui-select-item, the&:focus-visibleand&:focusselectors apply identical styles. Consolidate them to adhere to DRY:.rad-ui-select-item { - &:focus-visible { - outline: none; - background-color: var(--rad-ui-color-accent-300); - box-shadow: 0 0 0 2px var(--rad-ui-color-accent-500) inset; - } - - &:focus { - outline: none; - background-color: var(--rad-ui-color-accent-300); - } + &:focus, &:focus-visible { + outline: none; + background-color: var(--rad-ui-color-accent-300); + box-shadow: 0 0 0 2px var(--rad-ui-color-accent-500) inset; + } }
138-150: Simplify selected-item indicator specificity
You target.rad-ui-select-item-indicatorinside the[aria-selected]state. Since you already have an&-indicatorblock, nest it under the selected state to reduce selector verbosity:.rad-ui-select-item { /* ... */ - &[aria-selected="true"] .rad-ui-select-item-indicator { - opacity: 1; - } + &[aria-selected="true"] { + &-indicator { + opacity: 1; + } + } }
161-167: Normalize indentation for label selector
The.rad-ui-select-labelblock is indented differently than other top-level selectors. Aligning it to column zero improves readability:- .rad-ui-select-label { + .rad-ui-select-label {
169-176: Normalize indentation for icon selector
Similarly,.rad-ui-select-iconshould be aligned with other root-level classes for consistency:- .rad-ui-select-icon { + .rad-ui-select-icon {
178-183: Normalize indentation for text selector
The.rad-ui-select-textblock also has extra indentation. Outdent it to match the project’s top-level style convention:- .rad-ui-select-text { + .rad-ui-select-text {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
src/components/ui/AlertDialog/fragments/AlertDialogPortal.tsx(1 hunks)src/components/ui/Select/Select.tsx(1 hunks)src/components/ui/Select/fragments/SelectPortal.tsx(1 hunks)src/components/ui/Select/fragments/SelectTrigger.tsx(1 hunks)src/components/ui/Select/stories/Select.stories.tsx(1 hunks)src/core/primitives/Select/fragments/SelectPrimitiveContent.tsx(1 hunks)src/core/primitives/Select/fragments/SelectPrimitiveItem.tsx(1 hunks)src/core/primitives/Select/fragments/SelectPrimitiveOverlay.tsx(1 hunks)src/core/primitives/Select/fragments/SelectPrimitivePortal.tsx(1 hunks)src/core/primitives/Select/fragments/SelectPrimitiveRoot.tsx(1 hunks)src/core/primitives/Select/fragments/SelectPrimitiveTrigger.tsx(1 hunks)styles/themes/components/select.scss(1 hunks)styles/themes/default.scss(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- styles/themes/default.scss
🚧 Files skipped from review as they are similar to previous changes (7)
- src/components/ui/Select/fragments/SelectTrigger.tsx
- src/components/ui/Select/Select.tsx
- src/core/primitives/Select/fragments/SelectPrimitiveTrigger.tsx
- src/core/primitives/Select/fragments/SelectPrimitiveOverlay.tsx
- src/core/primitives/Select/fragments/SelectPrimitiveItem.tsx
- src/core/primitives/Select/fragments/SelectPrimitiveContent.tsx
- src/core/primitives/Select/fragments/SelectPrimitiveRoot.tsx
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/components/ui/Select/stories/Select.stories.tsx (1)
src/core/primitives/Select/stories/Select.stories.tsx (1)
ControlledExample(36-60)
src/core/primitives/Select/fragments/SelectPrimitivePortal.tsx (1)
src/core/primitives/Select/contexts/SelectPrimitiveContext.tsx (1)
SelectPrimitiveContext(22-22)
🔇 Additional comments (1)
styles/themes/components/select.scss (1)
1-5: .rad-ui-select-root: baseline styles
Root container styling is well-structured—positioning, inline-block display, and width fallback using a CSS variable provide good flexibility.
…/radui-fork into select-primitive-t1
kotAPI
added
the
automerge
2 participants
New Select primitive component
includes -
new Select theme component
20250512-2058-21.8318537.mp4
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Style
Documentation