Skip to content
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

feat : checkboxGroup 추가 #115

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/components/Checkbox/Checkbox.stories.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ type Story = StoryObj<Component>;
export const Primary: Story = {
// Add your story args here
args: {
labelText: 'label',
children: 'label',
checked: false,
disabled: false,
},
Expand Down
22 changes: 17 additions & 5 deletions src/components/Checkbox/Checkbox.tsx
Original file line number Diff line number Diff line change
@@ -1,15 +1,16 @@
import { ChangeEvent } from 'react';
import { ChangeEvent, useContext } from 'react';
import cx from 'classnames';
import { twMerge } from 'tailwind-merge';
import CheckIcon from '@material-design-icons/svg/outlined/check.svg';
import { CheckboxContext } from './CheckboxGroup';

import { Text } from '../Text';

interface Props {
/**
* 체크박스의 라벨에 들어가는 텍스트입니다.
*/
labelText: string;
children: string;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

요,,요건 언젠가 수정이 필요한ㄷ,,

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

헉걱걱

/**
* 체크박스의 체크 여부입니다.
*/
Expand All @@ -18,16 +19,27 @@ interface Props {
* 체크박스 활성화 여부입니다.
*/
disabled?: boolean;
/**
* 체크 정보가 있는 context(문자열 배열)과 대조하기 위한 키 값입니다.
*/
value: string;
/**
* 체크 여부 변경 시 호출되는 함수입니다.
*/
onChange?: (e: ChangeEvent<HTMLInputElement>) => void;
}

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);

if (context) {
checked = context.selectedValue.includes(value);
onChange = context.onChange;
}
Comment on lines +35 to +38
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

onChangeFromProps와 onChangeFromContext 모두 event를 전달하여 동작하게끔 하는 건 어떨까요?

요게 이유가 Checkbox에 핸들러를 전달했는데, context에 핸들러가 있을 경우 무시하게 되는데, 개발자 입장에서 interface엔 onChange가 열려있는데 왜 동작을 안 하지?,, 라고 오해할 수도 있을 것 같아요!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

요렇게 할 시에 그러면 두 핸들러 모두 동작하게 하는건가여?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

네네! 두 핸들러 모두 동작해요!!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

난중에 설명좀 부탁드려요ㅠ 잘 몰루

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

오호 유틸함수 추가해주신걸로 주말에 샴푸님이랑 리팩토링해볼게용

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

와 이게 뭐에요


return (
<label className={cx('flex flex-row items-center gap-2', { 'opacity-[0.3]': disabled })}>
<input type="checkbox" className="hidden" checked={checked} onChange={onChange} />
<input type="checkbox" className="hidden" checked={checked} value={value} onChange={onChange} />
<div
className={cx(
' flex h-[20px] w-[20px] items-center justify-center rounded-md border-2 border-border-primary transition-colors duration-200 hover:border-color-system_200 dark:border-border-primary_dark dark:hover:border-color-system_200',
Expand All @@ -43,7 +55,7 @@ export function Checkbox({ labelText, checked = false, disabled = false, onChang
)}
/>
</div>
<Text text={labelText} size="body2" weight="regular" />
<Text text={children} size="body2" weight="regular" />
</label>
);
}
41 changes: 41 additions & 0 deletions src/components/Checkbox/CheckboxGroup.stories.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
import type { Meta, StoryObj } from '@storybook/react';
import { CheckboxGroup } from './CheckboxGroup';
import { Checkbox } from './Checkbox';

type Component = typeof CheckboxGroup;
const meta: Meta<Component> = {
title: 'rookies/CheckboxGroup',
component: CheckboxGroup,
};

type Story = StoryObj<Component>;

export const Primary: Story = {
args: {
selectedValue: ['label1', 'label3'],
children: (
<>
<Checkbox value="label1">children</Checkbox>
<Checkbox value="label2">children</Checkbox>
<Checkbox value="label3">children</Checkbox>
</>
),
},
};

export const WithDisabled: Story = {
args: {
selectedValue: ['label1', 'label3'],
children: (
<>
<Checkbox value="label1">children</Checkbox>
<Checkbox value="label2">children</Checkbox>
<Checkbox value="label3" disabled>
children
</Checkbox>
</>
),
},
};

export default meta;
19 changes: 19 additions & 0 deletions src/components/Checkbox/CheckboxGroup.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
import { ReactNode, ChangeEventHandler, createContext } from 'react';

interface Props {
children: ReactNode;
selectedValue: string[];
onChange?: ChangeEventHandler<HTMLInputElement>;
}

type CheckboxContextType = Omit<Props, 'children'>;

export const CheckboxContext = createContext<CheckboxContextType | null>(null);
Comment on lines +9 to +11
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@asdf99245, @GulSam00
context는 context.ts파일로 분리하는 것은 어떨까요?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

좋네요 👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

좋습니다

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

요거는 utils의 createContext 이용해서 분리하게 되는거죠? 일단은 냅둘게요...!


export function CheckboxGroup({ children, ...restProps }: Props) {
return (
<CheckboxContext.Provider value={{ ...restProps }}>
<fieldset className="flex flex-col gap-2">{children}</fieldset>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

나중에 row, col 선택할 수 있게 추가하는건 어떨까요?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

원 헌드레드 퍼센트 동의합니닷

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

저도 동의,,!

</CheckboxContext.Provider>
);
}
1 change: 1 addition & 0 deletions src/components/Checkbox/index.ts
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
export { Checkbox } from './Checkbox';
export { CheckboxGroup } from './CheckboxGroup';