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

[Feature] Checkbox 컴포넌트 구현 #36

Merged
merged 30 commits into from
Jun 7, 2024
Merged

Conversation

SeieunYoo
Copy link
Collaborator

@SeieunYoo SeieunYoo commented Jun 1, 2024

🎉 변경 사항

  • Checkbox 컴포넌트를 구현합니다.
  • input 과 관련된 prop(aria-label...) 등을 받는 inputProps 를 추가합니다.
    image

🚩 관련 이슈

🙏 여기는 꼭 봐주세요!

  • 아이콘 만드는 스크립트에서 아이콘 svg 에서 기본 width, height, viewBox 를 가져와서 기본 propsString 으로 들어갈 수 있도록 수정했습니다! (아이콘의 width height 가 24가 아닌 경우가 있어서용)
  • Checked 관련 상태 관리하는 로직이 Switch 컴포넌트랑 겹치는 게 많아서 useCheckedState 라는 훅으로 분리해서 체크박스 컴포넌트에는 적용해보았는데 괜찮다면 Switch 컴포넌트에도 적용해보겠습니닷
  • test 도는 과정에서 wow-icons 없다는 에러가 떠서 build 해주는 과정도 추가했습니다 d4e23e9

🔗참고자료

Copy link
Contributor

github-actions bot commented Jun 1, 2024

@SeieunYoo SeieunYoo marked this pull request as ready for review June 1, 2024 18:33
Copy link
Contributor

github-actions bot commented Jun 1, 2024

@SeieunYoo SeieunYoo self-assigned this Jun 1, 2024
Copy link
Contributor

github-actions bot commented Jun 1, 2024

Copy link
Collaborator

@ghdtjgus76 ghdtjgus76 left a comment

Choose a reason for hiding this comment

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

굿굿 수고 많으셨어요!
제가 보기엔 고칠 게 많이 없는 거 같네요.. 👍

아래 몇 가지만 추가로 해주시면 좋지 않을까 해요!

  1. 루트 styled-system 폴더 삭제
  2. icons 패키지 UpArrow 삭제 (예시로 넣어둔거라...)

그리고 Switch 컴포넌트에도 useCheckedState 훅 적용해보신다고 하셨는데, Checkbox props 보니까 몇 가지 추가하면 좋은 게 있는데 제가 그거 추가하면서 같이 적용해봐도 괜찮을까요?

packages/wow-ui/src/components/Checkbox/index.tsx Outdated Show resolved Hide resolved
packages/wow-ui/src/hooks/useCheckedState.ts Outdated Show resolved Hide resolved
packages/wow-ui/src/components/Checkbox/index.tsx Outdated Show resolved Hide resolved
packages/wow-ui/src/components/Checkbox/index.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

github-actions bot commented Jun 2, 2024

Copy link
Contributor

github-actions bot commented Jun 2, 2024

@SeieunYoo
Copy link
Collaborator Author

@ghdtjgus76 리뷰 전부 반영해두었습니당~ Switch 컴포넌트는 따로 수정하지 않을게욥!

Copy link
Collaborator

@ghdtjgus76 ghdtjgus76 left a comment

Choose a reason for hiding this comment

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

굿굿 👍

Copy link
Member

@hamo-o hamo-o left a comment

Choose a reason for hiding this comment

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

수고하셨습니다!!

++ 요런 식으로 제 맘대로 묶어서 쓸 때는 상태 관리가 각각 안되는 것 같은데, 여러 개 사용하기 위해서는 Group 컴포넌트가 필수적으로 사용되어야 하는거 맞을까요?

image

packages/wow-ui/src/components/Checkbox/index.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

github-actions bot commented Jun 7, 2024

Copy link
Contributor

github-actions bot commented Jun 7, 2024

@SeieunYoo
Copy link
Collaborator Author

리뷰 전부 반영해두었습니다!!
onKeyDown 이나 onMouseDown 이벤트핸들러에 pressed 관련된 로직도 넣어야 될 것 같아서 useCheckedState 에서 pressed 상태도 추가로 관리하도록 수정해두었어요 Switch 컴포넌트에 적용하시게 된다면 참고 부탁드립니당 6defba6 cc. @ghdtjgus76

@SeieunYoo SeieunYoo requested review from hamo-o and ghdtjgus76 June 7, 2024 07:36
@ghdtjgus76
Copy link
Collaborator

넵 좋습니다!
다만, 현영님이 말씀하신 것처럼 Checkbox를 여러 개 가져다 놓고 쓰는 경우 제대로 상태 관리가 안 되는 문제가 있는데 아마 id 값이 다 동일해서 그렇지 않을까 싶어요
useId 훅 써서 이 부분만 수정하면 될 거 같아요!

Copy link
Contributor

github-actions bot commented Jun 7, 2024

@SeieunYoo
Copy link
Collaborator Author

++ 요런 식으로 제 맘대로 묶어서 쓸 때는 상태 관리가 각각 안되는 것 같은데, 여러 개 사용하기 위해서는 Group 컴포넌트가 필수적으로 사용되어야 하는거 맞을까요?

아항 CheckBox 도 그룹 형태로 쓰려면 SwitchGroup PR 에서 만드신 Group 컴포넌트를 사용해서 감싸주어야 합니다!
체크박스 여러 개 있을 때 id 는 중복되면 안될 것 같아서 고유 id 를 가지도록 말씀대로 수정해두었어용 ed65dff

Copy link
Member

@hamo-o hamo-o left a comment

Choose a reason for hiding this comment

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

LGTM 🤸‍♂️

@SeieunYoo SeieunYoo merged commit ffb0d6a into main Jun 7, 2024
3 checks passed
@SeieunYoo SeieunYoo deleted the feature/check-box-component branch June 7, 2024 12:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants