feat : checkboxGroup 추가 #115
Conversation
| import type { Meta, StoryObj } from '@storybook/react'; | ||
| import { CheckboxGroup } from './CheckboxGroup'; | ||
| import { Checkbox } from './Checkbox'; | ||
| import { Children } from 'react'; |
There was a problem hiding this comment.
import {Children} 요거땜에 build안되는거같네여 지우면될거같아여
|
흠 뭐지 prettier에서 오류가 엄청 나네요 로컬에서 |
|
앗 혹시 윈도우이슈..? |
|
윈도우는 컴퓨턴가요? |
There was a problem hiding this comment.
@asdf99245 @GulSam00 리뷰 남겼어요!!!!!
유틸함수 추가했는데, 추후에 페어로 요거 리팩토링 어떠신지?,,
| type CheckboxContextType = Omit<Props, 'children'>; | ||
|
|
||
| export const CheckboxContext = createContext<CheckboxContextType | null>(null); |
There was a problem hiding this comment.
@asdf99245, @GulSam00
context는 context.ts파일로 분리하는 것은 어떨까요?
There was a problem hiding this comment.
요거는 utils의 createContext 이용해서 분리하게 되는거죠? 일단은 냅둘게요...!
| if (context) { | ||
| checked = context.selectedValue.includes(value); | ||
| onChange = context.onChange; | ||
| } |
There was a problem hiding this comment.
onChangeFromProps와 onChangeFromContext 모두 event를 전달하여 동작하게끔 하는 건 어떨까요?
요게 이유가 Checkbox에 핸들러를 전달했는데, context에 핸들러가 있을 경우 무시하게 되는데, 개발자 입장에서 interface엔 onChange가 열려있는데 왜 동작을 안 하지?,, 라고 오해할 수도 있을 것 같아요!
There was a problem hiding this comment.
요렇게 할 시에 그러면 두 핸들러 모두 동작하게 하는건가여?
There was a problem hiding this comment.
오호 유틸함수 추가해주신걸로 주말에 샴푸님이랑 리팩토링해볼게용
|
|
||
| import { Text } from '../Text'; | ||
|
|
||
| interface Props { | ||
| labelText: string; | ||
| children: string; |
| type CheckboxContextType = Omit<Props, 'children'>; | ||
|
|
||
| export const CheckboxContext = createContext<CheckboxContextType | null>(null); |
src/components/Checkbox/Checkbox.tsx
Outdated
| export function Checkbox({ labelText, checked = false, disabled = false, onChange }: Props) { | ||
| export function Checkbox({ children, checked = false, disabled = false, value, onChange }: Props) { | ||
|
|
||
| const context = useContext(CheckboxContext); |
There was a problem hiding this comment.
useContext를 context선언한 곳에서 불러오는건 어떨까요?
There was a problem hiding this comment.
CheckboxGroup에서 불러온다는 말씀이실까요?
There was a problem hiding this comment.
context로 분리하면 그때 같이 요것도 넣어두면 좋을 것 같아요
| if (context) { | ||
| checked = context.selectedValue.includes(value); | ||
| onChange = context.onChange; | ||
| } |
| export function CheckboxGroup({ children, ...restProps }: Props) { | ||
| return ( | ||
| <CheckboxContext.Provider value={{ ...restProps }}> | ||
| <fieldset className="flex flex-col gap-2">{children}</fieldset> |
There was a problem hiding this comment.
나중에 row, col 선택할 수 있게 추가하는건 어떨까요?
src/components/Checkbox/Checkbox.tsx
Outdated
| import cx from 'classnames'; | ||
| import { twMerge } from 'tailwind-merge'; | ||
| import CheckIcon from '@material-design-icons/svg/outlined/check.svg'; | ||
| import { CheckboxContext } from './CheckboxGroup'; |
바뀐점
checkboxGroup 컴포넌트 추가
바꾼이유
설명
RadioGroup 참고하여 작업.
RadioGroup 처럼 selectedValue string 배열을 Context로 만들어서 전달. Checkbox 내부에서 대조하여서 적용.