-
Notifications
You must be signed in to change notification settings - Fork 0
[Feat] 공용 컴포넌트 Input 제작 #40
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis change introduces a new suite of form-related UI components and composite components. It adds Input, Label, and Hint components to the UI layer, creates FormInput and SearchBar composite components in shared, includes comprehensive Storybook stories for all new components, and updates import paths for the Icon component across the codebase. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro 📒 Files selected for processing (12)
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 |
📊 Coverage Report
📉 #40을 main에 병합하면 coverage가 Coverage 요약@@ Coverage Diff @@
## main #40 +/- ##
===========================================
- Coverage 42.71% 34.42% -8.29%
===========================================
Files 30 36 +6
Lines 817 1011 +194
Branches 75 82 +7
===========================================
- Hits 349 348 -1
+ Misses 468 663 +195 영향받은 파일
|
🚀 PR Preview Report✨ Build가 성공적으로 완료되었습니다. Preview에서 변경사항을 확인하세요.
|
🎨 Storybook Report✨ Story가 변경되었습니다 Chromatic에서 비주얼 변경사항을 확인하세요.
|
17a2995 to
dde96fa
Compare
- restructure Input to use iconButton - unify password input into FormInput - adjust SearchBar to use new Input
Chiman2937
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
수고하셨습니다!
| const passwordIconButton = isPasswordField ? ( | ||
| <button | ||
| className='absolute top-4 right-5 h-6 w-6' | ||
| aria-label={isVisible ? '비밀번호 숨기기' : '비밀번호 보기'} | ||
| type='button' | ||
| onClick={handleToggle} | ||
| > | ||
| <Icon id={isVisible ? 'visibility-true' : 'visibility-false'} className='text-gray-600' /> | ||
| </button> | ||
| ) : null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
현재 passwordIconButton이 FormInput 내부에 선언되어있는데 안티패턴이라서 지양하는 게 좋을 것 같아요!
현재는 함수컴포넌트로 되어있게 아니라 JSX 변수를 저장한 형태라 큰 문제를 일으키진 않지만 만약 passwordIconButton이 함수형 컴포넌트였다면 FormInput이 리렌더링 될 때마다 passwordIconButton 함수가 매번 재생성되면서 React가 이를 완전히 다른 컴포넌트로 인식해서 state 초기화, focus 손실 같은 버그가 발생할 수 있어요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
그렇네요. input 쪼개기에 급급해서 생각을 못했어요.
다른 리뷰들하고 같이 수정해서 다음 작업에 다른 방식을 적용시켜 볼게요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
나중에 Enter로 검색하는 기능도 추가되면 좋겠네요!
| <p | ||
| {...props} | ||
| className={cn('text-error-500 text-text-sm-medium w-full px-2', className)} | ||
| aria-live='polite' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aria-live는 처음 알았네요 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
치영님 코드 보고 찾아보다가 알아냈어요.
스크린 리더가 동적으로 변경된 콘텐츠를 자동으로 읽어주도록 설정 하는 역할을 한다고 합니다.
| export const Label = ({ children, className, required, ...props }: LabelProps) => { | ||
| return ( | ||
| <label {...props} className={cn('text-text-sm-medium flex items-center px-2', className)}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
label에 required가 적용이 안되고 있는것 같아요!
| interface FormInputProps extends React.InputHTMLAttributes<HTMLInputElement> { | ||
| labelName?: string; | ||
| hintMessage?: string; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
현재 FormInputProps가 Input type을 extend 해오고 있는데 FormInput은 div를 반환하고 있기 때문에 타입과 실제 구조가 일치하지 않아 혼란을 일으킬 수 있을 것 같아요
사실 지금상태는 큰 문제를 일으킬 것 같지는 않고 interface를 최대한 간소화시키기 위한 작업이라고 생각돼서 괜찮을 것 같긴 하네요 :)
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 변경 사항
Input 컴포넌트 제작
src/components/ui/input/index.tsx : Input 컴포넌트
src/components/ui/input/index.stories.tsx : story 파일
form input 컴포넌트 제작
src/components/ui/hint/index.tsx : Hint 컴포넌트 (error message)
src/components/ui/label/index.tsx : Label컴포넌트 (form input과 연결용)
src/components/shared/form-input/index.tsx : FormInput 컴포넌트
src/components/shared/form-input/index.stories.tsx : FormInput story 파일
search bar 컴포넌트 제작
src/components/shared/search-bar/index.tsx : SearchBar컴포넌트
src/components/shared/fsearch-bar/index.stories.tsx : SearchBar story 파일
Input 컴포넌트
FormInput 컴포넌트
사용 예시
SearchBar 컴포넌트
사용 예시
🔗 관련 이슈
Closes #31
🧪 테스트 방법
📸 스크린샷 (선택)
📋 체크리스트
💬 추가 코멘트
CodeRabbit Review는 자동으로 실행되지 않습니다.
Review를 실행하려면 comment에 아래와 같이 작성해주세요
Summary by CodeRabbit
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.