[REFACTOR] BottomSheet width 값 조절과 포지셔닝, close 버튼 추가 #59
Conversation
Walkthrough이 풀 리퀘스트는 Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
Update: 2024년 11월 26일 00시 57분 23초 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
src/components/common/BottomSheet/BottomSheetHeader.types.ts (1)
1-3: 타입 정의가 명확하고 PR 목적에 부합합니다.타입 정의가 잘 되어있으며, BottomSheet의 닫기 기능을 위한 Props 인터페이스가 적절하게 구현되었습니다.
더 나은 문서화를 위해 JSDoc 주석을 추가하는 것을 추천드립니다:
+/** + * BottomSheetHeader 컴포넌트의 Props + */ export type Props = { + /** BottomSheet의 열림/닫힘 상태를 제어하는 함수 */ setIsOpen: (value: boolean) => void; };src/components/common/BottomSheet/BottomSheetHeader.tsx (2)
5-8: handleClose 함수의 최적화를 고려해보세요.
handleClose함수를useCallback으로 메모이제이션하면 불필요한 리렌더링을 방지할 수 있습니다.다음과 같이 수정하는 것을 추천드립니다:
+import { useCallback } from 'react'; import { Props } from './BottomSheetHeader.types'; export const BottomSheetHeader = ({ setIsOpen }: Props) => { - const handleClose = () => { + const handleClose = useCallback(() => { setIsOpen(false); - }; + }, [setIsOpen]);
11-12: 반응형 디자인 고려사항현재 구현된 고정 너비(
w-20)와 높이(h-1.5) 값들이 모든 화면 크기에서 적절할지 검토가 필요합니다.다음과 같이 반응형 클래스를 추가하는 것을 고려해보세요:
- <div className="w-20 h-1.5 bg-primary mx-auto rounded-full" /> + <div className="w-16 md:w-20 h-1 md:h-1.5 bg-primary mx-auto rounded-full" />src/hooks/common/useBottomSheet.tsx (2)
49-54: 스크롤 높이 계산 최적화 제안
scrollHeight속성을 매 애니메이션 프레임마다 읽는 것은 성능에 영향을 줄 수 있습니다. 컨텐츠가 자주 변경되지 않는다면,useEffect를 사용하여 컨텐츠가 변경될 때만 높이를 계산하는 것이 좋습니다.다음과 같이 최적화할 수 있습니다:
+const contentHeightRef = useRef<number>(MAX_HEIGHT); + +useEffect(() => { + contentHeightRef.current = bottomSheetRef.current?.scrollHeight || MAX_HEIGHT; +}, [isOpen]); // isOpen이 변경될 때만 높이를 다시 계산 + const handleDrag = useCallback( (_: MouseEvent | TouchEvent | PointerEvent, info: DragInfo) => { if (isInteractionDisabled) return; requestAnimationFrame(() => { const dragAmount = -info.delta.y; dragOffsetRef.current += dragAmount * 5; - const contentHeight = bottomSheetRef.current?.scrollHeight || MAX_HEIGHT; const newHeight = Math.min( Math.max(initialPositionRef.current + dragOffsetRef.current, MIN_VISIBLE_HEIGHT), - Math.min(contentHeight, MAX_HEIGHT) + Math.min(contentHeightRef.current, MAX_HEIGHT) );
72-74: 상태 초기화 로직 개선드래그 종료 시 상태 초기화 로직이 추가되었습니다. 코드의 가독성과 유지보수성을 높이기 위해 초기화 로직을 별도의 함수로 분리하는 것이 좋습니다.
다음과 같이 리팩토링을 제안합니다:
+const resetDragState = (height: number) => { + initialPositionRef.current = height; + dragOffsetRef.current = 0; +}; + const handleDragEnd = useCallback(() => { if (sheetHeight <= MIN_VISIBLE_HEIGHT) { setIsOpen(false); setIsInteractionDisabled(true); + resetDragState(INITIAL_HEIGHT); return; } if (sheetHeight > MAX_HEIGHT) { setSheetHeight(MAX_HEIGHT); + resetDragState(MAX_HEIGHT); return; } - initialPositionRef.current = sheetHeight; - dragOffsetRef.current = 0; + resetDragState(sheetHeight); }, [setIsOpen, sheetHeight]);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
src/components/common/BottomSheet/BottomSheet.stories.tsx(1 hunks)src/components/common/BottomSheet/BottomSheetHeader.tsx(1 hunks)src/components/common/BottomSheet/BottomSheetHeader.types.ts(1 hunks)src/components/common/BottomSheet/index.tsx(1 hunks)src/constants/bottomSheetOptions.ts(1 hunks)src/hooks/common/useBottomSheet.tsx(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/components/common/BottomSheet/BottomSheet.stories.tsx
🔇 Additional comments (6)
src/constants/bottomSheetOptions.ts (2)
3-3: SSR 대응과 높이 조정이 개선되었습니다
다음과 같은 개선사항이 확인됩니다:
typeof window !== 'undefined'체크를 통해 SSR 호환성이 향상되었습니다- 최대 높이가 window 높이의 80%로 조정되어 더 나은 사용성을 제공할 것으로 예상됩니다
- 기본값 800px 설정으로 안정성이 향상되었습니다
✅ Verification successful
바텀시트 높이 조정 및 SSR 대응이 적절히 구현되었습니다
검증 결과 다음 사항들이 확인되었습니다:
useBottomSheet훅에서 MAX_HEIGHT가 적절히 활용되어 바텀시트의 최대 높이를 제한하고 있습니다- 컨텐츠 높이와 MAX_HEIGHT를 비교하여 적절한 높이 제어가 이루어지고 있습니다
- SSR 대응을 위한 window 객체 체크와 기본값 설정이 올바르게 구현되어 있습니다
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify MAX_HEIGHT usage in components
# and check for any potential layout issues
# Search for MAX_HEIGHT usage
rg -A 5 "MAX_HEIGHT" --type ts --type tsx
# Look for any height-related style definitions
ast-grep --pattern 'style={$_height$_}'
Length of output: 111
Script:
#!/bin/bash
# Search for MAX_HEIGHT usage with correct file type
rg -A 5 "MAX_HEIGHT" -t ts -t typescript -t jsx -t tsx
# Look for style definitions containing height
rg -A 3 "style.*height" -t ts -t typescript -t jsx -t tsx
# Check if the constant is imported and used in other files
rg "import.*MAX_HEIGHT.*from.*constants" -t ts -t typescript -t jsx -t tsx
Length of output: 296
Script:
#!/bin/bash
# Search for MAX_HEIGHT usage
rg -A 5 "MAX_HEIGHT"
# Look for style definitions containing height
rg -A 3 "style.*height"
# Find all TypeScript/JavaScript files that might use MAX_HEIGHT
fd "\.(ts|tsx|js|jsx)$" -x grep -l "MAX_HEIGHT" {}
Length of output: 2816
1-2: INITIAL_HEIGHT와 MIN_VISIBLE_HEIGHT 간의 관계 확인 필요
INITIAL_HEIGHT가 130px로 감소되었습니다. MIN_VISIBLE_HEIGHT가 60px로 설정된 것을 고려할 때, 두 값의 차이(70px)가 드래그 상호작용에 충분한지 확인이 필요합니다.
src/components/common/BottomSheet/BottomSheetHeader.tsx (1)
1-3: 타입과 컴포넌트 임포트가 깔끔하게 구성되었습니다!
컴포넌트와 타입을 분리하여 임포트한 구조가 잘 정리되어 있습니다.
src/components/common/BottomSheet/index.tsx (3)
22-22: 스타일링 개선사항 검토 필요
className에서 동적 높이를 템플릿 리터럴로 처리하는 부분이 잠재적인 문제를 일으킬 수 있습니다. sheetHeight가 undefined일 경우 스타일이 깨질 수 있습니다.
다음과 같이 개선하는 것을 고려해보세요:
-className="absolute bottom-0 left-0 right-0 m-auto max-w-[30rem] h-${sheetHeight} bg-white shadow-[0px_-4px_10px_0px_rgba(0,0,0,0.1)] rounded-t-3xl p-4 overflow-hidden z-40"
+className={`absolute bottom-0 left-0 right-0 m-auto max-w-[30rem] ${
+ sheetHeight ? `h-[${sheetHeight}]` : 'h-0'
+} bg-white shadow-[0px_-4px_10px_0px_rgba(0,0,0,0.1)] rounded-t-3xl p-4 overflow-hidden z-40`}29-33: 애니메이션 구현이 적절해 보입니다
initial 상태 추가와 BOTTOM_SHEET_ANIMATION 상수 활용이 자연스러운 동작을 만들어냅니다. 특히 height: 0부터 시작하는 것이 좋은 선택입니다.
35-35: 타입 안전성 확인 필요
BottomSheetHeader에 setIsOpen prop이 전달되었습니다. 연관된 타입 정의를 확인해주세요.
✅ Verification successful
The search results show that BottomSheetHeader has proper type definitions in src/components/common/BottomSheet/BottomSheetHeader.types.ts with setIsOpen correctly typed as (value: boolean) => void. This matches the usage in the code snippet. I can now generate the final response:
타입 정의가 올바르게 되어있음을 확인했습니다
BottomSheetHeader 컴포넌트의 Props 타입에 setIsOpen이 (value: boolean) => void로 정확하게 정의되어 있습니다.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify type definitions for BottomSheetHeader
ast-grep --pattern 'type Props = {
$$$
setIsOpen: $_
$$$
}'
Length of output: 1259
| <button onClick={handleClose} className="absolute right-4 top-0" aria-label="Close"> | ||
| <Icon name="deleteRound" size={24} /> | ||
| </button> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
접근성 개선 사항을 제안드립니다.
현재 기본적인 접근성이 구현되어 있으나, 다음과 같은 개선사항들을 고려해보시면 좋겠습니다:
다음과 같이 수정하는 것을 추천드립니다:
- <button onClick={handleClose} className="absolute right-4 top-0" aria-label="Close">
+ <button
+ onClick={handleClose}
+ className="absolute right-4 top-0 p-2 hover:bg-gray-100 rounded-full transition-colors"
+ aria-label="바텀시트 닫기"
+ type="button"
+ >
<Icon name="deleteRound" size={24} />
</button>변경사항:
- 한글 aria-label 추가
- 버튼 타입 명시
- 호버 상태 추가로 상호작용 피드백 개선
- 터치 영역 확장을 위한 패딩 추가
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <button onClick={handleClose} className="absolute right-4 top-0" aria-label="Close"> | |
| <Icon name="deleteRound" size={24} /> | |
| </button> | |
| <button | |
| onClick={handleClose} | |
| className="absolute right-4 top-0 p-2 hover:bg-gray-100 rounded-full transition-colors" | |
| aria-label="바텀시트 닫기" | |
| type="button" | |
| > | |
| <Icon name="deleteRound" size={24} /> | |
| </button> |
| @@ -19,13 +19,14 @@ export const BottomSheet = memo(({ children, isOpen, setIsOpen }: Props) => { | |||
| <Portal isOpen={isOpen}> | |||
| <motion.div | |||
| ref={bottomSheetRef} | |||
| className="absolute bottom-0 left-0 w-full bg-white shadow-[0px_-4px_10px_0px_rgba(0,0,0,0.1)] rounded-t-3xl p-4 overflow-hidden z-40 h-${sheetHeight}" | |||
| className="absolute bottom-0 left-0 right-0 m-auto max-w-[30rem] h-${sheetHeight} bg-white shadow-[0px_-4px_10px_0px_rgba(0,0,0,0.1)] rounded-t-3xl p-4 overflow-hidden z-40" | |||
There was a problem hiding this comment.
벡틱으로 감싸야 동적으로 sheetHeight 반영가능할 거 같아요~!
| className="absolute bottom-0 left-0 right-0 m-auto max-w-[30rem] h-${sheetHeight} bg-white shadow-[0px_-4px_10px_0px_rgba(0,0,0,0.1)] rounded-t-3xl p-4 overflow-hidden z-40" | |
| className={`absolute bottom-0 left-0 right-0 m-auto max-w-[30rem] h-${sheetHeight} bg-white shadow-[0px_-4px_10px_0px_rgba(0,0,0,0.1)] rounded-t-3xl p-4 overflow-hidden z-40`} |
관련 이슈
close #58
📑 작업 내용
💬 리뷰 중점 사항/기타 참고 사항
Summary by CodeRabbit
새로운 기능
BottomSheet컴포넌트의 콘텐츠 구조가 개선되어 여러 개의 내용이 추가됨.BottomSheet의 레이아웃과 애니메이션 동작이 향상됨.버그 수정