[REFACTOR] 아코디언 컴포넌트 아이콘 위치 조정 및 크기 조정#139
Conversation
JangYEhoon00
requested review from
keemsebin,
ujinsim and
yougyung
as code owners
WalkthroughAccordion 루트와 아이템에 아이콘 크기(iconSize)와 수직 정렬(iconAlign) 제어가 추가되고, 이를 보여주는 두 가지 스토리(IconSizeAccordion, IconAlignAccordion)가 추가되었습니다. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
|
Update: 2026년 03월 09일 23시 08분 33초 |
![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
🤖 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/Accordion/Accordion.tsx`:
- Around line 86-89: The prop name and comment for iconClassName in the
Accordion component are misleading because the prop is applied to the motion.div
wrapper around the arrow, not the SVG itself; update the API by renaming
iconClassName to iconWrapperClassName (and update its JSDoc) wherever
defined/used in Accordion (props interface and usages around the motion.div
rendering the arrow, e.g., the code referenced near lines 165-173), or
alternatively add a separate prop (e.g., iconSvgClassName) so callers can style
the SVG independently; change the prop name/introduce the new prop and update
all references and documentation/comments in the Accordion component and its
consumers to keep the API consistent.
- Around line 124-125: The prop iconSize passed into the Accordion component is
not validated so negative/NaN values flow through to Icon and become invalid CSS
sizes; normalize and validate iconSize inside the Accordion (where iconSize
default is defined and where it's forwarded to Icon around the existing usage at
line 175) by coercing to a Number, ensuring it is a finite positive value (e.g.,
Number(iconSize)), and falling back to the default (20) or clamping to a
sensible minimum if validation fails before passing it to the Icon component;
also ensure the Accordion prop type reflects a numeric value so callers are
guided to pass valid numbers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: cc4dc2ec-bd6f-4515-9433-1c2ca4fb2503
📒 Files selected for processing (2)
src/shared/ui/Accordion/Accordion.stories.tsxsrc/shared/ui/Accordion/Accordion.tsx
keemsebin
left a comment
keemsebin
left a comment
There was a problem hiding this comment.
수고하셨습니다~! 피그마 icon이랑 아코디언 아이콘이랑 뭔가 미묘하게 다른데 확인 부탁드립니다~!
| ); | ||
| }, | ||
| }; | ||
| export const IconSizeAccordion: Story = { |
There was a problem hiding this comment.
iconsize를 컨트롤할수있는 속성이 있음 좋을 것 같네용
| { | ||
| 'self-start': iconAlign === 'top', | ||
| 'self-center': iconAlign === 'center', | ||
| 'self-end': iconAlign === 'bottom', | ||
| }, |
There was a problem hiding this comment.
요 부분만 따로 객체형식으로 빼도 좋을 것 같네요~
start,center,end 라는 기존 네이밍을 따라가는건 어떠신가요?
아이콘 사이즈를 상위 컴포넌트에서 조절이 가능합니다.
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/ui/Accordion/Accordion.tsx`:
- Around line 99-103: The JSDoc `@default` for the iconSize prop is incorrect:
update the JSDoc comment for iconSize in Accordion.tsx to match the
implementation (iconSize ?? context.iconSize ?? 13.5) by changing `@default` 20 to
`@default` 13.5 so the documented default aligns with the actual default used by
the iconSize prop.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b475fd3f-c46b-4872-986e-b21972790277
📒 Files selected for processing (3)
src/shared/ui/Accordion/Accordion.context.tsxsrc/shared/ui/Accordion/Accordion.stories.tsxsrc/shared/ui/Accordion/Accordion.tsx
| /** | ||
| * icon size | ||
| * @default 20 | ||
| */ | ||
| iconSize?: number; |
There was a problem hiding this comment.
JSDoc의 @default 값이 실제 기본값과 불일치합니다.
JSDoc에서 @default 20으로 명시되어 있지만, 실제 코드(Line 146)에서는 iconSize ?? context.iconSize ?? 13.5로 기본값이 13.5입니다. 문서와 구현이 일치하도록 수정이 필요합니다.
📝 수정 제안
/**
* icon size
- * `@default` 20
+ * `@default` 13.5
*/
iconSize?: number;📝 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.
| /** | |
| * icon size | |
| * @default 20 | |
| */ | |
| iconSize?: number; | |
| /** | |
| * icon size | |
| * `@default` 13.5 | |
| */ | |
| iconSize?: number; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/shared/ui/Accordion/Accordion.tsx` around lines 99 - 103, The JSDoc
`@default` for the iconSize prop is incorrect: update the JSDoc comment for
iconSize in Accordion.tsx to match the implementation (iconSize ??
context.iconSize ?? 13.5) by changing `@default` 20 to `@default` 13.5 so the
documented default aligns with the actual default used by the iconSize prop.
3 participants
🔥 연관 이슈
🚀 작업 내용
2026-03-05.1.01.38.mov
iconSIze, iconAlign 프롭스를 통해서 아이콘의 크기 조절 및 아이콘의 위치를 조정할 수 있습니다!
🤔 고민했던 내용
self-align을 사용했는데 이 요소를 사용하는 게 나았을지 className 을 프롭스로 전달하여 아이콘의 위치를 보다 더 자유롭게 설정을 할 수 있게 해야 할지 고민했습니다. 컴포넌트에서 사용된다면 보다 엄격한 것이 레이아웃을 해치지 않을 것으로 판단하여 self-align 요소를 사용했습니다.
💬 리뷰 중점사항
Summary by CodeRabbit