Conversation
빌드 결과빌드 성공 🥳 |
빌드 결과빌드 성공 🥳 |
|
바텀시트 리뷰 중 (이렇게 찜하는 건가요 ㅋㅋ) |
mimizae
left a comment
There was a problem hiding this comment.
이것 또한 아티클 수준의 PR과 주석 덕분에 로직이 쉽게 쉽게 이해 되어서 너무 좋았습니다 🤩🤩🤩
버그를 하나 발견했는데!!!! ㅠㅠ 머리를 짜내어서 원인을 유추해 보았지만 이렇다 할 해결책까지는 제시하지 못했습니다. . . 리뷰를 마치고 좀 더 생각해 보겠습니다 ㅠ.ㅠ
너무 수고하셨어용 👏🏻👏🏻👏🏻👏🏻 ㅎㅎㅎ
There was a problem hiding this comment.
병합 충돌 방지하기 위해 임시로 bottom-sheet 폴더 아래에 bottom-sheet용 hook 폴더를 위치시키신 걸까요??
There was a problem hiding this comment.
바텀시트에서만 사용되는 훅이라 바텀시트 구현 코드와 함께 묶이는게 좋을 거라고 생각해 bottom-sheet/hooks처럼 선언해두긴 했는데, 처음에 컨벤션으로 정한 것과 일치하는 폴더구조는 아니죠..? 폴더구조도 고민이 많이 되네요...ㅠㅠ
| // 실제 바텀시트(흰색 박스)) | ||
| export const sheet = style({ | ||
| position: "relative", // overlay 기준으로 상대적인 위치 설정 | ||
| width: "100%", |
There was a problem hiding this comment.
저희가 모바일 웹앱이라 컴포넌트를 구현할 때 웹 환경까지 고려해서 구현하지는 않아서 width: 100%로 설정해뒀습니다..!
overlay는 backdrop과 sheet 자식 컴포넌트를 담는 용도로만 사용하고, 크기는 자식 컴포넌트들에서 조절하면 좋을 것 같아 sheet의 width를 375px로 변경했어요!
There was a problem hiding this comment.
버그를 발견했는데, 개발자 도구에서는 드래그해서 바텀 시트를 닫는 동작이 잘 먹히는데 일반 화면에서는 안 먹히네요 ㅠㅠ
제가 추측한 이유로는...
PR에 적어주신 설명에 따르면 바텀시트가 아래로 내려가는 조건은
- deltaY > 0
- content.scrollTop === 0 이걸 만족하면 내려간다고 되어 있는데
실제 전체 화면에서는 드래그를 시작하는 순간 브라우저가 이 동작을 ‘스크롤하려는 제스처’로 먼저 처리해버리는 것 같아요
그래서 content 영역에서 bounce → scroll 이 먼저 일어나고, scrollTop이 0이 되기 전에 touchmove 이벤트가 브라우저에서 소모되어 moveSheet() 쪽으로 아예 이벤트가 전달되지 않는 흐름인 것 같아요....
요약하자면 브라우저가 스크롤을 우선 처리해서 아래로 드래그하는 이벤트가 바텀시트 로직까지 도달하지 않는 것. . . 근데 진짜 단순 추측이라 ㅠㅠ. . . 정확히 어디를 어떻게 고쳐야 하는지는 못 찾았습니다 ㅠㅠ 이거 찾느라고 리뷰가 늦었네요...
2025-11-23.11.56.03.mov
There was a problem hiding this comment.
앗 이부분은 제가 모바일 환경만 고려하고 웹 환경에서의 드래그 핸들러 메서드를 구현해두지 않아서 그래요.
현재 바텀시트에
<div
ref={contentRef}
className={styles.content}
onTouchStart={handleTouchStart}
onTouchMove={handleTouchMove}
onTouchEnd={handleTouchEnd}
{children}
</div>이렇게 모바일 환경에서 터치 드래그를 담당하는 이벤트 핸들러만 prop로 넘겨주고 있고, 웹 환경에서 마우스 드래그를 담당하는 이벤트 핸들러는 넘겨주고 있지 않아요. 이번 합세가 모바일 웹앱이라 모바일에서의 동작만 신경쓰고 웹 환경에서의 동작은 크게 신경을 안써서ㅎㅎ..
로직에는 큰 문제가 없어 다음과 같이 별도로 마우스 이벤트 핸들러만 선언해 div에 넘겨주면 웹 환경에서도 잘 동작하더라고요!
<div
ref={contentRef}
className={styles.content}
onTouchStart={handleTouchStart}
onTouchMove={handleTouchMove}
onTouchEnd={handleTouchEnd}
onMouseDown={handleMouseDown}
onMouseMove={handleMouseMove}
onMouseUp={handleMouseUp}>
{children}
</div>2025-11-24.6.09.52.mov
참고로 수빈님은 캐러셀 구현하실 때 모바일, 웹 환경 둘 다 고려해서 스와이프 핸들러를 구현하셨어요. 컴포넌트에 핸들러를 넘길 때도 이벤트 객체를 미리 선언해둬 spread연산자로 깔끔하게 넘기셨던데 코드 확인해보셔도 좋을 것 같아 링크 남깁니다ㅎㅎ
https://github.com/SOPT-all/37-COLLABORATION-WEB-IDUS/pull/32/files#r2554087672
| return createPortal( | ||
| // TODO: 접근성 고려 | ||
| <div className={styles.overlay}> | ||
| <div className={styles.backdrop} onClick={onClose} /> |
There was a problem hiding this comment.
시간 여유가 있다면!! overlay/backdrop 클릭 시 바텀시트가 바로 사라지지 않고 부드럽게 아래로 내려가는 애니메이션 후 닫히도록 개선하면 UX가 더 자연스러울 것 같아요!
현재는 클릭 즉시 unmount되는데, isClosing 같은 상태를 추가해서 애니메이션 완료 후 onClose()가 호출되도록 하면 드래그로 닫을 때와 일관된 모션을 제공할 수 있을 것 같습니닷.
src/shared/components/bottom-sheet/hooks/use-bottom-sheet-drag.ts
Outdated
Show resolved
Hide resolved
| maxHeight: "90dvh", // | ||
| backgroundColor: color.white[100], | ||
| borderTopLeftRadius: "16px", | ||
| borderTopRightRadius: "16px", |
There was a problem hiding this comment.
border-radius는 의도적으로 px로 적용해뒀어요!
rem을 사용하는 목적은 사용자가 브라우저의 기본 글꼴 크기를 조정할 때, rem 단위를 사용하는 요소들도 함께 비례해서 조정하는 것이에요. 이를 통해 웹 accessibility를 향상시킬 수 있어요.
반면 px은 절댓값으로 브라우저 환경과 관계없이 일정한 값을 유지해요. 일반적으로 컴포넌트의 테두리 굵기(border), 곡률(borderRadius) 등 사용자의 글꼴 크기 설정과 관계없이 일정하게 유지되는게 시각적으로 분명하고 디자인 의도를 살릴 수 있는 속성은 px로 값을 설정해요.
px로 값을 사용하는 다른 속성들로는 box-shadow, position(요소를 화면의 고정된 위치에 배치해야 할 때 top: 20px같이 사용) 등이 있어요!
There was a problem hiding this comment.
와 죄송합니다 단순 오타인 줄 알고 px out 이렇게 남겼는데 이런 뜻이 있었군요 이 무지함에 야유를......;;;; 그럼에도 친절한 설명 감사합니다 ㅜㅜ!!!!!! 완전히 이해 되었습니다
| const [dragDistance, setDragDistance] = useState(0); // sheet이 내려간 거리 | ||
| const [isDragging, setIsDragging] = useState(false); |
There was a problem hiding this comment.
요 두 상태는 상태로 관리할 필요가 없어보입니다.
(필히 수정해 주세요... 는 아니지만 시간 여유가 있다면 리팩토링 해보는 게 어떨지 제안 드립니다... ㅎ.ㅎ)
- dragDistance는 지역 변수로 관리해도 충분해 보여요
지금 코드에서 드래그할 때마다 moveSheet가 실행되고 moveSheet 안에서 sheetDeltaY를 계산하는데 그걸 dragDistance에 저장하죠!
즉, dragDistance는 그냥 sheetDeltaY를 복사해서 보관하는 형태가 되죠
또, dragDistance가 활용되는 유일한 곳이
if (dragDistance > CLOSE_THRESHOLD) {
onClose();
}이 부분 뿐이라 마지막 드래그 거리만 알 수 있으면 충분해 보여요!!! 즉, 드래그 과정 전체를 state로 추적할 필요 없이 지역 변수로 처리해도 동작에는 문제가 없고, 오히려 불필요한 리렌더도 줄일 수 있을 것 같습니닷
- isDragging는 ref로 관리하는 게 더 적절해 보여요
이 값은 단순히 "지금 드래그 중인지"를 판단하기 위한 논리 플래그로만 쓰이는데 이런 값은 state로 둘 경우 리렌더를 불필요하게 유발할 수 있고,
또 state는 비동기 업데이트라 드래그 시작 직후 값이 즉시 반영되지 않을 수도 있는데, ref로 관리하면 즉시 반영되고 렌더에도 관여하지 않아서 더 안정적일 것 같습니다
There was a problem hiding this comment.
dragDistance는 드래그가 종료될 때 실행되는 메서드인 handleTouchEnd에서만 사용되는데 useState로 선언해서 드래그 할 때마다 리렌더가 발생하도록 구현할 필요가 없죠 지당하십니다
렌더링에 사용되는 값도 아니므로 dragDistance도 ref로 구현하면 좋을 것 같아요(let 지역변수로 선언하는 것보다 더 안정적, 예측 가능)
isDragging도 마찬가지 이유로 ref로 선언하는게 더 적합하다는 의견에 동의합니다ㅎㅎ
세심한 부분까지 신경쓰시면서 리뷰 남겨주셔서 감사합니다~
refactor: 드래그 핸들러 변수명 변경 Co-authored-by: mimizae <144243473+mimizae@users.noreply.github.com>
…7-COLLABORATION-WEB-IDUS into feat/bottom-sheet/#40
| <BottomSheet isOpen={isOpen} onClose={handleClose}> | ||
| <h2 className={styles.sheetTitle}>바텀시트 제목</h2> | ||
| <p className={styles.sheetDescription}> | ||
| 아래로 드래그하거나 뒷배경을 탭하면 닫힘 | ||
| </p> | ||
| <div className={styles.sheetContent}> | ||
| {Array.from({ length: 30 }, (_, i) => ( | ||
| <div key={i} className={styles.sheetItem}> | ||
| {i + 1} | ||
| </div> | ||
| ))} | ||
| </div> | ||
| </BottomSheet> |
There was a problem hiding this comment.
<BottomSheet>
<BottomSheet.Trigger>
<button>바텀시트 열기</button>
</BottomSheet.Trigger>
<BottomSheet.Content>
<BottomSheet.Header>바텀시트 제목</BottomSheet.Header>
<BottomSheet.Description>
아래로 드래그하거나 뒷배경을 탭하면 닫힘
</BottomSheet.Description>
. . .
</BottomSheet.Content>
</BottomSheet>요런 인터페이스를 가지면 느좋일듯해요.
읽기도 쉬워지고(책임을 명확히함), 변경에도 유연하고, 외부에서 오픈여부를 관리하고 알고있을 필요도 없어지고, ... 등등
@jstar000
There was a problem hiding this comment.
하 이까지 찾아와주셔서 리뷰까지 남겨주시고 감사합니다ㅠㅠ
작성해주신 Compound Component 패턴도 미나미미나때 다뤘었던 내용이랑 밀접한 관련이 있었네요 '어떻게' 대신 '무엇을' 할 지 집중할 수 있도록 해주는 선언적인 구현 방식!
현재
- 바텀시트 사용처에서 open상태, open/close 핸들러 등을 관리해야 함
- 사용 시 뭐가 헤더인지, 뭐가 본문인지 등 파악이 힘듦
=> 바텀시트가 어떻게 동작하는지를 사용처가 알아야 함
Compound 방식
<BottomSheet>
<BottomSheet.Trigger>
<button>열기</button>
</BottomSheet.Trigger>
<BottomSheet.Content>
<BottomSheet.Header>제목</BottomSheet.Header>
<BottomSheet.Description>설명</BottomSheet.Description>
<div>본문</div>
</BottomSheet.Content>
</BottomSheet>=> 사용처에서는 무엇을 렌더링할 지만 선언하면 됨
상태 관리를 Compound 컴포넌트 내부로 캡슐화하고 사용할 때는 구조만 선언하면 되므로 바텀시트 내부 로직이 바뀌어도 사용처에서는 수정할 필요가 없어 변경에도 유연해지네요.
=> 바텀시트 내부 동작(드래그, 애니메이이션 로직, 이벤트 핸들링 등)은 절차적일 수밖에 없으므로 절차적으로 구현, 사용처에서는 Compound Component 등 적절한 구현방식을 사용해 절차적인 내부 동작을 감추고 깔끔한 인터페이스만 노출하도록 수정해보겠습니다! 합세 끝나고 꼬옥 적용해볼게요 지금 수정하면 API 연결 못할 듯...ㅋㅋㅋ
미미나 듣고 어떤 상황에서 선언적으로 구현하는게 적합할지, 어떤 상황에서 절차적으로 구현하는게 적합할지에 대해서 합세 코드 다시 보면서 고민해봤는데, 합세에서 담당한 컴포넌트 지연로딩 구현은
<LazySection fallback={<LoadingFallback />}>
<ProductDetail />
</LazySection>이렇게 구현해 사용하는 쪽에서 선언적으로 사용할 수 있도록 구현, 내부로직은 절차적으로 구현한 것 같아요. 사실 선언적/절차적을 의식하고 구현한 건 아니었는데 미미나 듣고, 달아주신 리뷰 확인하고, 구현한 코드 다시 확인해보니 눈에 들어오네요ㅎㅎ 어떻게 구현해야할 지에 대해 큰~~ 깨달음을 주셨습니다 강민하사랑해
그리고 선생님저 질문이 생겼는데
- 미미나때 useQuery?가 내부적으로는 몇백줄의 절차적인 코드로 구현됐다고 얘기하셨는데, 저수준 로직은 본질적으로 절차적이니까 그렇게 구현될 수밖에 없는게 맞을까요..?
There was a problem hiding this comment.
미미나때 useQuery?가 내부적으로는 몇백줄의 절차적인 코드로 구현됐다고 얘기하셨는데, 저수준 로직은 본질적으로 절차적이니까 그렇게 구현될 수밖에 없는게 맞을까요..?
그쵸. 결국에 우리가 어떤 코드를 써도 결국엔 컴퓨터(cpu)의 절차적인 명령어로 처리가 됨.
예를 들면 많이들 작성해봤을 axios.interceptors 이런거는 필연적으로 절차적인 로직으로 작성될 수 밖에 없어요.ㅎㅎ
결국에 어디까지 로직을 숨기고 드러낼지가 핵심이에여(적절한 추상화)
There was a problem hiding this comment.
아하 이해했어요 앞으로 선언적/절차적 신경쓰고 추상화 레벨, 책임 분리, 캡슐화 고려하면서 구현해보겠습니다ㅎㅎ 리뷰 감사합니다~~
빌드 결과빌드 성공 🥳 |

📌 Summary
해당 PR에 대한 작업 내용을 요약하여 작성해주세요.
📄 Tasks
해당 PR에 수행한 작업을 작성해주세요.
🔍 To Reviewer
바텀시트 구현 과정은 다음과 같아요.
bottom-sheet.tsx: 바텀시트 기본 UI 구현
overlay: 전체 바텀 시트의 레이아웃 컨테이너,backdrop과sheet을 자식으로 가져요backdrop: 반투명의 검은 배경, 사용자가 backdrop 클릭 시 바텀시트가 닫혀요sheet: 실제 바텀시트(흰색 배경 영역)children: 바텀시트에 띄울 컨텐츠BottomSheet에 React-Portal 적용
React-Portal을 사용해 body 안에 바텀시트를 자식으로 추가했어요.뒷배경 스크롤 차단 로직 구현
바텀시트 뒤에 보이는 반투명한
overlay영역을 드래그했을때 발생하는 뒷배경 스크롤을 비활성화했어요. 따라서 바텀시트 내부가 스크롤돼야하는데 의도치 않게 뒷배경이 스크롤되는 상황을 방지할 수 있어요.useEffect 실행 시 뒷배경(’body’ element)의
overflow속성(스크롤 처리 css 속성)을 저장해두고, 바텀시트가 열린 동안에는 뒷배경의overflow속성을hidden으로 설정해 뒷배경 스크롤을 방지해요.바텀시트의
isOpen값이 바뀌거나 바텀시트가 언마운트될 때 useEffect의 클린업 함수가 실행되며 원래 뒷배경이 가졌던overflow속성으로 복구시켜요.바텀시트 드래그 로직 구현
use-bottom-sheet-drag.ts에 바텀시트를 드래그하는 로직을 구현했어요(sheet(바텀시트)와content(바텀시트 위에 올라가는 자식 컨텐츠)를 잘 구분해야해요)‘구매하기’ 버튼을 눌러 바텀시트가 올라온 상황에서, 이 바텀시트를 내리는 방법은 두 가지가 있어요.
backrdop영역 터치하기바텀시트를 일정 거리 이상 위에서 아래로 드래그하기
이때 위 → 아래 드래그 액션은 두가지 목적으로 나뉘어요.
content가 최상단에 위치한 경우에서 위 → 아래로 드래그: 이 경우 사용자는 바텀시트를 내리기 위해 드래그한 것
content가 아래로 스크롤된 상태에서 위 → 아래로 드래그: 이 경우 사용자는 content를 올리기 위해 드래그한 것
예를 들어, 다음 그림에서 전체 드래그 거리는
450px이지만,200px은content를 위로 올리기 위해,250px은 바텀시트를 아래로 내리기 위해 사용했어요.위 → 아래 드래그 액션을 두 가지로 구분하지 않으면, content를 위로 스크롤하기 위해 드래그했는데 바텀시트가 내려가는 등의 문제가 발생할 수 있어요. 따라서 사용자의 액션이 컨텐츠를 위로 스크롤하기 위한 동작인지, 바텀시트를 내리기 위한 동작인지 구분할 수 있어야해요.
저는 두 동작을 구분하는 기준으로 ref 객체의
scrollTop을 사용했어요.useRef로sheet,content컴포넌트를 감시하는 두 개의 ref 객체sheetRef,contentRef를 생성해요.contentRef.current.scrollTop은 사용자가 content를 얼마나 위에서 아래로 스크롤했는지 px 단위로 알려줘요. 따라서contentRef.current.scrollTop의 값이0이라면 content의 스크롤 위치가 가장 위에 있다는 것을 의미해요.드래그 과정 설명
드래그가 시작될 때
handleTouchStart메서드에서 드래그를 시작한 y좌표를startY에 저장해요.드래그 중에는
handleTouchMove메서드가 실행돼요.이 메서드는
currentY에 현재 드래그 중인 지점의 y좌표를 저장하고,deltaY(currentY-startY)에 y좌표 변화량을 저장해요.+) 브라우저의 y좌표
+) 브라우저 화면의 최상단의 y좌표값은 0이고, 화면 아래로 내려올 수록 y좌표의 크기가 커져요. 따라서deltaY < 0드래그를 아래에서 위로 진행, 즉
content를 아래로 스크롤하는 동작이므로 아무런 처리도 하지 않음(브라우저가 content 아래로 스크롤 알아서 처리)deltaY > 0드래그가 위에서 아래로 진행, 따라서 if문 안으로 진입해 사용자의 드래그 목적을 판단하기 위한 if문을 실행
contentRef.current.scrollTop === 0현재 content의 스크롤 위치가 바텀시트의 가장 위에 있다는 뜻이므로, 그 이후에 진행되는 드래그는 바텀시트를 내리기 위한 드래그라고 판단하고
moveSheet()함수를 호출해 바텀시트를 아래로 내림contentRef.current.scrollTop > 0content의 스크롤 위치가 바텀시트의 최상단이 아니다, 즉content가 아래로 스크롤된 상태를 의미함.이때 사용자는 바텀시트를 아래로 내리기 위해 드래그하는게 아니라,
content의 스크롤을 위로 올리기 위해 드래그하는 것이므로 아무런 처리도 하지 않음(브라우저가 content 위로 스크롤 알아서 처리)드래그가 종료되면
handleTouchEnd메서드가 실행돼요.dragDistance는 드래그를 시작한 시점의 y좌표와 드래그가 끝난 시점의 y좌표의 거리를 의미하는게 아니라 실제로 sheet이 내려간 거리를 의미해요. 위 예시 그림에서 실제로 바텀시트가 내려간 거리인250px이dragDistance에 저장된 값이에요.handleTouchEnd메서드는dragDistance가 일정 거리 이상(현재 200px로 세팅) 이동했다면 바텀시트를 닫고, 그것보다 작다면 바텀시트를 원위치로 복귀시켜요.성능
드래그가 진행 중일 때
handleTouchMove메서드가 수십~수백번 호출되지만 성능에는 문제가 없을 듯해요.handleTouchMove내에서 빠르게 return됨, 실제로 moveSheet() 하는 경우 적음transform기반리뷰어에게 요청하는 내용을 작성해주세요.
📸 Screenshot
2025-11-23.1.59.34.mov
작업한 내용에 대한 스크린샷을 첨부해주세요.