-
Notifications
You must be signed in to change notification settings - Fork 3
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(Select): Select를서브 컴포넌트로 분리하여 구성 #144
Conversation
|
|
고생하셨어요! 제가 생각한 것 보다 더 많은 것들을 변경해 주셨군요 ㄷㄷ 다만 마음에 걸리는 점은 말씀 해 주셨던 것 처럼 이전 방식과 하위 호환성이 깨지게 되는 문제가 발생하는데요, |
본문에 이 이슈만 해결하면 좋을 것 같은데, Radix자체에서 보내는 에러라면 어떻게 할 수 있을지 고민해 봐야 할 것 같아요 |
해당 에러는 제가 의도치 않은 동작을 방지하기 위해 임의로 에러를 추가한 부분인데요. 현재 Ref를 써서 모달 닫히고 열리는 동작에서의 로직이 서브 컴포넌트 사이 사이 연관되어있어서, root안에서 context를 내려받을 때에만 사용할수 있게 처리해놨습니다. (mds 드롭다운 UI의 기본동작)
해당 부분 에러를 제거하고, root 외부에서도 단순 UI 용도로 사용할 수 있게 사용하게끔 하는게 더 좋을것 같다는 의견이신가요~?! |
throw new Error('Select 컴포넌트는 Select.Root 내에서 사용되어야 합니다.'); | ||
} |
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.
여기 있었군요!
해당 부분 에러를 제거하고, root 외부에서도 단순 UI 용도로 사용할 수 있게 사용하게끔 하는게 더 좋을것 같다는 의견이신가요~?!
음.. 에러처리를 할 만큼 엄격하게 제한하지 않아도 된다는 생각이에요
이미 일부 컴포넌트를 분리해서 사용하도록 만드는 것이 해당 작업의 요지인만큼 작업자들이 의도대로 사용했는데 에러가 발생한다면 작업에 제한이 생길 것이라 생각이 들구요
이 부분에 대해서는 스토리북 문서 보강등을 통해 정책으로 풀어나가야 할 것 같습니다!
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.
음... 저도 사실 문서화를 통해 사용법만 잘 안내된다면 Root 밖에서 사용하는 케이스는 드물 것 같다는 생각이 들어요.
하지만 이렇게 커스텀 에러 처리를 하는 것도 좋아보여요!! Root 밖에서 사용한 케이스에서 에러 처리를 하지 않으면 Context Provider가 존재하지 않는다는 에러가 발생할 것으로 예상이 되는데요, 합성 컴포넌트 사용 패턴에 익숙치 않은 메이커분들이 해당 에러를 마주하면 충분히 혼란스러울 수 있기에 에러 해결 방안을 메시지로 제공하는 것도 좋을 것 같다는 생각이 듭니다!
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.
에러를 처리한 이유에 대해서 설명드리자면, 정우 말대로, root 밖에서 서브 컴포넌트를 사용했을 경우 context로 내려받을 상태나 함수 등이 없어 실제 에러가 발생합니다. 그래서 커스텀 에러로 어떤 에러인지 명시해주는게 나을것 같다고 생각하여 에러를 적용해주었습니다.
- root 컴포넌트의 밖에서 사용했을 때, 에러가 뜨지 않게 하고, 독립적인 사용을 해야할까에 대한 고민이 관건이네요..!
흠 저는 root안에서 사용하더라도 trigger와 MenuItem이라는 서브 컴포넌트의 조합으로 사용하는 방식이 Select라는 컴포넌트의 기본동작을 보장하면서도 어느정도 custom 할수 있는 여지가 있어 충분하지 않나 하는 의견입니다. 다른 분들 생각도 궁금해요
root 컴포넌트의 밖에서 사용했을 때, 에러가 뜨지 않게 하고, 독립적인 사용을 해야할까에 대한 고민이 관건이네요..!
만약 위 내용을 따른다면, context를 받아 올 수 없는 경우
, 있는 경우
로 분기하여 context 관련 로직을 모두 조건부로 처리해서 구현하는 방법이 떠올랐는데, 어떻게 구현하면 좋을지 다양한 의견을 듣고싶어요!
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.
좋습니다. 저도 다시한번 읽어보니 잘못 이해한 부분도 있었네요!
root 안에 Menuitem과 Trigger를 넣어줌으로써 저희가 의도한 커스텀 트리거 ui를 만들 수 있는데, 분리하기 위해선 root 밖으로 빼야 한다고 잘못 이해했네요 😓
현재 코드에서 변경사항은 없어도 될 것 같습니다. 워딩도 충분하다고 생각이 되구요!
Co-authored-by: HyeongKyeom Kim <97586683+Brokyeom@users.noreply.github.com>
이것은 Storybook 에 케이스별로 문서를 만드는게 좋겠다는 생각이에요. 스토리북은 브랜치 분리해서 가시죠 |
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.
좋습니다! 꼼꼼하게 작업해주셔서 감사해요
bump는 minor로 갈게요!
변경사항
radix-ui의 컴포넌트 구성 방식을 참고하여 Select 컴포넌트를 리팩토링 했어요.
https://github.com/radix-ui/themes/blob/main/packages/radix-ui-themes/src/components/dropdown-menu.tsx
변경된 Select의 구성은 다음과 같아요
Select.Root
Select.Root에서 사용자는 prop으로 데이터를 전달해주게돼요. Select.Root는 전달받은 데이터를 바탕으로 정의된 함수 상태들을 자식요소에게 내려주는 역할을 해요. (
context
를 사용했습니다.)Select.Trigger
Select.Trigger는 Select.Menu를 열고 닫는 트리거의 역할만을 해요.
children
을 받아 직접 custom한 UI도 trigger 역할을 할 수 있게 하여 유연성을 높였어요.Select.TriggerContent
Select.TriggerContent는 Select.Trigger에 쓰이는 단순 기존 UI에요. 편의를 위해 만들었어요.
Select.Menu
드롭다운 메뉴를 감싸주는 UI
Select.MenuItem
드롭다운 메뉴를 안에서 사용할 Item 요소에요.
dropdown 아이템 하나
를 클릭하면onChange 함수
를 통해서 현재 선택된 option을 변경해주게 되어있었어요, 즉 onClick함수에 현재 선택된 메뉴를 변경하는 기능만 한정되어있었죠.위와 같은 요구사항을 반영하려면, Item 요소에 조건부로 onClick 함수를 받아, 현재 선택된 option 을 변경하는 기능 외 추가적으로 커스텀 로직을 동작할수 있게 해야한다고 생각했어요. 그래서 추가 동작을 하는 onClick 을 조건부로 Select.MenuItem prop에 추가했어요.
고민점
trigger 컴포넌트 생성, 단순히 dropdown과 상단의 선택된 메뉴를 보여주는 UI(?)만을 분리한다면, trigger를 위한 동작을 위해 라이브러리를 사용하는 사용자가 함수 등을 정의해주는 번거로움이 있을거라고 생각했어요. 그래서 trigger 컴포넌트를 분리하였고, children으로 UI를 받아 headless하게 열고 닫는 동작을 할 수 있게 했습니다.
Menu 컴포넌트, 보통 UI 라이브러리들의 리스트 컴포넌트를 보면, 리스트를 감싸는 wrapper, 리스트 요소 하나에 해당하는 Item 컴포넌트를 따로 분리하게 되어있어요. 이런 구성은 wrapper와 item의 ui나 레이아웃이 변경되었을 때 유연하게 대처할수 있다는 장점이 있을것 같습니다. 그렇지만 기존 컴포넌트와 사용방식에 차이가 나게되는데요, 이에 대한 의견이 궁금해요.
상태와 함수, 현재는 상태와 함수는 거의 정리하지 않았어요. 리소스 상, 기존 로직을 유지하고 상태와 함수 개선이 꼭 필요한 부분이 생겼을 때 정리하는게 좋을것 같다는 생각이 들었어요.
new_select1.mov
new_select2.mov
링크
https://sopt-makers.slack.com/lists/T040QGZF77H/F07LCJ586TS?record_id=Rec07MHJ4LQ9J
시급한 정도
🏃♂️ 보통 : 최대한 빠르게 리뷰 부탁드립니다.
기타 사항