Conversation
|
🦋 Changeset detectedLatest commit: 3e8f831 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
wuzoo
force-pushed
the
feat/bottom-sheet
branch
2 times, most recently
from
3001a40 to
8c82c1c
Compare
constantly-dev
left a comment
constantly-dev
left a comment
There was a problem hiding this comment.
수고하셨습니다! 코드 깔끔한 것 같아요!
질문드리고 싶은 점이 구현하신 것처럼 제어/비제어의 경우를 모두 고려해 구현하신 이유가 사용 방법에 대한 선택을 더 많이 주기 위함인가요?
| export function BottomSheetTrigger({ children }: HTMLAttributes<HTMLButtonElement>) { | ||
| const { open, onOpenChange } = useBottomSheetContext(); | ||
|
|
||
| const handleOpenChange = () => { | ||
| onOpenChange(!open); | ||
| }; | ||
|
|
||
| return <div onClick={handleOpenChange}>{children}</div>; | ||
| } |
There was a problem hiding this comment.
trigger가 button의 역할을 하지 않는 경우 때문에 div로 두신 걸까요??
There was a problem hiding this comment.
네네 그냥 button으로 작성하고 radix에서 하다시피 asChild 옵션을 설정해두면 더 좋을 것 같긴한데 그거까지 고려하진 않았습니답
packages/ui/BottomSheet/index.tsx
Outdated
| return ( | ||
| open && ( | ||
| <div className={overlayStyle}> | ||
| {title && ( | ||
| <div className={titleWrapperStyle}> | ||
| {backIcon && <IconChevronLeft onClick={() => onOpenChange(false)} className={iconStyle} />} | ||
| <p className={titleTextStyle}>{title}</p> | ||
| </div> | ||
| )} | ||
| {children} | ||
| </div> | ||
| ) | ||
| ); |
There was a problem hiding this comment.
물론 open이 숫자 0이나 빈 문자열이 될 가능성이 없는 boolean이라 예상하지 못한 경우는 없어서 괜찮은데,
컴포넌트 조건부 렌더링(boolean)에서
- 현재처럼 &&를 사용 (
BottomSheetContent의 return 값이JSX.Element | false) - early return으로 null 반환 (
BottomSheetContent의 return 값이JSX.Element | null)
이 두개 중 어떤걸 선호하시나요?
There was a problem hiding this comment.
조건부 렌더링의 flag값이 boolean인 경우에 두 개 다 혼용해서 사용하는 것 같아요.
개선 결과로는 content에 createPortal를 활용하였어요. 첫 번째 인자로 넘겨지는 JSX에서 open에 따른 조건부 렌더링을 사용하여 BottomSheetContent의 리턴값은 ReactPortal 입니다
packages/ui/BottomSheet/index.tsx
Outdated
| const uncontrolled = _open === undefined; | ||
|
|
||
| const open = uncontrolled ? internalOpenValue : _open; | ||
| const handleOpenChange = (value: boolean) => { | ||
| if (uncontrolled) { | ||
| if (value) { | ||
| internalOpen(); | ||
| } else { | ||
| internalClose(); | ||
| } | ||
| } else { | ||
| onOpenChange?.(value); | ||
| } | ||
| }; |
There was a problem hiding this comment.
물론 지금도 좋지만 uncontrolled가 boolean 값을 가지니 일반적인 is- prefix를 써서 isControlled로 사용하고 조건을 반대로 해도 가독성이 좋을 것 같습니다.
There was a problem hiding this comment.
is- 붙히는걸로 수정했어요. 반대조건을 굳이 추가해주지 않아도 가독성에 문제 없을 것 같아서 요건 유지하겠습니다!
| const meta: Meta = { | ||
| title: 'Components/BottomSheet', | ||
| tags: ['autodocs'], | ||
| }; | ||
| export default meta; | ||
|
|
There was a problem hiding this comment.
component: BottomSheetExample를 meta에 지정하지 않아서 args table이 자동으로 안뜨는 것 같습니다!
추가로 compound component pattern이라 단순하게 스토리 적용이 안된다면 subcomponents 등의 옵션이나 복잡한 컴포넌트 관련해서 옵션들을 적용해봐야 할 것 같아요!
wuzoo
force-pushed
the
feat/bottom-sheet
branch
from
8c82c1c to
57ce2a5
Compare
wuzoo
force-pushed
the
feat/bottom-sheet
branch
from
3eb5528 to
3e8f831
Compare
2 participants
변경사항
바텀싯 컴포넌트를 구현하였습니다.
링크
시급한 정도
🏃♂️ 보통 : 최대한 빠르게 리뷰 부탁드립니다.
기타 사항
커밋 보시면서 리뷰해주시면 더 편하실 거에요