add select primitive #863
add select primitive #863
Conversation
WalkthroughThe changes introduce a new dropdown feature by adding a Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Button as Dropdown Button
participant Component as SelectPrimitive
User->>Button: Clicks button to toggle dropdown
Button->>Component: Call handleClick()
Component-->>User: Renders list of options if open
User->>Component: Clicks a dropdown option
Component->>Component: Call handleSelect()
Component-->>User: Updates selected option and closes dropdown
Suggested labels
Suggested reviewers
Poem
Tip 🌐 Web search-backed reviews and chat
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
🧹 Nitpick comments (5)
src/core/primitives/Dropdown/Select/SelectPrimitive.stories.tsx (3)
5-5: Consider extracting options to a constants file.The hardcoded options array could be moved to a separate constants file for better maintainability and reusability.
10-15: Consider adding more story variants.The current story only shows basic usage. Consider adding more variants to demonstrate different states and configurations:
- With a pre-selected value
- With a custom onChange handler
- With different styling variations
19-23: Fix formatting issues.There are a few minor formatting issues to address:
export const All = { args: { - children: 'Select Option', + children: 'Select Option' } -} +} +🧰 Tools
🪛 ESLint
[error] 21-21: Unexpected trailing comma.
(comma-dangle)
[error] 23-23: Newline required at end of file but not found.
(eol-last)
src/core/primitives/Dropdown/Select/index.tsx (2)
4-9: Consider enhancing the props interface.The current props interface could be improved:
- Add
classNameprop for custom styling- Add
disabledprop for disabling the select- Add
placeholderprop for empty state- Consider making
childrenrequired or providing a default value
38-38: Add missing newline at end of file.-export default SelectPrimitive +export default SelectPrimitive +🧰 Tools
🪛 ESLint
[error] 38-38: Newline required at end of file but not found.
(eol-last)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/core/primitives/Dropdown/Select/SelectPrimitive.stories.tsx(1 hunks)src/core/primitives/Dropdown/Select/index.tsx(1 hunks)
🧰 Additional context used
🪛 ESLint
src/core/primitives/Dropdown/Select/index.tsx
[error] 11-13: Block must not be padded by blank lines.
(padded-blocks)
[error] 34-36: Block must not be padded by blank lines.
(padded-blocks)
[error] 38-38: Newline required at end of file but not found.
(eol-last)
src/core/primitives/Dropdown/Select/SelectPrimitive.stories.tsx
[error] 21-21: Unexpected trailing comma.
(comma-dangle)
[error] 23-23: Newline required at end of file but not found.
(eol-last)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
| {isOpen && options.map((option, index) => ( | ||
| <div key={index} onClick={() => handleSelect(option)}>{option}</div> | ||
| ))} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add styling for dropdown positioning.
The dropdown options are rendered without any positioning, which could lead to layout issues. Consider adding proper styling:
- {isOpen && options.map((option, index) => (
+ {isOpen && (
+ <div className="absolute top-full left-0 w-full mt-1 bg-white border rounded-md shadow-lg">
+ {options.map((option, index) => (
<div key={index} onClick={() => handleSelect(option)}>{option}</div>
+ ))}
+ </div>
))}📝 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.
| {isOpen && options.map((option, index) => ( | |
| <div key={index} onClick={() => handleSelect(option)}>{option}</div> | |
| ))} | |
| {isOpen && ( | |
| <div className="absolute top-full left-0 w-full mt-1 bg-white border rounded-md shadow-lg"> | |
| {options.map((option, index) => ( | |
| <div key={index} onClick={() => handleSelect(option)}>{option}</div> | |
| ))} | |
| </div> | |
| )} |
| const SelectPrimitive = ({ children, options, value, onChange, ...props }:SelectPrimitiveProps) => { | ||
|
|
||
| const [isOpen, setIsOpen] = useState(false); | ||
| const [selected, setSelected] = useState(value ?? null) | ||
|
|
||
| const handleClick = () => { | ||
| setIsOpen(!isOpen) | ||
| } | ||
|
|
||
| const handleSelect = (option: string) => { | ||
| setSelected(option) | ||
| onChange?.(option) | ||
| setIsOpen(false) | ||
| } | ||
| return ( | ||
| <> | ||
| <Primitive.button onClick={handleClick} {...props}> | ||
| {children} | ||
| </Primitive.button> | ||
| {isOpen && options.map((option, index) => ( | ||
| <div key={index} onClick={() => handleSelect(option)}>{option}</div> | ||
| ))} | ||
| </> | ||
| ); | ||
|
|
||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add accessibility and keyboard navigation support.
The current implementation lacks essential features for accessibility and usability:
- Missing ARIA attributes (aria-expanded, aria-haspopup)
- Missing keyboard navigation (arrow keys, Enter, Escape)
- Missing focus management
- Missing dropdown positioning (might overlap with other content)
Here's a suggested implementation:
const SelectPrimitive = ({ children, options, value, onChange, ...props }:SelectPrimitiveProps) => {
+ const dropdownRef = useRef<HTMLDivElement>(null);
const [isOpen, setIsOpen] = useState(false);
const [selected, setSelected] = useState(value ?? null)
+ const [focusedIndex, setFocusedIndex] = useState(-1);
const handleClick = () => {
setIsOpen(!isOpen)
}
const handleSelect = (option: string) => {
setSelected(option)
onChange?.(option)
setIsOpen(false)
}
+
+ const handleKeyDown = (e: React.KeyboardEvent) => {
+ switch(e.key) {
+ case 'ArrowDown':
+ e.preventDefault();
+ if (!isOpen) {
+ setIsOpen(true);
+ }
+ setFocusedIndex(prev => Math.min(prev + 1, options.length - 1));
+ break;
+ case 'ArrowUp':
+ e.preventDefault();
+ setFocusedIndex(prev => Math.max(prev - 1, 0));
+ break;
+ case 'Enter':
+ e.preventDefault();
+ if (focusedIndex >= 0) {
+ handleSelect(options[focusedIndex]);
+ }
+ break;
+ case 'Escape':
+ e.preventDefault();
+ setIsOpen(false);
+ break;
+ }
+ }
+
+ useEffect(() => {
+ const handleClickOutside = (event: MouseEvent) => {
+ if (dropdownRef.current && !dropdownRef.current.contains(event.target as Node)) {
+ setIsOpen(false);
+ }
+ };
+
+ document.addEventListener('mousedown', handleClickOutside);
+ return () => document.removeEventListener('mousedown', handleClickOutside);
+ }, []);
return (
- <>
+ <div ref={dropdownRef} className="relative">
<Primitive.button
+ aria-haspopup="listbox"
+ aria-expanded={isOpen}
onClick={handleClick}
+ onKeyDown={handleKeyDown}
{...props}
>
{children}
</Primitive.button>
{isOpen && options.map((option, index) => (
- <div key={index} onClick={() => handleSelect(option)}>{option}</div>
+ <div
+ key={index}
+ role="option"
+ aria-selected={selected === option}
+ className={`cursor-pointer ${focusedIndex === index ? 'bg-gray-100' : ''}`}
+ onClick={() => handleSelect(option)}
+ >
+ {option}
+ </div>
))}
- </>
+ </div>
);
};📝 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 SelectPrimitive = ({ children, options, value, onChange, ...props }:SelectPrimitiveProps) => { | |
| const [isOpen, setIsOpen] = useState(false); | |
| const [selected, setSelected] = useState(value ?? null) | |
| const handleClick = () => { | |
| setIsOpen(!isOpen) | |
| } | |
| const handleSelect = (option: string) => { | |
| setSelected(option) | |
| onChange?.(option) | |
| setIsOpen(false) | |
| } | |
| return ( | |
| <> | |
| <Primitive.button onClick={handleClick} {...props}> | |
| {children} | |
| </Primitive.button> | |
| {isOpen && options.map((option, index) => ( | |
| <div key={index} onClick={() => handleSelect(option)}>{option}</div> | |
| ))} | |
| </> | |
| ); | |
| }; | |
| const SelectPrimitive = ({ children, options, value, onChange, ...props }: SelectPrimitiveProps) => { | |
| const dropdownRef = useRef<HTMLDivElement>(null); | |
| const [isOpen, setIsOpen] = useState(false); | |
| const [selected, setSelected] = useState(value ?? null); | |
| const [focusedIndex, setFocusedIndex] = useState(-1); | |
| const handleClick = () => { | |
| setIsOpen(!isOpen); | |
| }; | |
| const handleSelect = (option: string) => { | |
| setSelected(option); | |
| onChange?.(option); | |
| setIsOpen(false); | |
| }; | |
| const handleKeyDown = (e: React.KeyboardEvent) => { | |
| switch (e.key) { | |
| case 'ArrowDown': | |
| e.preventDefault(); | |
| if (!isOpen) { | |
| setIsOpen(true); | |
| } | |
| setFocusedIndex(prev => Math.min(prev + 1, options.length - 1)); | |
| break; | |
| case 'ArrowUp': | |
| e.preventDefault(); | |
| setFocusedIndex(prev => Math.max(prev - 1, 0)); | |
| break; | |
| case 'Enter': | |
| e.preventDefault(); | |
| if (focusedIndex >= 0) { | |
| handleSelect(options[focusedIndex]); | |
| } | |
| break; | |
| case 'Escape': | |
| e.preventDefault(); | |
| setIsOpen(false); | |
| break; | |
| } | |
| }; | |
| useEffect(() => { | |
| const handleClickOutside = (event: MouseEvent) => { | |
| if (dropdownRef.current && !dropdownRef.current.contains(event.target as Node)) { | |
| setIsOpen(false); | |
| } | |
| }; | |
| document.addEventListener('mousedown', handleClickOutside); | |
| return () => document.removeEventListener('mousedown', handleClickOutside); | |
| }, []); | |
| return ( | |
| <div ref={dropdownRef} className="relative"> | |
| <Primitive.button | |
| aria-haspopup="listbox" | |
| aria-expanded={isOpen} | |
| onClick={handleClick} | |
| onKeyDown={handleKeyDown} | |
| {...props} | |
| > | |
| {children} | |
| </Primitive.button> | |
| {isOpen && options.map((option, index) => ( | |
| <div | |
| key={index} | |
| role="option" | |
| aria-selected={selected === option} | |
| className={`cursor-pointer ${focusedIndex === index ? 'bg-gray-100' : ''}`} | |
| onClick={() => handleSelect(option)} | |
| > | |
| {option} | |
| </div> | |
| ))} | |
| </div> | |
| ); | |
| }; |
🧰 Tools
🪛 ESLint
[error] 11-13: Block must not be padded by blank lines.
(padded-blocks)
[error] 34-36: Block must not be padded by blank lines.
(padded-blocks)
|
👍 for the effort lets work on a better primitive |
Summary by CodeRabbit