[Feat] #62 - 공통 Lists & Tables 컴포넌트 추가#72
Conversation
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
📝 WalkthroughWalkthroughAdds new shared UI list components ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/shared/ui/lists/lists.stories.tsx (1)
169-172: Document the row click vs checkbox selection behavior.
handleRowClickimplements single-selection (deselects others), whilehandleRowCheckallows multi-selection via checkboxes. This behavioral difference is valid but could confuse users in the Storybook demo. Consider adding a comment or controls to clarify this is intentional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shared/ui/lists/lists.stories.tsx` around lines 169 - 172, handleRowClick currently implements single-selection (it sets isSelected only for the clicked id) while handleRowCheck supports multi-selection via checkboxes; make this explicit in the story by adding a concise comment near handleRowClick and/or a Storybook control/description that states "Row click = single-select, Checkbox = multi-select" so viewers understand the intentional difference between handleRowClick and handleRowCheck behavior.src/shared/ui/lists/lists.tsx (3)
74-79: Index signature causes type widening forisSelectedandisDisabled.The index signature
[key: string]: React.ReactNodecausesisSelectedandisDisabledto be typed asReact.ReactNodeinstead ofboolean | undefined, requiring the!!coercion at runtime (lines 309-310). Consider using an intersection type or restructuring to preserve type safety.♻️ Optional refactor for better type safety
-export type TableRowData = { - id: string; - [key: string]: React.ReactNode; - isSelected?: boolean; - isDisabled?: boolean; -}; +export type TableRowData = { + id: string; + isSelected?: boolean; + isDisabled?: boolean; + cells: Record<string, React.ReactNode>; +};This requires updating cell access from
row[col.id]torow.cells[col.id].🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shared/ui/lists/lists.tsx` around lines 74 - 79, The TableRowData index signature makes isSelected/isDisabled widen to React.ReactNode; change the model so boolean fields keep their types by removing the broad `[key: string]: React.ReactNode` and introduce a separate map for dynamic cell values (e.g., add a `cells: Record<string, React.ReactNode>` or use an intersection type) and update usages that access dynamic values from `row[col.id]` to `row.cells[col.id]`; ensure `isSelected` and `isDisabled` remain typed as `boolean | undefined` and remove the runtime `!!` coercions where used.
327-340: Apply same checkbox pattern improvement here.For consistency with
ListItem, consider using nativeonChangeon the checkbox input instead of wrapper click handling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shared/ui/lists/lists.tsx` around lines 327 - 340, The checkbox wrapper currently uses the div's onClick (handleCheckClick) while the input is readOnly—change to the same pattern used in ListItem: remove the div onClick, make the input handle interaction via an onChange handler that calls handleCheckClick (or rename to handleCheckboxChange), keep checked={isSelected} and disabled={isDisabled}, and remove readOnly so the native checkbox onChange fires; ensure accessibility by keeping the same classNames and aria attributes if present.
184-197: Consider using native checkboxonChangeinstead of wrapper click.The current pattern uses
readOnlyinput with click handling on the wrapper div. This works but bypasses native checkbox keyboard interaction. UsingonChangeon the input directly would provide better a11y support.♻️ Proposed refactor for native checkbox behavior
{hasCheckbox && ( - <div - className="flex items-center justify-center w-[20px] h-[20px] flex-shrink-0" - onClick={handleCheckClick} - > + <div className="flex items-center justify-center w-[20px] h-[20px] flex-shrink-0"> <input type="checkbox" checked={isChecked} - readOnly + onChange={(e) => { + e.stopPropagation(); + if (!isDisabled && onCheck) onCheck(id, e.target.checked); + }} disabled={isDisabled} className="w-4 h-4 cursor-pointer accent-[color:var(--color-primary-800,`#004A9C`)]" /> </div> )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shared/ui/lists/lists.tsx` around lines 184 - 197, The checkbox wrapper currently handles clicks via the div onClick (using readOnly on the input), which breaks native keyboard/a11y behavior; update the JSX in src/shared/ui/lists/lists.tsx to attach the handler to the input's onChange instead of the wrapper div (move or adapt handleCheckClick to accept the ChangeEvent and call it from the input's onChange), remove readOnly from the input so it behaves as a native controlled checkbox (keep checked={isChecked} and disabled={isDisabled}), and remove the wrapper div's onClick so keyboard focus/space toggling and screen reader interaction are preserved for the checkbox.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/shared/ui/lists/lists.tsx`:
- Around line 181-182: The clickable list container returned by the component
uses a plain <div> with onClick (see getListItemClasses and handleItemClick) but
lacks keyboard accessibility; add role="button", tabIndex={0} and an onKeyDown
handler that triggers the same activation logic as handleItemClick when Enter or
Space is pressed, and set aria-disabled={isDisabled} (and ensure the handler
no-ops when isDisabled is true) so keyboard and assistive tech users can
interact correctly.
- Around line 321-326: The table row div returned in lists.tsx is not keyboard
accessible; update the element keyed by row.id (the block using
getTableRowClasses(isSelected, isDisabled) and handleRowClick) to behave like a
button by adding role="button", tabIndex={isDisabled ? -1 : 0}, and
aria-disabled={isDisabled}, and implement an onKeyDown handler that triggers
handleRowClick when Enter or Space is pressed (respecting isDisabled) while
keeping the existing onClick; this mirrors the accessibility pattern used for
ListItem.
---
Nitpick comments:
In `@src/shared/ui/lists/lists.stories.tsx`:
- Around line 169-172: handleRowClick currently implements single-selection (it
sets isSelected only for the clicked id) while handleRowCheck supports
multi-selection via checkboxes; make this explicit in the story by adding a
concise comment near handleRowClick and/or a Storybook control/description that
states "Row click = single-select, Checkbox = multi-select" so viewers
understand the intentional difference between handleRowClick and handleRowCheck
behavior.
In `@src/shared/ui/lists/lists.tsx`:
- Around line 74-79: The TableRowData index signature makes
isSelected/isDisabled widen to React.ReactNode; change the model so boolean
fields keep their types by removing the broad `[key: string]: React.ReactNode`
and introduce a separate map for dynamic cell values (e.g., add a `cells:
Record<string, React.ReactNode>` or use an intersection type) and update usages
that access dynamic values from `row[col.id]` to `row.cells[col.id]`; ensure
`isSelected` and `isDisabled` remain typed as `boolean | undefined` and remove
the runtime `!!` coercions where used.
- Around line 327-340: The checkbox wrapper currently uses the div's onClick
(handleCheckClick) while the input is readOnly—change to the same pattern used
in ListItem: remove the div onClick, make the input handle interaction via an
onChange handler that calls handleCheckClick (or rename to
handleCheckboxChange), keep checked={isSelected} and disabled={isDisabled}, and
remove readOnly so the native checkbox onChange fires; ensure accessibility by
keeping the same classNames and aria attributes if present.
- Around line 184-197: The checkbox wrapper currently handles clicks via the div
onClick (using readOnly on the input), which breaks native keyboard/a11y
behavior; update the JSX in src/shared/ui/lists/lists.tsx to attach the handler
to the input's onChange instead of the wrapper div (move or adapt
handleCheckClick to accept the ChangeEvent and call it from the input's
onChange), remove readOnly from the input so it behaves as a native controlled
checkbox (keep checked={isChecked} and disabled={isDisabled}), and remove the
wrapper div's onClick so keyboard focus/space toggling and screen reader
interaction are preserved for the checkbox.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fdfd1c8f-3a4e-4e10-b4f8-b09c71a73fca
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (2)
src/shared/ui/lists/lists.stories.tsxsrc/shared/ui/lists/lists.tsx
| return ( | ||
| <div className={getListItemClasses(isActive, isDisabled)} onClick={handleItemClick}> |
There was a problem hiding this comment.
Add keyboard accessibility to clickable container.
The clickable <div> lacks role, tabIndex, and keyboard event handlers, making it inaccessible to keyboard users and screen readers. This can be an accessibility blocker per WCAG 2.1.
♿ Proposed fix for keyboard accessibility
return (
<div
+ role="button"
+ tabIndex={isDisabled ? -1 : 0}
+ onKeyDown={(e) => {
+ if (!isDisabled && (e.key === "Enter" || e.key === " ")) {
+ e.preventDefault();
+ onClick?.(id);
+ }
+ }}
className={getListItemClasses(isActive, isDisabled)}
onClick={handleItemClick}
+ aria-disabled={isDisabled}
>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/shared/ui/lists/lists.tsx` around lines 181 - 182, The clickable list
container returned by the component uses a plain <div> with onClick (see
getListItemClasses and handleItemClick) but lacks keyboard accessibility; add
role="button", tabIndex={0} and an onKeyDown handler that triggers the same
activation logic as handleItemClick when Enter or Space is pressed, and set
aria-disabled={isDisabled} (and ensure the handler no-ops when isDisabled is
true) so keyboard and assistive tech users can interact correctly.
| return ( | ||
| <div | ||
| key={row.id} | ||
| className={getTableRowClasses(isSelected, isDisabled)} | ||
| onClick={handleRowClick} | ||
| > |
There was a problem hiding this comment.
Add keyboard accessibility to clickable table rows.
Same accessibility issue as ListItem: clickable rows need role, tabIndex, and keyboard handlers for keyboard navigation.
♿ Proposed fix for table row accessibility
return (
<div
key={row.id}
className={getTableRowClasses(isSelected, isDisabled)}
onClick={handleRowClick}
+ role="button"
+ tabIndex={isDisabled ? -1 : 0}
+ onKeyDown={(e) => {
+ if (!isDisabled && (e.key === "Enter" || e.key === " ")) {
+ e.preventDefault();
+ onRowClick?.(row.id);
+ }
+ }}
+ aria-disabled={isDisabled}
>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/shared/ui/lists/lists.tsx` around lines 321 - 326, The table row div
returned in lists.tsx is not keyboard accessible; update the element keyed by
row.id (the block using getTableRowClasses(isSelected, isDisabled) and
handleRowClick) to behave like a button by adding role="button",
tabIndex={isDisabled ? -1 : 0}, and aria-disabled={isDisabled}, and implement an
onKeyDown handler that triggers handleRowClick when Enter or Space is pressed
(respecting isDisabled) while keeping the existing onClick; this mirrors the
accessibility pattern used for ListItem.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/shared/ui/icons/index.tsx (2)
378-558: Consider extracting shared SVG boilerplate into a base icon wrapper.The same
svgscaffolding is repeated across all new icons. A tinyBaseIconcomponent would reduce duplication and make future icon additions less error-prone.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shared/ui/icons/index.tsx` around lines 378 - 558, The SVG scaffold is duplicated across InboxIcon, UserSolidIcon, FileTextIcon, BellIcon, AlertCircleIcon and ClockIcon; create a small BaseIcon component that accepts size, children and additional SVG props and centralizes width/height/viewBox/fill/xmlns and {...props}, then refactor each icon to return <BaseIcon size={size} {...props}>...paths...</BaseIcon> (move the path elements into the children) so all common attributes live in BaseIcon and individual icon functions only supply unique path elements.
380-558: Add default decorative-icon accessibility behavior.These SVGs should default to being hidden from assistive tech unless an accessible label is explicitly provided. That avoids noisy screen reader output in common decorative usage.
♿ Suggested pattern to apply to each new icon
-export const InboxIcon: React.FC<IconProps> = ({ size = 24, ...props }) => ( - <svg +export const InboxIcon: React.FC<IconProps> = ({ size = 24, ...props }) => { + const isAccessible = Boolean(props["aria-label"] || props["aria-labelledby"]); + return ( + <svg width={size} height={size} viewBox="0 0 24 24" fill="none" xmlns="http://www.w3.org/2000/svg" + aria-hidden={isAccessible ? undefined : true} + focusable={isAccessible ? undefined : "false"} + role={isAccessible ? "img" : undefined} {...props} > @@ - </svg> -); + </svg> + ); +};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shared/ui/icons/index.tsx` around lines 380 - 558, Update each icon component (e.g., UserSolidIcon, FileTextIcon, BellIcon, AlertCircleIcon, ClockIcon, etc.) so SVGs are hidden from assistive tech by default: set focusable="false" and add an aria-hidden default true unless an accessible label is provided (check props['aria-label'] or props['aria-labelledby'] or props.title); when a label is present expose the SVG to AT (remove aria-hidden and set role="img"). Implement this logic inside each component around the <svg> props handling (use IconProps and the component props to decide aria-hidden/role/focusable) while keeping existing prop spreading.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/shared/ui/icons/index.tsx`:
- Around line 378-558: The SVG scaffold is duplicated across InboxIcon,
UserSolidIcon, FileTextIcon, BellIcon, AlertCircleIcon and ClockIcon; create a
small BaseIcon component that accepts size, children and additional SVG props
and centralizes width/height/viewBox/fill/xmlns and {...props}, then refactor
each icon to return <BaseIcon size={size} {...props}>...paths...</BaseIcon>
(move the path elements into the children) so all common attributes live in
BaseIcon and individual icon functions only supply unique path elements.
- Around line 380-558: Update each icon component (e.g., UserSolidIcon,
FileTextIcon, BellIcon, AlertCircleIcon, ClockIcon, etc.) so SVGs are hidden
from assistive tech by default: set focusable="false" and add an aria-hidden
default true unless an accessible label is provided (check props['aria-label']
or props['aria-labelledby'] or props.title); when a label is present expose the
SVG to AT (remove aria-hidden and set role="img"). Implement this logic inside
each component around the <svg> props handling (use IconProps and the component
props to decide aria-hidden/role/focusable) while keeping existing prop
spreading.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 95ce051e-ab5e-4b40-9776-1bff3fc8dce8
📒 Files selected for processing (2)
src/shared/ui/icons/index.tsxsrc/shared/ui/lists/lists.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/shared/ui/lists/lists.tsx
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/shared/ui/lists/lists.tsx (2)
145-146:⚠️ Potential issue | 🟠 MajorAdd keyboard semantics to the clickable list item container.
ListItemis click-only on a plaindiv, so keyboard users cannot activate it. Please add button-like semantics and key handling.♿ Proposed fix
- <div className={getListItemClasses(isActive, isDisabled)} onClick={handleItemClick}> + <div + className={getListItemClasses(isActive, isDisabled)} + onClick={handleItemClick} + role="button" + tabIndex={isDisabled ? -1 : 0} + aria-disabled={isDisabled} + onKeyDown={(e) => { + if (!isDisabled && (e.key === "Enter" || e.key === " ")) { + e.preventDefault(); + handleItemClick(); + } + }} + >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shared/ui/lists/lists.tsx` around lines 145 - 146, The clickable list item currently renders a plain div (in ListItem in lists.tsx) which prevents keyboard activation; update the container to expose button-like semantics by adding role="button" and keyboard handlers: set tabIndex=0 (or tabIndex={-1} when isDisabled), add an onKeyDown that invokes handleItemClick when Enter or Space is pressed, and set aria-disabled={isDisabled}; also add an appropriate aria-pressed or aria-selected reflecting isActive. Ensure getListItemClasses, handleItemClick, isActive and isDisabled are used to gate focusability and activation.
293-297:⚠️ Potential issue | 🟠 MajorAdd keyboard semantics to clickable table rows.
Rows are interactive via mouse only. Please mirror the same keyboard-accessible pattern used for button-like controls.
♿ Proposed fix
<div key={row.id} className={getTableRowClasses(isSelected, isDisabled)} onClick={handleRowClick} + role="button" + tabIndex={isDisabled ? -1 : 0} + aria-disabled={isDisabled} + onKeyDown={(e) => { + if (!isDisabled && (e.key === "Enter" || e.key === " ")) { + e.preventDefault(); + handleRowClick(); + } + }} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shared/ui/lists/lists.tsx` around lines 293 - 297, The clickable table row currently only handles mouse clicks; update the div rendered for each row (the element using key={row.id}, className={getTableRowClasses(isSelected, isDisabled)} and onClick={handleRowClick}) to be keyboard-accessible by adding role="button", tabIndex={isDisabled ? -1 : 0}, aria-disabled={isDisabled}, and an onKeyDown handler that triggers the same action as handleRowClick when Enter or Space is pressed (and ignores input when isDisabled). Ensure the onKeyDown logic mirrors handleRowClick behavior so keyboard users get identical interaction.
🧹 Nitpick comments (1)
src/shared/ui/lists/lists.tsx (1)
91-113: Avoid pointer/hover affordance when no click handler is provided.Current class builders always imply clickability in non-disabled state, even if
onClick/onRowClickis absent. Consider coupling interactive styles to actual handler presence.♻️ Proposed refactor
-const getListItemClasses = (isActive: boolean, isDisabled: boolean): string => { +const getListItemClasses = ( + isActive: boolean, + isDisabled: boolean, + isClickable: boolean +): string => { if (isDisabled) { return [listItemBaseClasses, "bg-white opacity-50 cursor-not-allowed"].join(" "); } const stateClasses = isActive ? "bg-[color:var(--color-primary-50,`#F0F6FD`)] border-[color:var(--color-primary-200,`#B3D1F7`)]" - : "bg-white hover:bg-[color:var(--color-gray-50,`#F9F9F9`)] cursor-pointer group"; + : isClickable + ? "bg-white hover:bg-[color:var(--color-gray-50,`#F9F9F9`)] cursor-pointer group" + : "bg-white"; return [listItemBaseClasses, stateClasses].join(" "); };-const getTableRowClasses = (isSelected: boolean, isDisabled: boolean): string => { +const getTableRowClasses = ( + isSelected: boolean, + isDisabled: boolean, + isClickable: boolean +): string => { if (isDisabled) { return [tableRowBaseClasses, "bg-white opacity-50 cursor-not-allowed"].join(" "); } const stateClasses = isSelected ? "bg-[color:var(--color-primary-50,`#F0F6FD`)] border-[color:var(--color-primary-200,`#B3D1F7`)]" - : "bg-white hover:bg-[color:var(--color-gray-50,`#F9F9F9`)] cursor-pointer"; + : isClickable + ? "bg-white hover:bg-[color:var(--color-gray-50,`#F9F9F9`)] cursor-pointer" + : "bg-white"; return [tableRowBaseClasses, stateClasses].join(" "); };- <div className={getListItemClasses(isActive, isDisabled)} onClick={handleItemClick}> + <div className={getListItemClasses(isActive, isDisabled, !!onClick)} onClick={handleItemClick}>- className={getTableRowClasses(isSelected, isDisabled)} + className={getTableRowClasses(isSelected, isDisabled, !!onRowClick)}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shared/ui/lists/lists.tsx` around lines 91 - 113, The current getListItemClasses and getTableRowClasses always add pointer/hover affordances even when no click handler exists; update their signatures to accept a third boolean (e.g., isClickable or hasOnClick) or change callers to pass handler presence, and only append "cursor-pointer" and hover classes when isClickable is true and isDisabled is false. Modify getListItemClasses and getTableRowClasses to combine listItemBaseClasses/tableRowBaseClasses with the active/selected styles exactly as before but gate the hover and cursor classes behind the new isClickable flag so non-clickable rows/items do not show interactive affordances.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/shared/ui/lists/lists.tsx`:
- Around line 145-146: The clickable list item currently renders a plain div (in
ListItem in lists.tsx) which prevents keyboard activation; update the container
to expose button-like semantics by adding role="button" and keyboard handlers:
set tabIndex=0 (or tabIndex={-1} when isDisabled), add an onKeyDown that invokes
handleItemClick when Enter or Space is pressed, and set
aria-disabled={isDisabled}; also add an appropriate aria-pressed or
aria-selected reflecting isActive. Ensure getListItemClasses, handleItemClick,
isActive and isDisabled are used to gate focusability and activation.
- Around line 293-297: The clickable table row currently only handles mouse
clicks; update the div rendered for each row (the element using key={row.id},
className={getTableRowClasses(isSelected, isDisabled)} and
onClick={handleRowClick}) to be keyboard-accessible by adding role="button",
tabIndex={isDisabled ? -1 : 0}, aria-disabled={isDisabled}, and an onKeyDown
handler that triggers the same action as handleRowClick when Enter or Space is
pressed (and ignores input when isDisabled). Ensure the onKeyDown logic mirrors
handleRowClick behavior so keyboard users get identical interaction.
---
Nitpick comments:
In `@src/shared/ui/lists/lists.tsx`:
- Around line 91-113: The current getListItemClasses and getTableRowClasses
always add pointer/hover affordances even when no click handler exists; update
their signatures to accept a third boolean (e.g., isClickable or hasOnClick) or
change callers to pass handler presence, and only append "cursor-pointer" and
hover classes when isClickable is true and isDisabled is false. Modify
getListItemClasses and getTableRowClasses to combine
listItemBaseClasses/tableRowBaseClasses with the active/selected styles exactly
as before but gate the hover and cursor classes behind the new isClickable flag
so non-clickable rows/items do not show interactive affordances.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3c0d2490-29ea-417e-8a80-ae0ba700a2a6
📒 Files selected for processing (1)
src/shared/ui/lists/lists.tsx
🔎 What is this PR?
📝 Changes
📸 Screenshots (선택)
📚 Background / Context (선택)
✔ Checklist
pnpm build)pnpm lint)🙏 Request
Summary by CodeRabbit
New Features
Documentation