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: useDisclosure 훅 구현 #22

Merged
merged 4 commits into from
Jun 6, 2024
Merged

feat: useDisclosure 훅 구현 #22

merged 4 commits into from
Jun 6, 2024

Conversation

dev2820
Copy link
Contributor

@dev2820 dev2820 commented May 31, 2024

useDisclosure 훅은 modal, disclosure 등의 열림/닫힘 상태를 가지는 컴포넌트를 제어하기 위해 생성된 커스텀 훅입니다. 자세한 설명은 storybook을 참고해주세요

Copy link
Member

@d0422 d0422 left a comment

Choose a reason for hiding this comment

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

@dev2820
안녕하세요! 기여해주셔서 감사합니다!
오 그런데 useDisclosure의 코드가 toggle을 제외하면 대부분 useSwitch와 동일한데요, 함수들의 역할이 비슷한 것 같습니다. useSwitch로 통일하는 것은 어떨까요?

image

@dev2820
Copy link
Contributor Author

dev2820 commented Jun 3, 2024

@d0422 의도적으로 나눈 것이긴 합니다. 스위치는 useSwitch를, 디스클로저엔 useDisclosure를 각각 사용하도록이요. 두 훅 모두 useBoolean을 컴포넌트에 맞춰 해석하고 있습니다. chakra의 useBoolean과 useDisclosure와 유사합니다

허나 라이브러리의 방향성이 심플함이나 가벼움이라면 useBoolean으로 통일하는 쪽은 괜찮을 것 같습니다. 어떻게 생각하시나요?

@d0422
Copy link
Member

d0422 commented Jun 3, 2024

@dev2820
의도를 설명해주셔서 감사합니다.
제가 생각하는 방향성은 추상화된 형태로 개발자에게 간편하게 맞춤 기능을 제공하는 라이브러리인데요... 이렇게 적고보니 더 모호한 것 같습니다.

구현부가 거의 겹치게 되어 말씀을드렸던 것인데, 남겨주신 의견을 통해 다시 생각해보니 확실히 useSwitch와 useDisclosure훅이 분리된 형태로 제공되는 것이 라이브러리 사용자(개발자)가 기능을 구현할때 더 유용할 것 같다는 생각이듭니다. 구현부가 같아도, 메서드 이름이 다르면 맥락이 완전히 달라질 수 있으니까요!

남겨주신 chakra의 useDisclosure 코드를 참고해보았는데요, 해당 훅에서는 getDisclosureProps, getButtonProps 함수를 통해 훅의 state를 변경하는 함수를 사용하지 않고도 간편하게 관리할 수 있게 구현이되어있더라구요.
이 라이브러리도 위처럼 훅에 맞춤 기능을 지속적으로 업데이트해가는게, 제가 생각하는 의도에도 맞을 것 같다는 생각입니다.

좋은 의견과 기여 감사합니다! 코드는 통합하지 않아도 좋을것 같아요!

@dev2820
Copy link
Contributor Author

dev2820 commented Jun 4, 2024

저도 PR을 너무 대충올린 것 같아 뜨끔하네요. 의도 설명도 없이 코드만....주의하겠습니다.

@d0422 님의 의견과 방향성 말해주셔서 감사합니다. 다음에 기여할때 참고할게요.

@d0422
Copy link
Member

d0422 commented Jun 4, 2024

@dev2820
아닙니다 기여해주셔서 감사하고, 의견나눌 수 있어 좋은 걸요 😁
덕분에 방향성에 대해 더 고민해볼 수 있어서 좋았습니다!
괜찮으시면 PR머지하도록할게요! 이모지 남겨주시면 머지하겠습니당

@d0422 d0422 merged commit 6813cac into Rapiders:main Jun 6, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants