feat: implement bottom sheet component #19
Conversation
- implement bottom sheet component using framer-motion - implement useMeasure hook - implement useWindowSize hook
|
The latest updates on your projects. Preview: https://vitamin-c-p8p55x780-poiu694s-projects.vercel.app
|
There was a problem hiding this comment.
고생 많으셨습니다! 접근성 관련해서 몇 가지 코멘트 남겼어요. 이건 접근성 관련해서 참고해볼만한 글입니다~!
https://www.inthepocket.design/articles/improving-the-accessibility-of-bottom-sheets
| onPointerDown={(e) => dragControls.start(e)} | ||
| > | ||
| {/* bar */} | ||
| <div className="w-[53px] h-[6px] bg-[#6D717A] my-0 mx-auto rounded-[999px]" /> |
There was a problem hiding this comment.
드래그앤 드랍을 사용하는 경우에는 이를 대체할 수 있는 버튼이나 기능이 있어야 해요.
이 bar 부분이 아래 부분라면
role="button" aria-label={open ? "닫기" : "열기" } 와 같이 대체 기능을 적용하는 걸 추천드려요.
그런데 드래그 앤 드랍이랑 기능이 겹칠 수도 있으니 이 부분을 체크하고 적용해주세요!
There was a problem hiding this comment.
디자인으로 어떻게 풀어낼 수 있을지 고민해보겠습니다!
| return ( | ||
| <> | ||
| {/* background overlay */} | ||
| <motion.div |
There was a problem hiding this comment.
이 경우 bottom sheet 이외에 영역에도 키보드나 스크린리더로 접근이 가능할 것 같아서 그 부분을 막을 방법을 생각해보시면 좋을 것 같아요!
https://nuli.navercorp.com/community/article/1133093
There was a problem hiding this comment.
지도의 경우 바텀시트가 활성화되어도 배경이 그대로 보일뿐만 아니라, 클릭도 바텀시트가 닫혀있을 때와 마찬가지로 동작해야 하는데, 어떤 부분을 막아야 하는 걸까요?
There was a problem hiding this comment.
재석님이 이런 케이스를 얘기하신건지 잘 모르겠지만 제가 이해되기로는 bottom sheet가 실제 눈에 보이지 않을때도 DOM에 존재를 하니 screen-reader에 읽히거나 키보드의 tab 동작으로 접근이 가능하니 현재 bottom-sheet에서 구현하신 실제 UI적인 부분말고 그거와 분리하여 bottom-sheet가 현재 열려있는지에 대한 상태를 관리하여 해당 상태를 이용해 aria-hidden이나 tabIndex를 컨트롤 하면 좋을거 같다라는 생각이 드네요 (상태를 추가할지 기존 상태를 활용할지는 상황에 맞게!!)
해당 코드는 bottom sheet는 아니지만 위에서 말씀드린 접근성 관련 처리를 한 Toggle 가능한 Tab Component에서 aria-hidden과 tabIndex를 조절했던 코드입니다! 코드가 짧으니 간단히 참고 정도 해도 괜찮을듯 해요ㅋㅋㅋㅋ
https://github.com/mash-up-kr/mash-up-recruit-fe/blob/main/src/components/common/MyPageTab/MyPageTab.component.tsx
There was a problem hiding this comment.
업로드한한 사진과 같이 바텀시트가 열려있는 경우에는 지도에 접근이 불가능해져야 돼요! 왜냐하면 바텀시트를 활성화 한 이유는 해당 바텀시트의 내용을 확인하기 위해 열었기 때문이죠. 그리고 대부분의 UI 상으로도 바텀시트 외부 요소를 클릭하게 되면 자동으로 바텀시트는 닫히도록 설정되어 있어요. 만약 바텀시트가 아닌 지도나 바텀시트 외부 요소에 접근하기 위해선 이미지 우측상단에 닫기 버튼 처럼 닫아야 사용이 가능하도록 바꿔야 할 것 같네요!
There was a problem hiding this comment.
- 이렇게 제한된 기능만을 구현하는 데에도 시간이 꽤 소요되는데, radix나 shadcn 같이 잘 만들어져있는 라이브러리를 사용하는 게 나으려나... 라는 생각이 꽤나 여러 번 들었습니다. 😇
- 직접 구현에 대한 여러분의 생각이 궁금합니다. 💬
당연히 서비스를 만들때의 공수 측면으로는 잘 구현해놓은걸 쓰는게 좋다고 생각은 드나 직접 구현함으로써 얻을 수 있는 이점도 많다고 생각이 듭니다.
지금 생각되는건 당연히 라이브러리를 쓰지 않으니 번들 자체가 가벼워지는 이점도 있을거 같은데 사실 이건 framer-motion이 붙음으로써 그렇게까지 크게 다가오지는 않는거 같아요ㅋㅋㅋㅋ framer-motion없이 구현해봤으면 더 좋았을것도 같은데 이건 팀원들과의 선택의 문제이니~~
근데 사실상 더 큰 이점은 이런 UI Component들 구현하다보면 얻는 인사이트들이 굉장히 많은거 같아요, 해보지 않으면 감이 잘 오지 않는 UI Component들의 구현 방식과 설계 방식에 대해 생각할 수 있게 되는거 같고 그러다보면 생긴건 다른 UI여도 내부 로직 입장에서보니 비슷한 패턴이라면 구현이 가능할것 같은데? 에 대한 판단도 되고 그거 자체가 사고가 넓어진거라고 생각을 합니다 :)
라이브러리를 쓰지 않는다는건 당연히 공수를 더 많이 들여야하고 이슈도 많을 수 있을 수 있지만 그만큼 본인이 시간을 많이 쓰려고 하는거 같아서 본인과 팀이 감당할 수 있는선에서는 좋은거 같습니다 (이 선이 명확하지 않지만은ㅋㅋㅋㅋ) 근데 바텀시트 정도의 UI Component 단위들은 사실 그 선 안에 들어오지 않나? 싶긴하네요👍
❓아 그리고 이제 Pn 룰 안쓰시나요? 최근 리뷰 보니까 Pn룰이 없네요ㅋㅋㅋㅋ❓
| onDragEnd={(event, info) => { | ||
| const offsetThreshold = 150 | ||
| const deltaThreshold = 5 | ||
|
|
||
| const isOverOffsetThreshold = | ||
| Math.abs(info.offset.y) > offsetThreshold | ||
| const isOverDeltaThreshold = Math.abs(info.delta.y) > deltaThreshold | ||
|
|
||
| const isOverThreshold = isOverOffsetThreshold || isOverDeltaThreshold | ||
|
|
||
| if (!isOverThreshold) return | ||
|
|
||
| setIsOpen(info.offset.y < 0) | ||
| }} |
There was a problem hiding this comment.
뷰단이랑 로직단을 완벽히 구분할 수는 없겠지만 이 경우에는 그래도 함수 몸체에서 기명 함수로 동작을 정의하고 JSX에서는 해당 함수를 할당만 해주는식으로 사용하는건 어떠신가요~?
| {/* header */} | ||
| <div | ||
| className="h-[80px] pt-[12px] cursor-grab select-none" | ||
| onPointerDown={(e) => dragControls.start(e)} |
There was a problem hiding this comment.
타입 이슈만 없다면 이렇게 사용하면 콜 스택이 하나 덜 쌓일거 같네요 :)
| onPointerDown={(e) => dragControls.start(e)} | |
| onPointerDown={dragControls.start} |
There was a problem hiding this comment.
이 경우에는 이벤트를 넘겨줘야 dragControls가 정상 작동해서 이렇게 작성해두었습니다~!
There was a problem hiding this comment.
dragControls.start 자체가 함수라고 한다면 제가 제안한대로 작성하면 event는 함수로 전달되긴 합니다
onPointerDown에서 dragControls.start를 아래 수도코드처럼 호출한다고 생각하면 될거 같네요 :)
function onPointerDown(dragControls.start) {
dragControls.start(event);
}There was a problem hiding this comment.
start 함수는 이렇게 작성되어있고,
말씀해주신대로 작성하면 에러가 발생하는데, event가 전달되는 경우와 전달되지 않는 경우가 어떻게 나뉘는 건지 더 찾아봐야겠습니다...!
There was a problem hiding this comment.
일단 제가 제안드린대로 넘겨보면 event는 전달이 잘 되는데 (캡쳐 해주신건 코드를 어떻게 작성하신지 보여주시면 좋을거 같네요) this 바인딩 이슈 때문에 this가 undefined를 참조하게 되어서 componentControls를 참조하지 못해 에러가 나네요
어쩔 수 없이 원래 작성하신대로 둬야할거 같습니다ㅎㅎ
DragControls의 start 함수는 아래처럼 작성되어 있습니당
start(event, options) {
this.componentControls.forEach((controls) => {
controls.start(event.nativeEvent || event, options);
});
}아니면 이렇게도 쓸 수는 있겠지만 굳이...?ㅋㅋㅋㅋ
<div
className="h-[80px] pt-[12px] cursor-grab select-none"
onPointerDown={dragControls.start.bind(dragControls)}
>
src/hooks/use-measure.ts
Outdated
| type Result = [ | ||
| (element: HTMLOrSVGElement | null) => void, | ||
| RectReadOnly, | ||
| () => void, |
There was a problem hiding this comment.
여긴 VoidFunction으로도 사용이 가능할거 같네요! 근데 아마 이것도 라이브러리 코드를 그대로 쓴거 같아서 가벼운 의견 정도로만 생각해주세요ㅋㅋㅋㅋ
There was a problem hiding this comment.
VoidFunction을 사용해본 적이 없고, 정보가 적은데 보통 언제 사용하는지 알려주실 수 있나요?
저 부분을 VoidFunction으로 변경해주면, handleChange의 리턴도 VoidFunction으로 변경해줘야 하는데, 변경해주면 에러가 발생해서 용례를 알고 싶습니다!
Cf. microsoft/TypeScript#58099 Function lacks ending return statement and return type does not include 'undefined'
There was a problem hiding this comment.
type A = () => void;
type B = VoidFunctionThere was a problem hiding this comment.
상민님 작성해주신대로 그냥 () => void를 선언적으로 표현하기 위해 type으로 alias를 한번 해준 느낌으로 생각하면 될 듯 합니당
첨부하신대로 쓰면 useCallback의 callback function에 void function을 준것이니 맞지 않는것 같고 딱 리뷰한쪽에만 써도 될 것 같아요! (사실 첫 리뷰에 얘기했듯이 이게 더 선언적이고 깔끔하다고 생각이 드신다면 적용하면 되는 P3 정도의 의견입니다ㅋㅋㅋ)
| const [windowSize, setWindowSize] = useState<Size>({ | ||
| width: MIN_WIDTH, | ||
| height: MIN_HEIGHT, | ||
| }) | ||
|
|
||
| useEffect(() => { | ||
| const handleResize = () => { | ||
| setWindowSize({ | ||
| width: window.innerWidth, | ||
| height: window.innerHeight, | ||
| }) | ||
| } |
There was a problem hiding this comment.
지금 당장 해당 hook을 완벽하게 만들자는 아닌거 같긴한데 초기값은 MIN_WIDTH, MIN_HEIGHT인데 실제로 resize 이벤트를 발생시키다보면 MIN 값 보다 더 작은 값도 상태값으로 할당이 될 수 있어보이네요, 만약 지금의 동작이 의도된거라고 한다면 MIN_WIDTH, MIN_HEIGHT보단 INITIAL_WIDTH, INITIAL_HEIGHT라는 이름이 더 적절할거 같아요!
근데 만약 지금의 동작을 의도한게 아닌 진짜 MIN값을 유지하려는게 의도였다면 handleResize 함수에서 min값에 대한 예외처리를 해주면 좋을거 같습니다 :)
There was a problem hiding this comment.
저희가 지원할 값들 안에서 resizing을 제한하기 위한 목적이어서, 예외 처리를 해줘야 할 것 같습니다!
| }) | ||
| } | ||
|
|
||
| window.addEventListener('resize', handleResize) |
There was a problem hiding this comment.
resize 이벤트는 이벤트 발생 빈도가 너무 높기 때문에 throttle을 적용하여 대략 200ms 내외의 간격으로만 이벤트를 발생시켜도 충분하지 않을가 싶기도 하네요 :) handleResize는 state를 변경하여 리렌더를 일으키기도 하니 UX에 영향이 없는 범위 정도로 throttle 해주면 어떨까합니다!
해당 리뷰를 남기는 라인이 handleResize 선언부가 맞을수도 있긴한데 일단 이벤트 add 하는 부분에서 생각이 들어서 여기에 남깁니다ㅋㅋㅋㅋ
There was a problem hiding this comment.
아! react-use-measure에서 debounce 처리를 해뒀던데, 이러한 이유였겠네요. use-measure랑 같이 throttle 이든, debounce든 처리해보겠습니닷~!
Cf. https://github.com/pmndrs/react-use-measure/blob/master/src/web/index.ts#L70
There was a problem hiding this comment.
넵 scroll이랑 resize는 이벤트 발생빈도가 잦아서 정말 꼭 필요한(의도한) 경우를 제외하고는 최적화를 한번 해주는게 좋은거 같더라고요 :)
| pointerEvents: 'none', | ||
| }, | ||
| }} | ||
| onTap={() => setIsOpen(false)} |
There was a problem hiding this comment.
아래는 onPointerDown이고 여기서는 onTap인데 혹시 이유가 있을까용?
There was a problem hiding this comment.
배경과 바텀시트 영역에서 각각 기능이 다른데, 각 기능에 가장 최적화되어있다고 생각한 콜백을 사용했습니다!
- 배경은 탭(
onTap)했을 때 닫기
Cf. https://www.framer.com/motion/gestures/#tap - 바텀시트 영역 내부에서 바텀시트를 조절할 때는 드래그
framer-motion 문서에서 dragControls에는 onPointerDown을 사용
| state.current.resizeObserver = new ResizeObserver(handleChange) | ||
| state.current.resizeObserver!.observe(state.current.element) |
There was a problem hiding this comment.
assert로 하신 이유가 궁금합니다~! 아래처럼 if로 감싸지 않은 이유가 있을까요~>!
There was a problem hiding this comment.
!.를 검색해보니 Nullish coalescing operator (??)의 축약이라고 하는 거 같은데 이걸 assert라고 부르는군요?!
아래는 어차피 scroll 여부도 확인해야 해서 if문으로 감쌌는데, 이 경우에는
if (state.current.resizeObserver) {
state.current.resizeObserver.observe(state.current.element)
}
보다 !.로 나타내는게 훨씬 간결해서 저렇게 사용한 것 같네요. 근데 이해하기 편하기 위해 if 문으로 감싸야겠습니다!
There was a problem hiding this comment.
null이나 undefined 될 수 있는 부분을 강제로 아니라고 하는 문법이에요! as 키워드로 강제했을 때 타입이 오류날 수 있듯이, assertion도 확실한 부분이 아니라면 안 쓰는게 런타임 디버깅에서 좋습니다~
There was a problem hiding this comment.
사실 원본 코드에서는 ResizeObserver가 없는 경우가 발생하지 않게끔 예외처리를 해줬는데,
declare type ResizeObserverCallback = (
entries: any[],
observer: ResizeObserver,
) => void
declare class ResizeObserver {
constructor(callback: ResizeObserverCallback)
observe(target: Element, options?: any): void
unobserve(target: Element): void
disconnect(): void
static toString(): string
}
...
const ResizeObserver =
polyfill || (typeof window === 'undefined' ? class ResizeObserver {} : (window as any).ResizeObserver)
if (!ResizeObserver) {
throw new Error(
'This browser does not support ResizeObserver out of the box. See: https://github.com/react-spring/react-use-measure/#resize-observer-polyfills'
)
}
타입 에러가 발생해서 해당 부분을 제거하는 바람에 의문이 생기는 코드가 되어버린 것 같네요...! 지금 amolang 터져라 코드가 되어버린 것 같은데 ResizeObserver를 처리하는 좋은 방법이 있을까요? 😢
There was a problem hiding this comment.
일단 useMeasure 상단에 error throw하는 것만 추가할까 싶기도 합니다!
const useMeasure = (
{ scroll, offsetSize }: Options = {
scroll: false,
offsetSize: false,
},
): Result => {
if (!ResizeObserver) {
throw new Error(
'This browser does not support ResizeObserver out of the box. See: https://github.com/react-spring/react-use-measure/#resize-observer-polyfills',
)
}
...
}There was a problem hiding this comment.
오잉 저 Type Error는 중복된 변수명 방지 아닌가여?
// 혹은 다른 변수명
const resizeObserver = typeof === .... ResizeObserver 이런식으로는 안되나요?
There was a problem hiding this comment.
저거 다시 보는데 polyfill을 신경써줘야 할 수도 있겠네요..? 🤔
There was a problem hiding this comment.
그렇게 하면 위 에러가 발생해서 다음과 같이 수정해야 하는데,
const resizeObserver =
typeof window === 'undefined'
? ResizeObserver
: (window as any).ResizeObserver기존 코드가 의도했던 class를 override하는 듯한 기능이 동일하게 작동하는지가 확실하지 않아서요!
There was a problem hiding this comment.
react-use-measure 소스 코드에 있던 polyfill도 없애고 debounce도 없앴었는데, 이 정도면 그냥 저 라이브러리를 갖다쓰는게 맞는 것 같다는 생각도 스멀스멀... 드는군요... 🫠
이 부분은 사실 해보고 싶은 부분이면 구현을 직접 해 보는게 사이드 플젝의 묘미 아닐까 싶어요! 처음부터 라이브러리에서 사용하는 것처럼 완벽하기엔 어려우니까, 플젝에서 만들어보고 테스트를 통해서 디벨롭해보는 것도 재미중에 하나라고 생각하거든요. 좋은 아이디어가 생긴다면 기존 라이브러리에 기여해볼 수 있는 경험도 생기고요! 대신에 너무 매몰되지만 않으면 좋을 것 같아요. 많은 부분을 신경써야 하니까 기간 산정이나 개발을 진행할 때 주의만 하면 좋을 것 같아요! 고도화하다가 힘들겠다 싶으면 라이브러리로 바꾸어도 좋습니다! 추가로 사용성에 대한 부분이나 그런건 메신저나 피그마보면서 얘기했던 부분 더 봐도 좋을 것 같아요!~ |
|
The latest updates on your projects. Preview: https://vitamin-c-5ptviglk7-poiu694s-projects.vercel.app
|
|
The latest updates on your projects. Preview: https://vitamin-c-5isxz6gyc-poiu694s-projects.vercel.app
|
|
The latest updates on your projects. Preview: https://vitamin-c-ans7vtr55-poiu694s-projects.vercel.app
|
* chore(layout): set kakao type * chore(page): add kakaoMap component for test * feat(KakaoMap): set test component with marker * eslint: fix lint error * fix: set kakao dts and set script self closing * chore: set typeRoots for dts * chore: change to kebab case * refactor: remove annotation and change useEffect import * refactor: remove annotation * refactor: use Script and set stratege * feat: set initialLevel prop * chore: set self closing * feat: change var to const and use useRef * chore: remove unuseful annotation * feat: set kakao map type" * eslint: set lint disable for do it only on Mount * feat: set useMap hook * feat: set addMarker and changeLocation * chore: change constans name * chore: change variable name * chore: set early return * chore: set useCallback * feat: set map component * feat: set map component * chore: set ealry return * chore: change import * refactor: set correct type to eventType * feat: set delete and reset marker logic * fix: fix lint error * fix: fix build error * chore: remove annotation and rename markerId * refactor: use Object.values * refactor: change type * fix: fix delete Object
- default - expanded - collapsed
- prevent background from scroll - add no-scrollbar tailwind utilities
- aria-expanded - aria-hidden
to fix Type error: Cannot find type definition file for 'bottom-sheet'. The file is in the program because: Entry point for implicit type library 'bottom-sheet'
|
The latest updates on your projects. Preview: https://vitamin-c-cb6dedr6s-poiu694s-projects.vercel.app
|
Issue Number
#16
Description
기본 바텀 시트를 구현하였습니다.
skipOneStepbottom-sheet.stories.ts)에서 테스트 가능하며,/cc에서 사용 예제를 확인할 수 있습니다.To Reviewers
framer-motion을 설치하여 이용하였습니다.react-use-measure를 설치하지 않고, 해당 소스 코드를 참고하여useMeasurehook을 만들었습니다.그러면서 debounce 옵션과 ResizeObserver를 위한 polyfill 옵션도 빼게 되었습니다. (∵ debounce 패키지 추가 설치 필요 및 ResizeObserver class 선언으로 인한 타입 에러 이슈 발생)
vaul(shadcn/ui에서 import해서 사용하고 있는 headless ui)을 보고 여러 가지 생각이 들었습니다.radix-ui의 dialog를 import하고 있어서, 이 블로그와 유사한 형태로 아주 간단한 형태의 바텀 시트를 구현하였습니다.radix나shadcn같이 잘 만들어져있는 라이브러리를 사용하는 게 나으려나... 라는 생각이 꽤나 여러 번 들었습니다. 😇Checklist
feat: add login page)