-
Notifications
You must be signed in to change notification settings - Fork 89
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
[2단계 - 자주 가는 음식점] 황펭(양이진) 미션 제출합니다. #74
Conversation
src/components/RestaurantCard.ts
Outdated
onClickFavoriteButton() { | ||
const $restaurantCardList = | ||
document.querySelector<RestaurantCardList>(".restaurant-list"); | ||
|
||
if ($restaurantCardList?.dataset.view === "favorite") { | ||
$restaurantCardList?.setRestaurants("favorite"); | ||
$restaurantCardList?.render(); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RestaurantCard에서 FavoriteButton 추가 이벤트입니다!
onClickFavoriteButton() { | ||
const $restaurantCardFavorite = document.querySelector<FavoriteButton>( | ||
`.favorite__button button[value="${this.#restaurantId}"]` | ||
); | ||
$restaurantCardFavorite?.toggleIsFavorite(); | ||
|
||
const $restaurantCardList = | ||
document.querySelector<RestaurantCardList>(".restaurant-list"); | ||
|
||
if ($restaurantCardList?.dataset.view === "favorite") { | ||
$restaurantCardList?.setRestaurants("favorite"); | ||
$restaurantCardList?.render(); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RestaurantDetailModal에서 FavoriteButton 추가 이벤트입니다!
두 컴포넌트에서 자주 가는 음식점 탭인 경우 RestaurantCardList를 렌더링하는 로직이 중복되고 있습니다 😭
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
안녕하세요 황팽! 리뷰가 늦어 죄송합니다 🥲
css 파일을 분리 잘 하신 것 같아요! 보기 좋습니당 👍
딱히 리뷰는 없어서 코멘트 확인하고 댓글 남겨주시면 될 것 같습니다!
질문 답변
- 말씀해주신 중복 로직의 경우 restaurantCardList 에 중복되는 로직을 모아 함수로 만들어두면 되지 않을까 싶습니다!
- 음 커스텀 이벤트를 생성해서 각자 알아서 동작하도록 만드는 방법도 있을 것 같아요!
.should("contains.text", name); | ||
}); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
깔끔하군요! 👍
src/components/FavoriteButton.ts
Outdated
import restaurantState from "../states/restaurants"; | ||
|
||
class FavoriteButton extends HTMLButtonElement { | ||
#isFavorite: Boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Boolean 과 boolean 의 차이는 뭘까요? 그리고 boolean 을 사용하면 좋을 것 같습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- boolean: 원시 타입
- Boolean: boolean 값을 감싸고 있는 객체, 생성자 함수
isFavorite
의 값은 true
혹은 false
이니 boolean
으로 수정하겠습니다.!
실수가 Boolean
에 대해 알아보는 기회가 되었네요 👍
super(); | ||
|
||
this.#isFavorite = | ||
restaurantState.getTargetRestaurant(this.value)?.isFavorite || false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this.value 도 상단에 isFavorite 처럼 멤버변수 목록으로 작성해주세요~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HTMLButtonElement
에서 상속해 value
의 값을 그냥 가져와 사용했습니다.
혹시 value
에 대해서도 따로 멤버변수로 등록해야 할까요.?
#value: string;
constructor() {
super();
this.#value = this.value
}
위와 같이 작성하는 것만 생각이 나는데.. 제가 이해한 것이 맞을까요 🧐
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아!!!! 맞네요 아닙니다!
src/components/FavoriteButton.ts
Outdated
|
||
this.#isFavorite = | ||
restaurantState.getTargetRestaurant(this.value)?.isFavorite || false; | ||
this.onClick = this.onClick.bind(this); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
요건 왜 하는거죠?!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
진행하면서 코드를 작성할 때 이벤트를 제거하는 메서드가 있어 등록과 제거의 콜백을 동일시 하기 위해 이 작업을 진행했습니다. 메서드를 지우면서 해당 부분은 삭제를 못했네요 😅
} | ||
|
||
render(restaurant: Restaurant) { | ||
connectedCallback() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
생명주기 활용 좋네요!
안녕하세요 헤인티! 놓치고 있었던 부분, 또 질문에 대한 키워드를 주셔서 감사합니다 😁 리팩터링
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
수정하신 내용 확인했습니다! 이번 미션을 통해 타입스크립트에 대해 맛도 보고 e2e 테스트에도 발을 담궈보는 좋은 경험이 되셨길 바라요 ㅎㅎ 고생 많으셨습니다!
안녕하세요 헤인티! 주말 잘 보내고 계신가요?
2단계 진행 후 리뷰요청 드립니다 😁 우선 1단계 마지막 코멘트에 답변을 달자면,
label
label
의 경우 주어진 템플릿을 가져와 사용하다보니 헤인티가 말씀해주신 에러를 발견하지 못했습니다 😭 아마text-caption
은 클래스네임인 것으로 보여 제거해label
의 역할을 수행하게 하였습니다!isSortingFilterAttribute
이 메서드의 경우
newValue
의 타입을 좁히기 위해 사용했습니다.속성 값은string
또는null
타입을 가지기에category
필드에 타입의 맞는 값인지 확인하기 위해 작성했습니다.CategoryFilterOption
과 같이 선언한 타입에 대한 확인 방법이 위와 같이 메서드를 작성하는 방법 외에typeof
와 같은 연산자가 있는지 혹은 다른 방법이 있는지 궁금합니다!배포 사이트
추가 구현
자주 가는 음식점 등록/제거, 음식점 삭제, 음식점 상세 정보 확인, 자주 가는 음식점 탭 기능을 추가했습니다!
Restaurants
FavoriteButton
RestaurantCard
RestaurantDetailModal
TabMenu
e2e 테스트
어려웠던 점
자주가는 음식점 등록/삭제 시 버튼 및 리스트 렌더링에서 고려해야 할 상황이 많았고, 다루기 어려웠습니다..
그렇다 보니 즐겨찾기 버튼에 추가이벤트를 받을 수 있도록 했습니다.!
이 과정에서 중복되는 로직이 생겼습니다 😭 이렇게 여러 컴포넌트에서 쓰이는 컴포넌트에 대한 이벤트를 정의할 때 각각 정의해서 추가해주는 방식 외에 다른 방법이 있을까요.? 해당 중복되는 로직을 어디서 정의해야 할지 궁금합니다!
궁금한 점
현재 음식점 리스트의 추가/삭제/즐겨찾기/필터/정렬 등 상태의 변경이 있을 때
RestaurantCardList
의 속성값의 변화를 주고 그 변화를 감지해Callback
이 실행되도록 구현했습니다. 2단계를 진행하며 느낀 점이 즐겨찾기의 속성이 추가되다 보니, 이후 다른 속성들이 추가된다면 각 속성의 경우를 모두 따져주어야 한다는 생각과 해당Callback
의 길이가 길어질 것이라는 생각이 듭니다 😅 각각의 속성의 경우를 전부 나누는 것이 아닌 상태의 변경(추가, 제거, 즐겨찾기 값 변경 등)을 감지한다면 해당 경우를 나누지 않아도 될 것이란 생각이 듭니다! 해당 내용에 대한 키워드가 있을까요.?