Conversation
Closed
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughIconsResponsive(아이콘 렌더러), BtnIcon(아이콘 버튼), 관련 스타일 IconRenewal.css.ts, 그리고 BtnIcon용 Storybook 스토리를 새로 추가합니다. BtnIcon은 크기(S/M/L/XL), disabled 및 onClick 프로퍼티를 지원합니다. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
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 |
빌드 결과빌드 성공 🎊 |
🎨 Storybook 빌드 완료!📚 Storybook: https://686a831b8e000345a949970a-ijsltexcmm.chromatic.com/ 📊 빌드 정보
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d89dad53da
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| @@ -1,3 +0,0 @@ | |||
| <svg width="24" height="24" viewBox="0 0 24 24" fill="none" xmlns="http://www.w3.org/2000/svg"> | |||
There was a problem hiding this comment.
This change removes legacy Icn*.svg files, but several existing components still import those exact paths (for example src/shared/components/v2/bottomSheet/BottomSheetBase.tsx:6, src/shared/components/v2/navBar/TitleNavBar.tsx:1, and src/shared/components/v2/roomTypeCard/RoomTypeCard.tsx:7-10). In environments that compile these modules, bundling fails when @assets/v2/svg/IcnX.svg?react (and similar) can no longer be resolved, so the app cannot build or load those screens. Please keep compatibility aliases or update all remaining imports in the same commit.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 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/components/v2/iconsRenewal/BtnIcon.tsx`:
- Around line 18-23: The BtnIconProps interface declares size as required while
the BtnIcon component provides a default value (same discrepancy as
IconsResponsive); update the types to match behavior by making size optional in
BtnIconProps (change to size?: BtnIconSize) or alternatively remove the
component's default size — locate the BtnIconProps declaration and the BtnIcon
functional component in BtnIcon.tsx and apply the chosen fix so the prop type
and runtime default are consistent.
- Around line 47-58: The button in the BtnIcon component is missing an explicit
type, which defaults to "submit" inside forms; update the JSX for the <button>
element in BtnIcon to include type="button" so clicks don't trigger unexpected
form submissions while preserving existing className, disabled, and pointer
event handlers (see styles.btnIcon usage and props
onClick/onPointerDown/onPointerUp/onPointerLeave).
- Around line 9-16: Move the BtnIconSize type declaration above the
BTN_ICON_SIZE constant so the type is declared before it's referenced;
specifically, place "export type BtnIconSize = 'S' | 'M' | 'L' | 'XL';" before
the const BTN_ICON_SIZE declaration and keep BTN_ICON_SIZE: Record<BtnIconSize,
IconSize> = { ... } as const unchanged.
- Around line 34-45: Rename the event handler functions onPointerDown,
onPointerUp, and onPointerLeave to follow the guideline prefix (e.g.,
handlePointerDown, handlePointerUp, handlePointerLeave) and update all
references/usages in the component JSX and any exported props to the new names;
ensure the implementations keep the same logic (checking disabled and setting
setIsPressed) and update any prop names or forwarded handlers to avoid name
collisions with DOM/event props.
In `@src/shared/components/v2/iconsRenewal/IconRenewal.css.ts`:
- Around line 14-24: The btnIcon recipe is missing a base reset for button
default browser styles; update the btnIcon recipe to add a base property that
resets background, border, padding, margin, and appearance (e.g., background:
'none', border: 'none', padding: 0, margin: 0, appearance: 'none') and adds
sensible defaults for layout (e.g., display: 'inline-flex', alignItems:
'center', justifyContent: 'center', lineHeight: 1) so the icon button renders
consistently; add this base block alongside the existing variants in the btnIcon
recipe to ensure the reset applies universally.
In `@src/shared/components/v2/iconsRenewal/IconsResponsive.tsx`:
- Around line 26-49: The IconsName object uses repetitive key:value pairs for
icons (e.g., ArrowLeft: ArrowLeft, ArrowRight: ArrowRight); update the IconsName
declaration to use ES6 object property shorthand by listing the identifiers
directly (ArrowLeft, ArrowLeftFill, ArrowRight, ArrowRightFill, ArrowUp,
ChevronDown, ChevronUp, Close, CloseFillBlack, CloseFillGray, DoubleStar,
FlipHorizontal, HeartFillColor, HeartFillGray, HeartStrokeGray,
HeartStrokeWhite, Link, PlusFill, Profile, Refresh, Search, ViewDetail) and keep
the existing "as const" to preserve types.
- Around line 54-59: The prop typing is inconsistent: IconsResponsiveProps
declares size as required but the IconsResponsive component provides a default
('24'); make them consistent by marking size optional in IconsResponsiveProps
(change size: IconSize to size?: IconSize) so the component can safely rely on
the default, and ensure the default value ('24') is a valid IconSize.
In `@src/stories/BtnIcon.stories.tsx`:
- Around line 5-11: The Storybook meta for BtnIcon is missing argTypes and
individual variant stories; update the exported meta object (meta: Meta<typeof
BtnIcon>) to include an argTypes map that exposes the BtnIcon props (e.g.,
variant, size, disabled, onClick) with controls/types so the Controls panel can
manipulate them, and add separate Story exports (e.g., Primary, Secondary or
VariantSmall/VariantLarge) using the StoryObj<typeof BtnIcon> type that set
different args for each prop variant; reference the meta object and the BtnIcon
component to implement these changes.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a14dab63-a8b0-42c5-a768-1fe691f9b447
⛔ Files ignored due to path filters (42)
src/shared/assets/v2/svg/ArrowLeft.svgis excluded by!**/*.svg,!**/*.svgand included bysrc/**src/shared/assets/v2/svg/ArrowLeftFill.svgis excluded by!**/*.svg,!**/*.svgand included bysrc/**src/shared/assets/v2/svg/ArrowRight.svgis excluded by!**/*.svg,!**/*.svgand included bysrc/**src/shared/assets/v2/svg/ArrowRightFill.svgis excluded by!**/*.svg,!**/*.svgand included bysrc/**src/shared/assets/v2/svg/ArrowUp.svgis excluded by!**/*.svg,!**/*.svgand included bysrc/**src/shared/assets/v2/svg/ChevronDown.svgis excluded by!**/*.svg,!**/*.svgand included bysrc/**src/shared/assets/v2/svg/ChevronUp.svgis excluded by!**/*.svg,!**/*.svgand included bysrc/**src/shared/assets/v2/svg/Close.svgis excluded by!**/*.svg,!**/*.svgand included bysrc/**src/shared/assets/v2/svg/CloseFillBlack.svgis excluded by!**/*.svg,!**/*.svgand included bysrc/**src/shared/assets/v2/svg/CloseFillGray.svgis excluded by!**/*.svg,!**/*.svgand included bysrc/**src/shared/assets/v2/svg/DoubleStar.svgis excluded by!**/*.svg,!**/*.svgand included bysrc/**src/shared/assets/v2/svg/FlipHorizontal.svgis excluded by!**/*.svg,!**/*.svgand included bysrc/**src/shared/assets/v2/svg/HeartFillColor.svgis excluded by!**/*.svg,!**/*.svgand included bysrc/**src/shared/assets/v2/svg/HeartFillGray.svgis excluded by!**/*.svg,!**/*.svgand included bysrc/**src/shared/assets/v2/svg/HeartStrokeGray.svgis excluded by!**/*.svg,!**/*.svgand included bysrc/**src/shared/assets/v2/svg/HeartStrokeWhite.svgis excluded by!**/*.svg,!**/*.svgand included bysrc/**src/shared/assets/v2/svg/IcnArrowLeftM.svgis excluded by!**/*.svg,!**/*.svgand included bysrc/**src/shared/assets/v2/svg/IcnArrowLeftS.svgis excluded by!**/*.svg,!**/*.svgand included bysrc/**src/shared/assets/v2/svg/IcnArrowLeftXs.svgis excluded by!**/*.svg,!**/*.svgand included bysrc/**src/shared/assets/v2/svg/IcnArrowRightS.svgis excluded by!**/*.svg,!**/*.svgand included bysrc/**src/shared/assets/v2/svg/IcnArrowRightXs.svgis excluded by!**/*.svg,!**/*.svgand included bysrc/**src/shared/assets/v2/svg/IcnArrowUp.svgis excluded by!**/*.svg,!**/*.svgand included bysrc/**src/shared/assets/v2/svg/IcnChevronDown.svgis excluded by!**/*.svg,!**/*.svgand included bysrc/**src/shared/assets/v2/svg/IcnCloseFill.svgis excluded by!**/*.svg,!**/*.svgand included bysrc/**src/shared/assets/v2/svg/IcnDelete.svgis excluded by!**/*.svg,!**/*.svgand included bysrc/**src/shared/assets/v2/svg/IcnDoubleStar.svgis excluded by!**/*.svg,!**/*.svgand included bysrc/**src/shared/assets/v2/svg/IcnFlipHorizontal.svgis excluded by!**/*.svg,!**/*.svgand included bysrc/**src/shared/assets/v2/svg/IcnHeartColorL.svgis excluded by!**/*.svg,!**/*.svgand included bysrc/**src/shared/assets/v2/svg/IcnHeartGrayL.svgis excluded by!**/*.svg,!**/*.svgand included bysrc/**src/shared/assets/v2/svg/IcnHeartGrayXs.svgis excluded by!**/*.svg,!**/*.svgand included bysrc/**src/shared/assets/v2/svg/IcnLink.svgis excluded by!**/*.svg,!**/*.svgand included bysrc/**src/shared/assets/v2/svg/IcnPlusFill.svgis excluded by!**/*.svg,!**/*.svgand included bysrc/**src/shared/assets/v2/svg/IcnRefresh.svgis excluded by!**/*.svg,!**/*.svgand included bysrc/**src/shared/assets/v2/svg/IcnSearch.svgis excluded by!**/*.svg,!**/*.svgand included bysrc/**src/shared/assets/v2/svg/IcnViewDetail.svgis excluded by!**/*.svg,!**/*.svgand included bysrc/**src/shared/assets/v2/svg/IcnX.svgis excluded by!**/*.svg,!**/*.svgand included bysrc/**src/shared/assets/v2/svg/Link.svgis excluded by!**/*.svg,!**/*.svgand included bysrc/**src/shared/assets/v2/svg/PlusFill.svgis excluded by!**/*.svg,!**/*.svgand included bysrc/**src/shared/assets/v2/svg/Profile.svgis excluded by!**/*.svg,!**/*.svgand included bysrc/**src/shared/assets/v2/svg/Refresh.svgis excluded by!**/*.svg,!**/*.svgand included bysrc/**src/shared/assets/v2/svg/Search.svgis excluded by!**/*.svg,!**/*.svgand included bysrc/**src/shared/assets/v2/svg/ViewDetail.svgis excluded by!**/*.svg,!**/*.svgand included bysrc/**
📒 Files selected for processing (4)
src/shared/components/v2/iconsRenewal/BtnIcon.tsxsrc/shared/components/v2/iconsRenewal/IconRenewal.css.tssrc/shared/components/v2/iconsRenewal/IconsResponsive.tsxsrc/stories/BtnIcon.stories.tsx
Comment on lines
+5
to
+11
| const meta: Meta<typeof BtnIcon> = { | ||
| title: 'Components/BtnIcon', | ||
| component: BtnIcon, | ||
| }; | ||
| export default meta; | ||
|
|
||
| type Story = StoryObj<typeof BtnIcon>; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
argTypes 추가로 Storybook 컨트롤을 개선할 수 있어요.
argTypes를 정의하면 Storybook Controls 패널에서 props를 인터랙티브하게 조작할 수 있어요. 또한 개별 variant 스토리 추가도 고려해 보세요.
♻️ argTypes 및 개별 스토리 추가 예시
const meta: Meta<typeof BtnIcon> = {
title: 'Components/BtnIcon',
component: BtnIcon,
+ argTypes: {
+ name: {
+ control: 'select',
+ options: ['Search', 'Close', 'ArrowLeft', 'ArrowRight'],
+ },
+ size: {
+ control: 'radio',
+ options: ['S', 'M', 'L', 'XL'],
+ },
+ disabled: {
+ control: 'boolean',
+ },
+ },
};
export default meta;
type Story = StoryObj<typeof BtnIcon>;
+// 기본 스토리 (컨트롤 패널 사용 가능)
+export const Default: Story = {
+ args: {
+ name: 'Search',
+ size: 'M',
+ disabled: false,
+ },
+};As per coding guidelines: "모든 props 변형에 대한 스토리 작성"
📝 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.
Suggested change
| const meta: Meta<typeof BtnIcon> = { | |
| title: 'Components/BtnIcon', | |
| component: BtnIcon, | |
| }; | |
| export default meta; | |
| type Story = StoryObj<typeof BtnIcon>; | |
| const meta: Meta<typeof BtnIcon> = { | |
| title: 'Components/BtnIcon', | |
| component: BtnIcon, | |
| argTypes: { | |
| name: { | |
| control: 'select', | |
| options: ['Search', 'Close', 'ArrowLeft', 'ArrowRight'], | |
| }, | |
| size: { | |
| control: 'radio', | |
| options: ['S', 'M', 'L', 'XL'], | |
| }, | |
| disabled: { | |
| control: 'boolean', | |
| }, | |
| }, | |
| }; | |
| export default meta; | |
| type Story = StoryObj<typeof BtnIcon>; | |
| // 기본 스토리 (컨트롤 패널 사용 가능) | |
| export const Default: Story = { | |
| args: { | |
| name: 'Search', | |
| size: 'M', | |
| disabled: false, | |
| }, | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/stories/BtnIcon.stories.tsx` around lines 5 - 11, The Storybook meta for
BtnIcon is missing argTypes and individual variant stories; update the exported
meta object (meta: Meta<typeof BtnIcon>) to include an argTypes map that exposes
the BtnIcon props (e.g., variant, size, disabled, onClick) with controls/types
so the Controls panel can manipulate them, and add separate Story exports (e.g.,
Primary, Secondary or VariantSmall/VariantLarge) using the StoryObj<typeof
BtnIcon> type that set different args for each prop variant; reference the meta
object and the BtnIcon component to implement these changes.
빌드 결과빌드 성공 🎊 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/components/v2/iconsRenewal/BtnIcon.tsx`:
- Around line 18-23: The BtnIcon component lacks an aria-label prop making icon
buttons inaccessible to screen readers; update the BtnIconProps interface to
include an optional ariaLabel?: string (or aria-label as a string prop name used
in the codebase) and forward that prop from the BtnIcon component to the
underlying button element (ensure the button receives aria-label={ariaLabel} and
preserve existing onClick/disabled/size handling); update any usages and the
component signature (BtnIconProps and BtnIcon) so callers can pass aria labels
like <BtnIcon ariaLabel="검색" ... /> and add tests or story examples
demonstrating the new prop.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d96ddd9d-5fad-46df-afc1-62801a9d4aaf
📒 Files selected for processing (2)
src/shared/components/v2/iconsRenewal/BtnIcon.tsxsrc/shared/components/v2/iconsRenewal/IconsResponsive.tsx
Comment on lines
+18
to
+23
| export interface BtnIconProps { | ||
| name: IconName; | ||
| size?: BtnIconSize; | ||
| disabled?: boolean; | ||
| onClick?: () => void; | ||
| } |
There was a problem hiding this comment.
아이콘 전용 버튼에 접근성 레이블이 필요해요.
스크린 리더 사용자가 버튼의 용도를 파악할 수 없어요. aria-label prop을 추가해서 버튼의 기능을 명시해주세요.
♿ 접근성 개선 제안
export interface BtnIconProps {
name: IconName;
size?: BtnIconSize;
disabled?: boolean;
onClick?: () => void;
+ 'aria-label': string;
}
const BtnIcon = ({
name,
size = 'S',
disabled = false,
onClick,
+ 'aria-label': ariaLabel,
}: BtnIconProps) => {
// ...
return (
<button
type="button"
+ aria-label={ariaLabel}
className={styles.btnIcon({사용 예시:
<BtnIcon name="Search" aria-label="검색" onClick={handleSearch} />As per coding guidelines: "접근성 준수: ARIA 속성, label 연결, 키보드 포커스"
Also applies to: 47-61
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/shared/components/v2/iconsRenewal/BtnIcon.tsx` around lines 18 - 23, The
BtnIcon component lacks an aria-label prop making icon buttons inaccessible to
screen readers; update the BtnIconProps interface to include an optional
ariaLabel?: string (or aria-label as a string prop name used in the codebase)
and forward that prop from the BtnIcon component to the underlying button
element (ensure the button receives aria-label={ariaLabel} and preserve existing
onClick/disabled/size handling); update any usages and the component signature
(BtnIconProps and BtnIcon) so callers can pass aria labels like <BtnIcon
ariaLabel="검색" ... /> and add tests or story examples demonstrating the new
prop.
빌드 결과빌드 성공 🎊 |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/shared/components/v2/iconsRenewal/BtnIcon.tsx (1)
18-23:⚠️ Potential issue | 🟠 Major아이콘 전용 버튼에
aria-labelprop이 누락되어 있어요.스크린 리더 사용자가 버튼의 용도를 파악할 수 없어요. 접근성 가이드라인 준수를 위해
aria-labelprop을 추가해주세요.♿ 접근성 개선 제안
export interface BtnIconProps { name: IconName; size?: BtnIconSize; disabled?: boolean; onClick?: () => void; + 'aria-label': string; } const BtnIcon = ({ name, size = 'S', disabled = false, onClick, + 'aria-label': ariaLabel, }: BtnIconProps) => {<button type="button" + aria-label={ariaLabel} className={styles.btnIcon({As per coding guidelines: "접근성 준수: ARIA 속성, label 연결, 키보드 포커스"
Also applies to: 47-62
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shared/components/v2/iconsRenewal/BtnIcon.tsx` around lines 18 - 23, BtnIconProps is missing an aria-label prop which prevents screen readers from describing the button; add ariaLabel?: string to the BtnIconProps interface and update the BtnIcon component (where props are destructured and the <button> is rendered) to accept and apply aria-label={ariaLabel} on the button element (also update any internal uses in the BtnIcon render path around the code in the BtnIcon component, including the block referenced at lines ~47-62). Make the prop optional in the type but ensure callers are encouraged to pass a meaningful label; propagate the prop through any wrapper components and tests.
🤖 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/components/v2/iconsRenewal/BtnIcon.tsx`:
- Around line 25-30: The BtnIcon component lacks a React displayName which helps
DevTools and error stacks; add a displayName property on the BtnIcon function
(e.g., set BtnIcon.displayName = 'BtnIcon' or a more descriptive name like
'IconsRenewal.BtnIcon') immediately after the component declaration so the
runtime and DevTools show a readable component name; reference the BtnIcon
function (and BtnIconProps if needed) to locate where to attach the displayName.
In `@src/shared/components/v2/iconsRenewal/IconRenewal.css.ts`:
- Around line 28-33: The size variant object in IconRenewal.css.ts currently
leaves S, L, and XL as empty objects so only the base gapPadding[100] applies
and only M overrides to gapPadding[200]; update the size variants (the size: {
S, M, L, XL } block) to explicitly set the intended padding for S, L, and XL
(e.g., gapPadding[100/150/200] or whatever design tokens are required) instead
of empty objects, or add a clear comment that leaving them empty is intentional;
ensure you reference unitVars.unit.gapPadding[...] when assigning the per-size
padding so each variant has the correct override.
---
Duplicate comments:
In `@src/shared/components/v2/iconsRenewal/BtnIcon.tsx`:
- Around line 18-23: BtnIconProps is missing an aria-label prop which prevents
screen readers from describing the button; add ariaLabel?: string to the
BtnIconProps interface and update the BtnIcon component (where props are
destructured and the <button> is rendered) to accept and apply
aria-label={ariaLabel} on the button element (also update any internal uses in
the BtnIcon render path around the code in the BtnIcon component, including the
block referenced at lines ~47-62). Make the prop optional in the type but ensure
callers are encouraged to pass a meaningful label; propagate the prop through
any wrapper components and tests.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e048a7b6-e849-4972-9d47-806bb036b060
📒 Files selected for processing (2)
src/shared/components/v2/iconsRenewal/BtnIcon.tsxsrc/shared/components/v2/iconsRenewal/IconRenewal.css.ts
Comment on lines
+25
to
+30
| const BtnIcon = ({ | ||
| name, | ||
| size = 'S', | ||
| disabled = false, | ||
| onClick, | ||
| }: BtnIconProps) => { |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
displayName 설정을 권장해요.
공통 컴포넌트에서 displayName을 설정하면 React DevTools 디버깅과 에러 스택 추적이 용이해요.
♻️ displayName 추가
const BtnIcon = ({
// ...
}: BtnIconProps) => {
// ...
};
+BtnIcon.displayName = 'BtnIcon';
+
export default BtnIcon;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/shared/components/v2/iconsRenewal/BtnIcon.tsx` around lines 25 - 30, The
BtnIcon component lacks a React displayName which helps DevTools and error
stacks; add a displayName property on the BtnIcon function (e.g., set
BtnIcon.displayName = 'BtnIcon' or a more descriptive name like
'IconsRenewal.BtnIcon') immediately after the component declaration so the
runtime and DevTools show a readable component name; reference the BtnIcon
function (and BtnIconProps if needed) to locate where to attach the displayName.
Comment on lines
+28
to
+33
| size: { | ||
| S: {}, | ||
| M: { padding: unitVars.unit.gapPadding[200] }, | ||
| L: {}, | ||
| XL: {}, | ||
| }, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
size variants 중 S, L, XL이 빈 객체예요.
현재 S, L, XL은 빈 객체로 base의 gapPadding[100] 패딩만 적용되고, M만 gapPadding[200]으로 오버라이드돼요. 의도한 디자인이라면 괜찮지만, 크기별로 다른 패딩이 필요할 수 있어요.
♻️ 크기별 패딩 차별화 제안
size: {
- S: {},
- M: { padding: unitVars.unit.gapPadding[200] },
- L: {},
- XL: {},
+ S: { padding: unitVars.unit.gapPadding[100] },
+ M: { padding: unitVars.unit.gapPadding[200] },
+ L: { padding: unitVars.unit.gapPadding[300] },
+ XL: { padding: unitVars.unit.gapPadding[400] },
},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/shared/components/v2/iconsRenewal/IconRenewal.css.ts` around lines 28 -
33, The size variant object in IconRenewal.css.ts currently leaves S, L, and XL
as empty objects so only the base gapPadding[100] applies and only M overrides
to gapPadding[200]; update the size variants (the size: { S, M, L, XL } block)
to explicitly set the intended padding for S, L, and XL (e.g.,
gapPadding[100/150/200] or whatever design tokens are required) instead of empty
objects, or add a clear comment that leaving them empty is intentional; ensure
you reference unitVars.unit.gapPadding[...] when assigning the per-size padding
so each variant has the correct override.
earl9rey
reviewed
Mar 18, 2026
Contributor
earl9rey
left a comment
earl9rey
left a comment
There was a problem hiding this comment.
확인했습니다!!
아이콘 크기 단위 관련해서는 가볍게 논의가 필요할 것 같기도.. 항상 크기를 고정하려면 px이 적절할 것 같은데 또 반응형이나 브라우저 사용자 설정에 좀 더 맞추려면 rem이 더 적절할 것 같기도 하고요 (unit 토큰도 rem으로 통일되어 있으니) 근데 또 모바일이니까 괜찮을 것 같기도 하고..?
@jstar000 정답을 알려줘
pressed 관련 리뷰만 한번 확인해주시면 될 것 같아요!!
수고 많으셨어요 👏🏻
| disabled = false, | ||
| onClick, | ||
| }: BtnIconProps) => { | ||
| const [isPressed, setIsPressed] = useState(false); |
Contributor
There was a problem hiding this comment.
지금 pressed 여부를 state로 관리하고 있는데, CSS에서:active로 처리하면 더 단순해질 것 같아요.
selectors: {
'&:active': {
transform: 'scale(0.95)',
},저는 이렇게 status variant 대신 selectors로 처리했는데, 차이점은 터치 후 손가락을 버튼 밖으로 밀었을 때, 현재는 pointerLeave에서 바로 해제되고 :active는 손을 뗄 때까지 유지될 수 있다는 정도라고 하네요?!
개인적으로는 :active로 단순화해도 괜찮을 것 같다고 생각하는데 소이님 의견이 궁금합니다!!
Contributor
Author
There was a problem hiding this comment.
크기만 작아지니까 단순하게 가는게 좋을 것 같아요! 반영하겠습니당
Contributor
Author
빌드 결과빌드 성공 🎊 |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/shared/components/v2/iconsRenewal/BtnIcon.tsx (2)
42-44: 🧹 Nitpick | 🔵 Trivial공통 컴포넌트에
displayName설정을 권장해요.React DevTools 디버깅과 에러 스택 추적이 용이해져요.
♻️ displayName 추가
}; +BtnIcon.displayName = 'BtnIcon'; + export default BtnIcon;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shared/components/v2/iconsRenewal/BtnIcon.tsx` around lines 42 - 44, 컴포넌트 BtnIcon에 displayName이 빠져 있어 디버깅과 에러 스택 추적이 어렵습니다; BtnIcon 함수/상수 선언부(및 파일 끝의 export default BtnIcon)를 찾아 컴포넌트에 수동으로 displayName을 설정하십시오(예: BtnIcon.displayName = 'BtnIcon') so React DevTools와 스택 트레이스에 명확한 이름이 나오도록 합니다.
16-21:⚠️ Potential issue | 🟠 Major아이콘 전용 버튼에
aria-labelprop이 필요해요.텍스트 없이 아이콘만 있는 버튼은 스크린 리더 사용자가 용도를 파악할 수 없어요. 접근성을 위해
aria-label을 필수 prop으로 추가해주세요.♿ 접근성 개선 제안
export interface BtnIconProps { name: IconName; size?: BtnIconSize; disabled?: boolean; onClick?: () => void; + 'aria-label': string; } const BtnIcon = ({ name, size = 'S', disabled = false, onClick, + 'aria-label': ariaLabel, }: BtnIconProps) => { return ( <button type="button" + aria-label={ariaLabel} className={styles.btnIcon({사용 예시:
<BtnIcon name="Search" aria-label="검색" onClick={handleSearch} />As per coding guidelines: "접근성 준수: ARIA 속성, label 연결, 키보드 포커스"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shared/components/v2/iconsRenewal/BtnIcon.tsx` around lines 16 - 21, Make aria-label a required prop for the icon-only button by adding ariaLabel (or rename to aria-label via index signature) to the BtnIconProps interface and update the BtnIcon component to pass that value to the rendered <button> element (e.g., ensure the component uses props.ariaLabel on the button and not omit it), and update any prop destructuring in the BtnIcon function to include ariaLabel so screen readers get the label; keep the prop required (remove the optional ?) and update any call-sites/types accordingly.
🤖 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/components/v2/iconsRenewal/IconRenewal.css.ts`:
- Around line 6-14: The iconSize styleVariants currently uses pixel values;
replace those px strings with rem values using the project convention (reset
sets 1rem=10px) so convert each size (e.g., '12' -> '1.2rem', '14' -> '1.4rem',
'16' -> '1.6rem', '20' -> '2rem', '24' -> '2.4rem', '32' -> '3.2rem', '40' ->
'4rem') while keeping the same keys and structure in the exported iconSize (and
leave styleVariants usage unchanged).
---
Duplicate comments:
In `@src/shared/components/v2/iconsRenewal/BtnIcon.tsx`:
- Around line 42-44: 컴포넌트 BtnIcon에 displayName이 빠져 있어 디버깅과 에러 스택 추적이 어렵습니다;
BtnIcon 함수/상수 선언부(및 파일 끝의 export default BtnIcon)를 찾아 컴포넌트에 수동으로 displayName을
설정하십시오(예: BtnIcon.displayName = 'BtnIcon') so React DevTools와 스택 트레이스에 명확한 이름이
나오도록 합니다.
- Around line 16-21: Make aria-label a required prop for the icon-only button by
adding ariaLabel (or rename to aria-label via index signature) to the
BtnIconProps interface and update the BtnIcon component to pass that value to
the rendered <button> element (e.g., ensure the component uses props.ariaLabel
on the button and not omit it), and update any prop destructuring in the BtnIcon
function to include ariaLabel so screen readers get the label; keep the prop
required (remove the optional ?) and update any call-sites/types accordingly.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f8d540ac-8203-482c-ac0b-89f04a2d2fb1
📒 Files selected for processing (2)
src/shared/components/v2/iconsRenewal/BtnIcon.tsxsrc/shared/components/v2/iconsRenewal/IconRenewal.css.ts
빌드 결과빌드 성공 🎊 |
빌드 결과빌드 성공 🎊 |
jstar000
reviewed
Mar 18, 2026
Contributor
jstar000
left a comment
jstar000
left a comment
There was a problem hiding this comment.
확인했습니다~ 네이밍때문에 나중에 헷갈릴 것 같은 부분들이 있어서 리뷰 남겨요 의견 부탁드립니다ㅎㅎ
- v2/iconsRenewal 폴더 아래에 파일을 생성해두셨는데
renewal은 도메인이나 역할명이 아니라 일회성 작업명이라 장기적으로 유지할 폴더명으로는 적절하지 않아보여요. 역할 기준으로 icon, button 처럼 나누면 더 자연스러울 것 같은데 어떻게 생각하세요? - BtnIcon은 아이콘이 아니라 아이콘만 포함한 '버튼'이므로 이름을
IconButton으로 바꾸고(story파일과 BtnIcon.tsx 내부 상수/변수값 네이밍에도 반영)shared/components/v2/button폴더 아래에 넣는건 어떨까요? 앞으로 다른 버튼도 추가된다는 점을 생각하면 이 방식도 좋을 것 같아요. - IconResponsive 아이콘은 name과 size를 받아서 아이콘을 렌더링하는 컴포넌트이므로 responsive한 컴포넌트는 아님 -> 이름을
Icon으로 바꾸고shared/components/v2/icon/Icon.tsx로 이동하는건 어떨까요? - 컨벤션상 컴포넌트와 css 파일명은 맞추고 있는데 현재 컴포넌트 파일명은 'IconResponsive', css 파일은 'IconRenewal'이에요. 컴포넌트 이름을 Icon으로 바꾼다면 css 파일 이름도
Icon.css.ts로 수정해면 좋을 것 같아요
| XL: '40', | ||
| } as const; | ||
|
|
||
| export interface BtnIconProps { |
Contributor
There was a problem hiding this comment.
disabled, onclick 같은 button의 속성을 하나씩 props에 명시해주기보다는
interface IconButtonProps
extends Omit<React.ComponentProps<'button'>, 'children'> {
name: IconName;
size?: ButtonSize;
}
const IconButton = ({
name,
size = 'S',
type = 'button',
className,
disabled = false,
...props
}: IconButtonProps) => {
return (
<button
type={type}
disabled={disabled}
className={clsx(
styles.iconButton({ size, disabled }),
className
)}
{...props}
>
<Icon name={name} size={ICON_SIZE_BY_BUTTON[size]} />
</button>
);
};처럼 React.ComponentProps<'button'>을 상속하도록 하면 aria-label, className, id 등을 사용처에서 전달해 버튼을 더 유연하게 사용 가능하고,
<IconButton name="CloseFillGray" aria-label="닫기" /> 처럼 외부에서 접근성 관련 값을 전달할 수 있으므로, 내부 이미지의 alt가 버튼 이름으로 노출되는 현재 구조보다 접근성 측면에서 유리할 것 같아요.
Contributor
Author
There was a problem hiding this comment.
오 이거 까먹고 있었어요! 적용하겠습니다
Contributor
Author
There was a problem hiding this comment.
폴더/파일명 관련해서도 의견남겨주셔서 감사합니다! 처음에 Icon으로 하려다가 헷갈릴 것 같아서 우선 figma에 있는 이름으로 해놨었는데, 임리드님의 의견에 다시 힘을 받고 수정하도록 하겠습니다!
빌드 결과빌드 성공 🎊 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
📌 Summary
📄 Tasks
IconsResponsive와BtnIcon의 스타일을IconRenewal.css.ts하나에서 관리하도록 했습니다. (두 컴포넌트가 아이콘이라는 같은 도메인에 속하기 때문 + 스타일이 두개 뿐이라..)IconsResponsive.tsx
svgr를 활용했기에 SVG url 관리를 해당 컴포넌트 내부에서 일괄 처리하고,name과sizeprops만으로 아이콘을 렌더링 할 수 있도록 했습니다.BtnIcon.tsx
IconsResponsive에서 정의했던 사이즈 중 필요한 사이즈를 매핑했고,disabled,pressed의 옵션도 관리합니다.pressed상태를 관리할 때onPointerDown/Up/Leave을 사용해 마우스, 터치 환경에서 모두 대응하도록 했습니다. (예전에 바텀시트하면서 mouse/touch 다 따로 설정하니 너무 길어져서 이 방법을 사용했었습니다!)사용방법
🔍 To Reviewer
📸 Screenshot