Skip to content
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] BottomSheet 컴포넌트, 스토리북 #41

Merged
merged 21 commits into from
Nov 17, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
5af13cf
[FEAT] BottomSheet 컴포넌트, 스토리북
houony Nov 8, 2024
3d3cf25
[REFACTOR] BottomSheet 컴포넌트
houony Nov 10, 2024
7bf1a90
Merge branch 'main' of https://github.com/Findy-org/FINDY_FE into fea…
houony Nov 10, 2024
366e0cb
[CHORE] BottomSheet 컴포넌트, 스토리북
houony Nov 10, 2024
70ffaf2
refactor : add error handling, reduce redundancy
houony Nov 11, 2024
eb7d761
refactor: add MouseEvent and TouchEvent
houony Nov 11, 2024
beeb54a
Merge branch 'main' of https://github.com/Findy-org/FINDY_FE into fea…
houony Nov 11, 2024
a92c437
chore : remove try-catch
houony Nov 11, 2024
c1a3f12
refactor : remove calculation of drag velocity
houony Nov 11, 2024
c63dfac
Merge branch 'main' of https://github.com/Findy-org/FINDY_FE into fea…
houony Nov 11, 2024
2279cf6
refactor : improve BottomSheet animation and positioning
houony Nov 12, 2024
631810a
refactor : optimize drag handler with requestAnimationFrame
houony Nov 12, 2024
90d4e6a
refactor : remove unnecessary code, change handleDragEnd logic
houony Nov 12, 2024
f273b49
refactor : remove onClose
houony Nov 12, 2024
f8f8c1f
refactor : add early return at handleDragEnd
houony Nov 12, 2024
390eb14
refactor : change style to tailwind, change dragElastic
houony Nov 12, 2024
5bf5e10
refactor : change drag amount
houony Nov 13, 2024
489baf2
refactor : move BottomSheet framer setting to motions.ts
houony Nov 14, 2024
b2f0822
refactor : remove onClose, add BottomSheetHeader, remove backdrop, up…
houony Nov 14, 2024
f3ff5ab
chore : remove react-use-measure, debounce
houony Nov 15, 2024
a4d2f8b
refactor : remove AnimatePresence, setTimeout
houony Nov 15, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 38 additions & 0 deletions .pnp.cjs

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
"react-dom": "^18.3.1",
"react-error-boundary": "^4.0.13",
"react-router-dom": "^6.26.2",
"react-use-measure": "^2.1.1",
"storybook": "^8.3.5",
"tailwind-merge": "^2.5.4",
"tailwindcss": "^3.4.13",
Expand Down
41 changes: 41 additions & 0 deletions src/components/common/BottomSheet/BottomSheet.stories.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
/* eslint-disable no-restricted-exports */
import { useState } from 'react';
import type { Meta, StoryObj } from '@storybook/react';

import { BottomSheet } from '.';
import { Button } from '../Button';
import { Layout } from '../Layout';

const meta = {
title: 'components/common/BottomSheet',
component: BottomSheet,
tags: ['autodocs'],
} satisfies Meta<typeof BottomSheet>;

export default meta;
type Story = StoryObj<typeof BottomSheet>;

export const Basic: Story = {
render: () => {
const [resetTrigger, setResetTrigger] = useState(0);

const handleClickBottomSheet = () => {
setResetTrigger((prev) => prev + 1);
};

return (
<Layout>
<div>
<Button size="medium" onClick={handleClickBottomSheet} variant="primary">
Open BottomSheet
</Button>
<BottomSheet resetTrigger={resetTrigger}>
<div className="flex flex-col gap-6 items-center">
<div>Contents</div>
</div>
</BottomSheet>
</div>
</Layout>
);
},
};
10 changes: 10 additions & 0 deletions src/components/common/BottomSheet/BottomSheet.types.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
import { ComponentPropsWithoutRef, ReactNode } from 'react';

export type Props = ComponentPropsWithoutRef<'div'> & {
resetTrigger?: number;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resetTrigger 값이 0인 경우 BottomSheetclose 상태인건가요??

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resetTrigger가 0인 경우가 BottomSheet의 상태를 나타내는 것은 아니고 사이드메뉴의 즐겨찾기 버튼을 한 번도 누르지 않은 초기 상태를 말합니다
resetTrigger 값이 바뀔 때마다(버튼을 누를 때마다) BottomSheet가 initialHeight(=150)으로 맞춰지며 BottomSheet가 열리게 됩니다

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

그럼 0보다는 boolean이 더 적합하지 않나요? 숫자형으로 하신 이유가 궁금합니다!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

엇 맞아요 그래서 요건 지금 boolean으로 관리하고 있습니다!

children?: ReactNode;
};

export type DragInfo = {
delta: { x: number; y: number };
};
51 changes: 51 additions & 0 deletions src/components/common/BottomSheet/index.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
import { memo } from 'react';
import { motion, useDragControls, AnimatePresence } from 'framer-motion';

import { useBottomSheet } from '@/hooks/common/useBottomSheet';

import { Props } from './BottomSheet.types';

const Content = ({ children }: React.PropsWithChildren) => (
<div className="w-full text-black p-6">{children}</div>
);

export const BottomSheet = memo(({ resetTrigger = 0, children }: Props) => {
const initialHeight = 150;
const minVisibleHeight = 60;
const dragControls = useDragControls();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

매직 넘버를 설정 가능한 props로 변경 필요

initialHeightminVisibleHeight가 하드코딩되어 있습니다. 이 값들을 props로 받아 컴포넌트의 유연성을 높이는 것이 좋겠습니다.

-export const BottomSheet = memo(({ resetTrigger = 0, children }: Props) => {
-  const initialHeight = 150;
-  const minVisibleHeight = 60;
+export const BottomSheet = memo(({ 
+  resetTrigger = 0, 
+  initialHeight = 150,
+  minVisibleHeight = 60,
+  children 
+}: Props & {
+  initialHeight?: number;
+  minVisibleHeight?: number;
+}) => {
📝 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.

Suggested change
const initialHeight = 150;
const minVisibleHeight = 60;
const dragControls = useDragControls();
export const BottomSheet = memo(({
resetTrigger = 0,
initialHeight = 150,
minVisibleHeight = 60,
children
}: Props & {
initialHeight?: number;
minVisibleHeight?: number;
}) => {
const dragControls = useDragControls();


const { sheetHeight, isHidden, isInteractionDisabled, handleDrag, handleDragEnd, resetSheet } =
useBottomSheet(initialHeight, minVisibleHeight, resetTrigger);

return (
<>
<div
className={`absolute top-0 left-0 w-full h-full transition-opacity duration-300 ${
sheetHeight > 10 ? 'opacity-70 pointer-events-auto' : 'opacity-0 pointer-events-none'
}`}
onClick={resetSheet}
/>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

백드롭 투명도 전환 로직 개선 필요

현재 백드롭의 투명도 전환이 하드코딩된 값(10)에 의존하고 있습니다. 이는 버그를 유발할 수 있으며, 의도를 파악하기 어렵게 만듭니다.

-sheetHeight > 10 ? 'opacity-70 pointer-events-auto' : 'opacity-0 pointer-events-none'
+sheetHeight > minVisibleHeight * 0.2 ? 'opacity-70 pointer-events-auto' : 'opacity-0 pointer-events-none'

Committable suggestion skipped: line range outside the PR's diff.

<AnimatePresence>
{!isHidden && (
<motion.div
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"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shadow 값이 혹시 적용이 안돼서 요렇게 하신건가요?!
shadow-[0px_-4px_10px_0px_rgba(0,0,0,0.1)]

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

넵 기본 tailwind shadow 설정을 적용했을 때는 그림자가 잘 보이지 않아서 커스텀 값 넣었습니다

style={{ height: sheetHeight }}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

styletailwindCSS 둘 다 사용하신 이유가 있으신가욥??

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sheetHeight은 드래그에 따라 동적으로 변하는 값이어서 tailwind를 통해 적용하기 어려워, style 속성을 따로 적용했습니다

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tailwind도 동적으로 변경 가능해요~!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아하 JIT로 tailwind도 동적으로 적용 가능한 것 확인했습니다!
그렇지만 바텀시트 특성상 드래그에 따라 sheetHeight 값이 자주 변하고, css 속성 중 height 값만 변하기 때문에 동적 클래스로 적용하는 않는 편이 성능이나 유지보수에서 더 나을 것이라고 생각하는데 그대로 두어도 괜찮을까요?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

height만 동적으로 변경하면 되지 않을까요?? 성능이나 유지보수 어디에 문제가 생기는지 궁금합니당

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

문제가 생긴다기 보다..! tailwind에서 매번 새로운 동적 클래스를 생성하는 성능보다 style에서 직접 다루는 것이 더 나을 거라고 생각했습니당
큰 차이가 없다면 통일성을 위해서 tailwind로 바꿀까요?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

어짜피 계산된 값이 들어갈텐데, style속성에 넣어도 새로운 값을 업데이트 하는건 똑같지 않나요??

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

저의 생각은 굳이 style,tailwind를 나눌 필요는 없어 보여요! 유지보수 측면에서도 css가 나눠져 있는 것 보단 하나에서 관리 하는게 좋을 것 같아요. 실제 성능상 유의미한 변화가 있는 지 체크 해보시면 좋을 것 같습니다~!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아하 알겠습니다! 확인해보고 수정해서 올리겠습니당

drag="y"
dragControls={dragControls}
dragElastic={0}
dragConstraints={{ top: 0, bottom: 0 }}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

요 속성들이 다 적용되고 있는게 맞나요??
불필요한 속성이 있다면 지워도 될 것 같아요.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

네 이 속성들은 다 적용되고 있습니당

onDrag={handleDrag}
onDragEnd={handleDragEnd}
animate={{ height: sheetHeight, opacity: isHidden ? 0 : 1 }}
transition={{ type: 'spring', stiffness: 170, damping: 30, duration: 0.3 }}
onPointerDown={(e) => !isInteractionDisabled && dragControls.start(e)}
exit={{ height: 0, opacity: 0 }}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

framer 관련 객체들 저는 constants의 motion.ts 에서 관리하는데, 따로 빼도 깔끔할것 같네요!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아하 확인했습니다! 수정해서 올리겠습니당

>
<div className="w-20 h-1.5 bg-primary mx-auto rounded-full" />
<Content>{children}</Content>
</motion.div>
)}
</AnimatePresence>
</>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

접근성 개선이 필요합니다

현재 구현에는 다음과 같은 접근성 관련 개선사항들이 필요합니다:

  1. ARIA 속성 추가
  2. 키보드 네비게이션 지원
  3. 스크린 리더 지원
 <motion.div
+  role="dialog"
+  aria-modal="true"
+  aria-label="바텀 시트"
   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"
+  onKeyDown={(e) => {
+    if (e.key === 'Escape') {
+      resetSheet();
+    }
+  }}
+  tabIndex={0}

또한 성능 최적화를 위해 다음 사항들을 고려해보세요:

  1. AnimatePresence 컴포넌트를 별도의 컴포넌트로 분리
  2. 백드롭 컴포넌트를 메모이제이션

Committable suggestion skipped: line range outside the PR's diff.

);
});
52 changes: 52 additions & 0 deletions src/hooks/common/useBottomSheet.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
import { useState, useCallback, useRef, useEffect } from 'react';

import { DragInfo } from '@/components/common/BottomSheet/BottomSheet.types';

export const useBottomSheet = (initialHeight = 150, minVisibleHeight = 60, resetTrigger = 0) => {
const [sheetHeight, setSheetHeight] = useState(initialHeight);
const [isHidden, setIsHidden] = useState(false);
const [isInteractionDisabled, setIsInteractionDisabled] = useState(false);
const dragOffsetRef = useRef(0);
const initialPositionRef = useRef(initialHeight);

useEffect(() => {
setSheetHeight(initialHeight);
setIsHidden(false);
setIsInteractionDisabled(false);
dragOffsetRef.current = 0;
initialPositionRef.current = initialHeight;
}, [resetTrigger, initialHeight]);

const handleDrag = useCallback(
(_: MouseEvent | TouchEvent | PointerEvent, info: DragInfo) => {
if (isInteractionDisabled) return;

const dragAmount = -info.delta.y;
dragOffsetRef.current += dragAmount;

const newHeight = Math.max(
initialPositionRef.current + dragOffsetRef.current,
minVisibleHeight
);
setSheetHeight(newHeight);
},
[isInteractionDisabled, minVisibleHeight]
);

const handleDragEnd = useCallback(() => {
if (sheetHeight <= minVisibleHeight) {
setIsHidden(true);
setIsInteractionDisabled(true);
}
}, [sheetHeight, minVisibleHeight]);

const resetSheet = useCallback(() => {
setSheetHeight(initialHeight);
setIsHidden(false);
setIsInteractionDisabled(false);
dragOffsetRef.current = 0;
initialPositionRef.current = initialHeight;
}, [initialHeight]);

return { sheetHeight, isHidden, isInteractionDisabled, handleDrag, handleDragEnd, resetSheet };
};
20 changes: 20 additions & 0 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -4584,6 +4584,13 @@ __metadata:
languageName: node
linkType: hard

"debounce@npm:^1.2.1":
version: 1.2.1
resolution: "debounce@npm:1.2.1"
checksum: 10c0/6c9320aa0973fc42050814621a7a8a78146c1975799b5b3cc1becf1f77ba9a5aa583987884230da0842a03f385def452fad5d60db97c3d1c8b824e38a8edf500
languageName: node
linkType: hard

"debug@npm:2.6.9":
version: 2.6.9
resolution: "debug@npm:2.6.9"
Expand Down Expand Up @@ -5933,6 +5940,7 @@ __metadata:
react-dom: "npm:^18.3.1"
react-error-boundary: "npm:^4.0.13"
react-router-dom: "npm:^6.26.2"
react-use-measure: "npm:^2.1.1"
storybook: "npm:^8.3.5"
storybook-addon-sass-postcss: "npm:^0.3.2"
tailwind-merge: "npm:^2.5.4"
Expand Down Expand Up @@ -9293,6 +9301,18 @@ __metadata:
languageName: node
linkType: hard

"react-use-measure@npm:^2.1.1":
version: 2.1.1
resolution: "react-use-measure@npm:2.1.1"
dependencies:
debounce: "npm:^1.2.1"
peerDependencies:
react: ">=16.13"
react-dom: ">=16.13"
checksum: 10c0/77b035189dbd613f50014ae56cbfc1363a4eba5104f68f3bc09cbdd20719ae7fb42884e53328175c30b238215c5b8064c60098d70b3fa9b8d902db6ffb07c6a3
languageName: node
linkType: hard

"react@npm:^16.8.0 || ^17.0.0 || ^18.0.0, react@npm:^18.3.1":
version: 18.3.1
resolution: "react@npm:18.3.1"
Expand Down
Loading