-
-
Notifications
You must be signed in to change notification settings - Fork 3
CheckboxGroup 컴포넌트 구현 #711
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
base: main
Are you sure you want to change the base?
Conversation
4b0bd7e to
567a8f2
Compare
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
567a8f2 to
43a7500
Compare
Bundle ReportChanges will increase total bundle size by 22.63kB (4.08%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: daleui-bundle-esmAssets Changed:
|
| <div | ||
| className={css({ display: "flex", flexDirection: "column", gap: "32" })} | ||
| > |
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.
중앙정렬이 들어가도 괜찮다면 VStack 컴포넌트를 사용하는것이 어떨까요?
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.
VStack으로 변경하여 반영했습니다. 감사합니다!
| onValueChange: (value: string, checked: boolean) => void; | ||
| } | null>(null); | ||
|
|
||
| export interface CheckboxGroupProps { |
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.
required는 지원안하는건지 궁금합니다.
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.
디자이너와 명확히 확인 후 반영토록 하겠습니다, 감사합니다!
| /** | ||
| * 컴포넌트가 처음 렌더링될 때 선택되는 값들입니다. | ||
| * @default undefined | ||
| */ | ||
| defaultValue?: string[]; | ||
|
|
||
| /** | ||
| * 외부에서 선택 값을 직접 제어할 때 사용합니다. | ||
| * @default undefined | ||
| */ | ||
| value?: 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.
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.
복수형 좋습니다, defaultValues로 수정하였습니다. 감사합니다!
| * true이면 모든 체크박스가 비활성화되어 상호작용이 불가합니다. | ||
| * @default false | ||
| */ | ||
| disabled?: boolean; |
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.
default가 false라면서 컴포넌트 함수 default parameter에는 아무것도 할당되어 있지 않은데 의도하신 건가요?
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.
doc과 일치하도록 함수 파라미터에 기본값을 추가했습니다. 짚어주셔서 감사합니다!
| /** | ||
| * true이면 이 체크박스가 비활성화됩니다. | ||
| * @default false | ||
| */ | ||
| disabled?: boolean; |
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.
여기도 이 코멘트와 동일합니다.
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.
마찬가지로 doc과 일치하도록 함수 파라미터에 기본값을 추가했습니다.
af80521 to
f0efba2
Compare
… enhance stories and tests for consistency
f0efba2 to
e07630c
Compare
| <CheckboxItem value="apple">사과</CheckboxItem> | ||
| <CheckboxItem value="banana">바나나</CheckboxItem> | ||
| <CheckboxItem value="orange">오렌지</CheckboxItem> |
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.
동일한 피드백입니다.
변경 사항
목적
리뷰어에게
PR 작성자 체크 리스트