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

[점심 뭐 먹지 미션 Step 2] 황펭(양이진) 미션 제출합니다. #65

Merged
merged 9 commits into from
Apr 17, 2023

Conversation

Leejin-Yang
Copy link

안녕하세요 그루밍! step2로 돌아왔습니다 😊

배포주소

step2는 기능 추가 없이 다음과 같은 요구 사항이 있습니다.

  • Class Component를 Function Component로 마이그레이션
  • Custom Hooks을 이용하여 재사용 가능한 기능을 분리한다.

그리고 step1에서 그루밍이 리뷰해주신 부분 수정해보았습니다!
step1 PR

  • 컴포넌트 prop 타입 네이밍 수정
  • 이미지 로직 분리
  • Header 컴포넌트 title props

Custom Hooks

이번 미션을 진행하면서 두 개의 커스텀 훅을 만들어보았습니다.

  • useModal: 모달의 열려 있는지의 유무의 상태를 관리하는 hook
  • useRestaurants: 카테고리, 정렬 옵션 상태를 관리하고 음식점 리스트를 상태에 따라 반환하는 hook

커스텀 훅은 컴포넌트에서 사용하는 값이나 상태 관련 로직을 추상화해 재사용하는 기능이라고 생각하는데요.!
useModal의 경우다른 모달도 있다면, 열려 있는지의 유무의 상태를 정의하고 open, close 함수를 작성해주는 것이 중복된다 생각해 작성했습니다.
useRestaurants의 경우 RestaurantList 컴포넌트가 리스트를 필터링하고 정렬하는 것이 아닌 단순히 list를 받고 렌더링만 해주는 역할을 해야 한다 생각해 작성했습니다.
그루밍은 어떤 기준으로 hook을 정의하시나요?

@ddongule ddongule self-assigned this Apr 17, 2023
@ddongule ddongule self-requested a review April 17, 2023 12:40
Copy link

@ddongule ddongule left a comment

Choose a reason for hiding this comment

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

안뇽하세요 황펭!~ 그루밍입니닷! 🤓
step2도 잘 구현해주셨군요 👍
함수형 컴포넌트 사용해보니 어떠셨나요! ㅎㅎ 재미를 느끼고 계시면 좋겠네용 헤헿.
요번 미션은 step1에서 코멘트 남긴 내용도 잘 반영해주신 것 같구, 또 수정사항도 크지 않아 바로 머지해도 될 것 같습니다! 그래도 혹시 궁금한 점이 생긴다면 언제든 코멘트로 남겨주세욧!! 😉 꺅
질문주신 내용에 대해서 답변 쇽 남기고 이번 미션은 마무리하도록 하겠습니당 호홋

QnA

저는 주로 특정 컴포넌트나 어떤 구체적인 상황에 종속되지 않고, 범용적인 로직을 다룰 때에 많이 커스텀 훅으로 분리하는 것 같아요. 혹은 아예 어떠한 특정한 도메인 또는 기능에 집중하는 로직들을 분리하기도 해요. 저도 커스텀 훅을 처음 마주했을 때 황펭과 마찬가지로 어떤 기준에 따라 만들어야할지 고민을 많이 했던 기억이 나네요. . . 😵‍💫 최대한 많이 만들어보셔요! 어디까지 분리하면 좋을지 계속 고민하다보면 차차 감이 잡히실거에요👾! 호홋.

요번 미션두 고생 많으셨어요 황펭!~~!!!
궁금한 점 있으면 언제든지 알려주쎄욧!!!!!!
화이팅입니다!!!!! 😆🍎😆🍎😆

<h2 className={`${styles.name} text-title`}>{name}</h2>
<span className={`${styles.distance} text-body`}>캠퍼스부터 {distance}분 내</span>
<p className={`${styles.description} text-body`}>{description}</p>
<a href={link} className={styles.link} target="__blank">

Choose a reason for hiding this comment

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

__blank_blank의 차이를 한번 찾아보셔도 좋을 것 같아요!
더해서 noopener noreferrer도 함께 찾아보셔도 좋아용~👍

<div className={styles.backdrop} onClick={onClose}></div>
{children}
</>,
document.getElementById("modal-root") as HTMLDivElement

Choose a reason for hiding this comment

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

만약 요 dom이 없다면 에러가 발생할 수 있는 가능성이 있어보이네요!
null 체크를 통해 해당 dom이 없다면 null을 반환해주는 등의 확인 로직이 들어가면 더 좋을 것 같아요 ㅎㅎ

<p className={`${styles.description} text-body`}>{description}</p>
</div>
</li>
{isModalOpen && <RestaurantDetailModal restaurant={restaurant} onClose={closeModal} />}

Choose a reason for hiding this comment

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

이렇게 사용되면 restaurantList에서 item들이 렌더링되면서 modal도 함께 여러번 렌더링될 것 같아요.
상단에서 하나의 모달만 open/close 되면서 모달 내부에 있는 데이터들만 변경해주어도 괜찮을 것 같은데, 요런 방법도 한번 생각해보시면 좋을 것 같아용 😉

@ddongule ddongule merged commit dac54e3 into woowacourse:leejin-yang Apr 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants