feat(Dropdown): Dropdown 컴포넌트 구현 #29
Conversation
|
Important Installation incomplete: to start using Gemini Code Assist, please ask the organization owner(s) to visit the Gemini Code Assist Admin Console and sign the Terms of Services. |
📝 WalkthroughWalkthroughA new self-contained Dropdown component is introduced with a composite pattern architecture. It provides Provider, Container, Trigger, List, and Item sub-components with dual-context state management, keyboard navigation support (ArrowUp/Down, Enter, Escape), and comprehensive styling with tone variants. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Trigger as DropdownTrigger
participant Provider as DropdownProvider<br/>(Context)
participant List as DropdownList
participant Item as DropdownItem
User->>Trigger: Click to toggle
Trigger->>Provider: toggle() dispatch
Provider->>Provider: setIsOpen(true)
Provider->>List: Render items (isOpen=true)
rect rgb(200, 220, 240)
Note over User,Item: Keyboard Navigation
User->>Trigger: ArrowDown
Trigger->>Provider: handleKeyDown(ArrowDown)
Provider->>Provider: focusIndex = (current + 1) % length
Provider->>List: Re-render with focusedIndex
List->>Item: Highlight focused item & scroll into view
end
rect rgb(220, 240, 200)
Note over User,Item: Selection
User->>Item: Click or press Enter
Item->>Provider: selectIndex(index) dispatch
Provider->>Provider: selectedIndex = index, setIsOpen(false)
Provider->>Trigger: Update display with selected value
List->>List: Close (isOpen=false)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ 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. Comment |
- 컴파운드 컴포넌트 패턴으로 Dropdown 구현 - Provider, Container, Trigger, List, Item 서브 컴포넌트 - 키보드 네비게이션 지원 (ArrowUp/Down, Enter, Escape) - 다양한 tone 스타일 지원
dydals3440
force-pushed
the
feature/dropdown-component
branch
from
f683be5 to
74512a3
Compare
There was a problem hiding this comment.
Pull request overview
This PR implements a Dropdown component using the compound component pattern with Context API for state management. The implementation includes keyboard navigation support (ArrowUp/Down, Enter, Escape) and basic accessibility features with ARIA attributes. The component supports multiple tone styles (brand, neutral, danger, warning, success, info) and provides a flexible API through Provider, Container, Trigger, List, and Item sub-components.
- Compound component pattern with Context API-based state management for dropdown behavior
- Keyboard navigation with circular list traversal and accessibility attributes
- Flexible styling system with CVA variants supporting multiple tones
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // 더 돌지않는것 | ||
| // Math.max(prev - 1, 0); | ||
|
|
||
| // 뺑글뺑글 도는것 캐러젤 |
There was a problem hiding this comment.
The comment contains a typo. "뺑글뺑글 도는것 캐러젤" should be "뺑글뺑글 도는 것 캐러셀" (with spaces and corrected spelling of "carousel" in Korean).
| // 뺑글뺑글 도는것 캐러젤 | |
| // 뺑글뺑글 도는 것 캐러셀 |
| Enter: (_event, { selectIndex, focusedIndex }) => { | ||
| // 기본적으로 Enter를 누르면, Select가 열고 닫힘. Select Box에 포커스가 되어있기 때문이다. | ||
| // event.preventDefault(); |
There was a problem hiding this comment.
The Enter key handler doesn't call event.preventDefault() to prevent the default form submission behavior when the dropdown trigger is inside a form. The commented-out code and comment on line 60-61 indicate this was considered but not implemented. Without preventDefault, pressing Enter on a focused item could cause unintended form submissions.
| Enter: (_event, { selectIndex, focusedIndex }) => { | |
| // 기본적으로 Enter를 누르면, Select가 열고 닫힘. Select Box에 포커스가 되어있기 때문이다. | |
| // event.preventDefault(); | |
| Enter: (event, { selectIndex, focusedIndex }) => { | |
| // 기본적으로 Enter를 누르면, Select가 열고 닫힘. Select Box에 포커스가 되어있기 때문이다. | |
| event.preventDefault(); |
| onKeyDown={(e) => { | ||
| if (e.key === "Enter" || e.key === " ") { | ||
| selectIndex(index); | ||
| toggle(false); | ||
| } | ||
| }} |
There was a problem hiding this comment.
The keyboard handler in DropdownItem only handles Enter and Space keys, but doesn't prevent the default behavior for Space key. This will cause the page to scroll when Space is pressed on a focused dropdown item, which is inconsistent with expected dropdown behavior.
| Enter: (_event, { selectIndex, focusedIndex }) => { | ||
| // 기본적으로 Enter를 누르면, Select가 열고 닫힘. Select Box에 포커스가 되어있기 때문이다. | ||
| // event.preventDefault(); | ||
| selectIndex(focusedIndex); |
There was a problem hiding this comment.
When a user selects an item by pressing Enter (through the KeyEventMap handler on line 59-63), the dropdown doesn't close automatically. The selectIndex is called but toggle is not, leaving the dropdown open after selection. This is inconsistent with the click behavior (lines 193-196) where the dropdown closes after selection.
| Enter: (_event, { selectIndex, focusedIndex }) => { | |
| // 기본적으로 Enter를 누르면, Select가 열고 닫힘. Select Box에 포커스가 되어있기 때문이다. | |
| // event.preventDefault(); | |
| selectIndex(focusedIndex); | |
| Enter: (_event, { selectIndex, focusedIndex, toggle }) => { | |
| // 기본적으로 Enter를 누르면, Select가 열고 닫힘. Select Box에 포커스가 되어있기 때문이다. | |
| // event.preventDefault(); | |
| selectIndex(focusedIndex); | |
| toggle(false); |
| Container: DropdownContainer, | ||
| Trigger: DropdownTrigger, | ||
| List: DropdownList, | ||
| Item: DropdownItem, |
There was a problem hiding this comment.
The DropdownItem component is exported but not intended for external use based on the compound component pattern. It's only used internally by DropdownList and doesn't have proper standalone functionality since it requires specific props like itemsRef that consumers shouldn't need to manage.
| Item: DropdownItem, |
| const KeyEventMap: Partial<Record<KeyboardEvent["key"], KeyEventHandler>> = { | ||
| ArrowUp: (_event, { focusIndex, items }) => { | ||
| focusIndex((prev) => { | ||
| // 더 돌지않는것 |
There was a problem hiding this comment.
The comment contains a typo and unclear grammar. "더 돌지않는것" should be "더 돌지 않는 것" (with spaces).
| // 더 돌지않는것 | |
| // 더 돌지 않는 것 |
| ArrowUp: (_event, { focusIndex, items }) => { | ||
| focusIndex((prev) => { | ||
| // 더 돌지않는것 | ||
| // Math.max(prev - 1, 0); | ||
|
|
||
| // 뺑글뺑글 도는것 캐러젤 | ||
| return (items.length + prev - 1) % items.length; | ||
| }); | ||
| }, | ||
| ArrowDown: (_event, { focusIndex, items }) => { | ||
| // focusIndex((prev) => Math.min(prev + 1, items.length - 1)); | ||
| focusIndex((prev) => (items.length + prev + 1) % items.length); | ||
| }, |
There was a problem hiding this comment.
The ArrowUp and ArrowDown key handlers don't call event.preventDefault(), which means the page will scroll when using arrow keys to navigate the dropdown. This creates a poor user experience as the viewport moves while trying to navigate dropdown options.
| <button | ||
| className={triggerVariants({ tone })} | ||
| type="button" | ||
| onClick={() => toggle()} | ||
| onKeyDown={handleKeyDown} | ||
| aria-expanded={isOpen} | ||
| aria-haspopup="listbox" | ||
| > | ||
| <span className={triggerTextStyle}> | ||
| {selectedItem?.text || placeholder} | ||
| </span> | ||
| <ChevronDown | ||
| size={16} | ||
| className={css({ | ||
| transition: "0.2s", | ||
| transform: isOpen ? "rotate(180deg)" : "rotate(0deg)", | ||
| })} | ||
| /> | ||
| </button> | ||
| ); |
There was a problem hiding this comment.
The dropdown trigger button is missing proper keyboard navigation support. When the dropdown is opened via keyboard (Space or Enter), the focus should move to the first item or the selected item in the list. Currently, focus remains on the trigger button, making it impossible to navigate the dropdown items with keyboard alone without tabbing out and back in.
| <button | ||
| className={triggerVariants({ tone })} | ||
| type="button" | ||
| onClick={() => toggle()} | ||
| onKeyDown={handleKeyDown} | ||
| aria-expanded={isOpen} | ||
| aria-haspopup="listbox" |
There was a problem hiding this comment.
The dropdown list is missing required ARIA attributes for a combobox pattern. The listbox should have an id that's referenced by the trigger's aria-controls attribute, and the trigger should have role="combobox" instead of relying solely on aria-haspopup="listbox". This helps screen readers properly announce the relationship between trigger and list.
| const DropdownList = () => { | ||
| const { items, isOpen, focusedIndex } = useDropdown(); | ||
| const itemsRef = useRef<(HTMLDivElement | null)[]>([]); | ||
|
|
There was a problem hiding this comment.
The itemsRef initialization and management could lead to memory leaks. The ref array grows but is never cleaned up when items are removed or the component unmounts. Consider initializing itemsRef with the items length or implementing proper cleanup.
| // items 배열의 길이에 맞춰 ref 배열 길이도 조정 | |
| useEffect(() => { | |
| if (!itemsRef.current) return; | |
| // 배열 길이를 items.length에 맞게 잘라서 오래된 ref를 정리 | |
| itemsRef.current.length = items.length; | |
| }, [items.length]); | |
| // 컴포넌트 언마운트 시 ref 배열을 정리 | |
| useEffect(() => { | |
| return () => { | |
| if (itemsRef.current) { | |
| itemsRef.current.length = 0; | |
| } | |
| }; | |
| }, []); |
![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/components/Dropdown/Dropdown.tsx (5)
16-19: Consider exportingDropdownItemPropsfor consumer convenience.Consumers of this component need to provide items with
idandtextproperties. Exporting this type would help them type their data correctly.-interface DropdownItemProps { +export interface DropdownItemProps { id: string; text: string; }
84-86: Consider throwing an error when hooks are used outside Provider.Using no-op defaults silently fails if components are used outside
Dropdown.Provider. Throwing an error provides better developer experience.🔎 Suggested pattern
const DropdownContext = createContext<DropdownProps | null>(null); const useDropdown = () => { const context = useContext(DropdownContext); if (!context) { throw new Error("useDropdown must be used within Dropdown.Provider"); } return context; };
97-99: InitializefocusedIndexwhen dropdown opens.When the dropdown opens,
focusedIndexremains at -1, so the first ArrowDown press goes to index 0, which is correct. However, if an item is already selected, it would be more intuitive to start focus from the selected item.🔎 Proposed enhancement
const toggle = (force?: boolean) => { - setIsOpen((prev) => (typeof force === "boolean" ? force : !prev)); + setIsOpen((prev) => { + const next = typeof force === "boolean" ? force : !prev; + if (next) { + // Initialize focus to selected item or first item when opening + focusIndex(selectedIndex >= 0 ? selectedIndex : 0); + } + return next; + }); };
146-166: Good accessibility implementation; consider addingaria-controls.The trigger has good ARIA attributes (
aria-expanded,aria-haspopup="listbox"). For complete accessibility, consider linking the trigger to the list witharia-controlsusing a shared ID.
236-242: Consider whetherItemshould be exposed in the public API.
DropdownItemis used internally byDropdownListand requiresitemsRefprop that's managed internally. Exposing it might confuse consumers since they typically shouldn't use it directly. If it's intended for advanced use cases, consider documenting this.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Dropdown/Dropdown.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
src/components/Dropdown/Dropdown.tsx (1)
src/tokens/colors.ts (1)
Tone(22-28)
🪛 Biome (2.1.2)
src/components/Dropdown/Dropdown.tsx
[error] 191-191: The elements with this role can be changed to the following elements:
For examples and more information, see WAI-ARIA Roles
(lint/a11y/useSemanticElements)
[error] 229-230: The elements with this role can be changed to the following elements:
For examples and more information, see WAI-ARIA Roles
(lint/a11y/useSemanticElements)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
GitHub Check: build-chromatic
🔇 Additional comments (4)
src/components/Dropdown/Dropdown.tsx (4)
129-131: LGTM!
Simple container wrapper with appropriate positioning style.
184-206: Implementation is correct; static analysis hint can be safely ignored.
The Biome hint about using <option> is a false positive here. Native <option> elements can only exist inside <select> and have very limited styling capabilities. Using <div role="option"> is the standard accessible pattern for custom dropdown implementations.
Good handling of both click and keyboard (Enter/Space) interactions with proper aria-selected attribute.
209-234: Good implementation; static analysis hint about <select> can be ignored.
The Biome suggestion to use <select> is not applicable for custom styled dropdowns. Using <div role="listbox"> is the appropriate accessible pattern that allows full styling control.
The useEffect for scrolling the focused item into view is a nice touch for long lists.
244-361: LGTM!
Styles are well-organized with:
Proper relative/absolute positioning for dropdown behavior
Complete coverage of all Tone variants
Clear visual states for hover, focus-visible, selected, and disabled
Max-height with overflow for long lists
| const KeyEventMap: Partial<Record<KeyboardEvent["key"], KeyEventHandler>> = { | ||
| ArrowUp: (_event, { focusIndex, items }) => { | ||
| focusIndex((prev) => { | ||
| // 더 돌지않는것 | ||
| // Math.max(prev - 1, 0); | ||
|
|
||
| // 뺑글뺑글 도는것 캐러젤 | ||
| return (items.length + prev - 1) % items.length; | ||
| }); | ||
| }, | ||
| ArrowDown: (_event, { focusIndex, items }) => { | ||
| // focusIndex((prev) => Math.min(prev + 1, items.length - 1)); | ||
| focusIndex((prev) => (items.length + prev + 1) % items.length); | ||
| }, | ||
| Enter: (_event, { selectIndex, focusedIndex }) => { | ||
| // 기본적으로 Enter를 누르면, Select가 열고 닫힘. Select Box에 포커스가 되어있기 때문이다. | ||
| // event.preventDefault(); | ||
| selectIndex(focusedIndex); | ||
| }, | ||
| Escape: (_event, { toggle }) => { | ||
| toggle(false); | ||
| }, | ||
| }; |
There was a problem hiding this comment.
Enter key should close dropdown and prevent default; add guard for empty items.
Several issues with the keyboard handlers:
- Inconsistent behavior: Enter selects but doesn't close the dropdown, unlike click in
DropdownItem(lines 193-196). - Division by zero: If
items.lengthis 0, the modulo operation returnsNaN. - Missing
event.preventDefault(): ArrowUp/ArrowDown may scroll the page; Enter may trigger form submission.
🔎 Proposed fix
const KeyEventMap: Partial<Record<KeyboardEvent["key"], KeyEventHandler>> = {
ArrowUp: (event, { focusIndex, items }) => {
+ if (items.length === 0) return;
+ event.preventDefault();
focusIndex((prev) => {
- // 더 돌지않는것
- // Math.max(prev - 1, 0);
-
- // 뺑글뺑글 도는것 캐러젤
return (items.length + prev - 1) % items.length;
});
},
ArrowDown: (event, { focusIndex, items }) => {
- // focusIndex((prev) => Math.min(prev + 1, items.length - 1));
+ if (items.length === 0) return;
+ event.preventDefault();
focusIndex((prev) => (items.length + prev + 1) % items.length);
},
- Enter: (_event, { selectIndex, focusedIndex }) => {
- // 기본적으로 Enter를 누르면, Select가 열고 닫힘. Select Box에 포커스가 되어있기 때문이다.
- // event.preventDefault();
- selectIndex(focusedIndex);
+ Enter: (event, { selectIndex, focusedIndex, toggle }) => {
+ event.preventDefault();
+ if (focusedIndex >= 0) {
+ selectIndex(focusedIndex);
+ toggle(false);
+ }
},
Escape: (_event, { toggle }) => {
toggle(false);
},
};📝 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 KeyEventMap: Partial<Record<KeyboardEvent["key"], KeyEventHandler>> = { | |
| ArrowUp: (_event, { focusIndex, items }) => { | |
| focusIndex((prev) => { | |
| // 더 돌지않는것 | |
| // Math.max(prev - 1, 0); | |
| // 뺑글뺑글 도는것 캐러젤 | |
| return (items.length + prev - 1) % items.length; | |
| }); | |
| }, | |
| ArrowDown: (_event, { focusIndex, items }) => { | |
| // focusIndex((prev) => Math.min(prev + 1, items.length - 1)); | |
| focusIndex((prev) => (items.length + prev + 1) % items.length); | |
| }, | |
| Enter: (_event, { selectIndex, focusedIndex }) => { | |
| // 기본적으로 Enter를 누르면, Select가 열고 닫힘. Select Box에 포커스가 되어있기 때문이다. | |
| // event.preventDefault(); | |
| selectIndex(focusedIndex); | |
| }, | |
| Escape: (_event, { toggle }) => { | |
| toggle(false); | |
| }, | |
| }; | |
| const KeyEventMap: Partial<Record<KeyboardEvent["key"], KeyEventHandler>> = { | |
| ArrowUp: (event, { focusIndex, items }) => { | |
| if (items.length === 0) return; | |
| event.preventDefault(); | |
| focusIndex((prev) => { | |
| return (items.length + prev - 1) % items.length; | |
| }); | |
| }, | |
| ArrowDown: (event, { focusIndex, items }) => { | |
| if (items.length === 0) return; | |
| event.preventDefault(); | |
| focusIndex((prev) => (items.length + prev + 1) % items.length); | |
| }, | |
| Enter: (event, { selectIndex, focusedIndex, toggle }) => { | |
| event.preventDefault(); | |
| if (focusedIndex >= 0) { | |
| selectIndex(focusedIndex); | |
| toggle(false); | |
| } | |
| }, | |
| Escape: (_event, { toggle }) => { | |
| toggle(false); | |
| }, | |
| }; |
🤖 Prompt for AI Agents
In src/components/Dropdown/Dropdown.tsx around lines 45-67, update the keyboard
handlers to (1) guard against empty items for ArrowUp/ArrowDown by returning
early when items.length === 0 to avoid modulo/NaN, (2) call
event.preventDefault() in ArrowUp, ArrowDown and Enter to stop page scrolling
and form submits, and (3) make Enter also close the dropdown after selecting by
using the provided toggle function (add toggle to the Enter handler params) and
only call selectIndex when focusedIndex is valid and items.length > 0.
| const DropdownContextProvider = ({ | ||
| children, | ||
| defaultItems, | ||
| }: PropsWithChildren<{ defaultItems: DropdownItemProps[] }>) => { | ||
| const [items, setItems] = useState<DropdownItemProps[]>(defaultItems); | ||
| const [isOpen, setIsOpen] = useState(false); | ||
| const [focusedIndex, focusIndex] = useState(-1); | ||
| const [selectedIndex, selectIndex] = useState(-1); | ||
|
|
||
| const toggle = (force?: boolean) => { | ||
| setIsOpen((prev) => (typeof force === "boolean" ? force : !prev)); | ||
| }; | ||
|
|
||
| const handleKeyDown = (event: React.KeyboardEvent) => { | ||
| const { key } = event; | ||
| const handler = KeyEventMap[key]; | ||
|
|
||
| if (handler) { | ||
| handler(event, { | ||
| focusedIndex, | ||
| focusIndex, | ||
| selectIndex, | ||
| toggle, | ||
| items, | ||
| }); | ||
| } | ||
| }; | ||
|
|
||
| return ( | ||
| <DropdownContext.Provider | ||
| value={{ items, isOpen, focusedIndex, selectedIndex }} | ||
| > | ||
| <DropdownDispatchContext.Provider | ||
| value={{ setItems, toggle, focusIndex, selectIndex, handleKeyDown }} | ||
| > | ||
| {children} | ||
| </DropdownDispatchContext.Provider> | ||
| </DropdownContext.Provider> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
Missing click-outside handler to close dropdown.
Users expect dropdowns to close when clicking outside. Consider adding an effect to handle click-outside events.
🔎 Example implementation
// In DropdownContainer or DropdownContextProvider
const containerRef = useRef<HTMLDivElement>(null);
useEffect(() => {
if (!isOpen) return;
const handleClickOutside = (event: MouseEvent) => {
if (containerRef.current && !containerRef.current.contains(event.target as Node)) {
toggle(false);
}
};
document.addEventListener("mousedown", handleClickOutside);
return () => document.removeEventListener("mousedown", handleClickOutside);
}, [isOpen]);🤖 Prompt for AI Agents
In src/components/Dropdown/Dropdown.tsx around lines 88–127, the provider
doesn't close the dropdown when the user clicks outside; add a containerRef
(useRef<HTMLDivElement | null>) and a useEffect that, when isOpen is true,
registers a mousedown (or click) listener on document which checks if
event.target is outside containerRef.current and calls toggle(false) if so, and
removes the listener in cleanup. Attach the ref to a wrapper element that
encloses the children (or expose the ref via context if the container lives
elsewhere), ensure proper typing/casting of event.target to Node, and include
isOpen and toggle in the effect dependencies so the listener is only active
while open.
1 participant
Summary
구현 내용
향후 계획
Closes
Closes #28
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.