-
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
Merged
Merged
Changes from 10 commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
4c5efe9
feat(Select): Select 컴포넌트를 서브 컴포넌트로 분리
suwonthugger 01ad51e
fix(Select): 불필요한 코드 수정
suwonthugger ab7a87f
feat(Select): Menu, MenuItem 컴포넌트 분리 및 options prop 삭제
suwonthugger 487ac6b
fix(Select): placeholder prop 위치 수정 및 불필요한 context 삭제
suwonthugger f9245a6
refactor(Select): onChange 함수 조건부로 변경
suwonthugger 6e6ed29
fix(Select): 함수 위치 일부 수정
suwonthugger 80bfddc
feat(Select): deprecated 폴더 추가 및 기존 컴포넌트 생성, jsdocs로 설명추가
suwonthugger 70b597f
remove(Select): displayName 제거 및 불필요한 prop 삭제
suwonthugger 45010c1
docs(Select): jsdoc @deprecated 명시
suwonthugger 496b3f0
Merge branch 'main' into feat/#138/seperate-select-menu
Brokyeom 5bac1fd
cs
Brokyeom File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
아니면 경고 정도로 좀 더 가벼운 워딩을 사용하는건 어떨까 싶네요!
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안에서 사용하더라도 trigger와 MenuItem이라는 서브 컴포넌트의 조합으로 사용하는 방식이 Select라는 컴포넌트의 기본동작을 보장하면서도 어느정도 custom 할수 있는 여지가 있어 충분하지 않나 하는 의견입니다. 다른 분들 생각도 궁금해요
만약 위 내용을 따른다면, 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 밖으로 빼야 한다고 잘못 이해했네요 😓
현재 코드에서 변경사항은 없어도 될 것 같습니다. 워딩도 충분하다고 생각이 되구요!