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

refactor: bottomSheet hooks 리팩토링 #286

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

sujin-park
Copy link
Collaborator

@sujin-park sujin-park commented Nov 14, 2022

Description

바텀시트 하위컴포넌트를 동적으로 변경하기 위해서 useBottomSheet 에 함수를 추가하였습니다.

Changes

useBottomSheet 를 사용하는 방법

페이지 1개에 bottomSheet 하위 컴포넌트가 고정일 때

const { isVisibleSheet, calcBottomSheetHeight, toggleSheet } = useBottomSheet();

<>
{isVisibleSheet && (
   <CommonBottomSheetContainer onClose={onCloseBottomSheet} bottomSheetHeight={calcBottomSheetHeight(...)}>
      {bottomSheetRender.children}
   </CommonBottomSheetContainer>
)}
</>

페이지 1개에 bottomSheet children 이 변경될 수 있을 때

const {
  isVisibleSheet,
  toggleSheet,
  calcBottomSheetHeight,
  bottomSheetRender,
  setBottomSheetRender,
} = useBottomSheet();

<>
{isVisibleSheet && (
   <CommonBottomSheetContainer onClose={toggleSheet} bottomSheetHeight={bottomSheetRender.height}>
      {bottomSheetRender.children}
   </CommonBottomSheetContainer>
)}
</>

이점 (이라고 생각하는 부분)

  • bottomSheet 의 type 을 상수로 관리하거나 bottomSheet 관련 객체를 선언해둘 필요가 없습니다.
  • bottomSheet 는 한 페이지에 한 개만 노출될 수 있고, children 만 변경이 되기 때문에 isVisibleSheet 플래그 하나만을 바라볼 수 있습니다.

@vercel
Copy link

vercel bot commented Nov 14, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
11th-5team-fe ✅ Ready (Inspect) Visit Preview Nov 22, 2022 at 2:02PM (UTC)

Copy link
Contributor

@sangbooom sangbooom left a comment

Choose a reason for hiding this comment

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

수고하셨습니다~~👍🏻

};

const onClose = useCallback(
(event?: MouseEvent<HTMLElement | HTMLButtonElement>) => {
event && event.stopPropagation();
Copy link
Contributor

Choose a reason for hiding this comment

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

stopPropagation을 넣으신 이유가 어떤 상황을 고려하신건가용?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

제가 잘못 넣었습니다.. 테스트하다가 넣었네요.. 제거했습니다!! 5d8830f

Base automatically changed from feat/home-change to main November 22, 2022 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Enhancement 📈 Enhancement to our codebase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants