-
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단계 - 자주 가는 음식점] 해리(최현웅) 미션 제출합니다. #155
Conversation
- CategoryIcon 컴포넌트 분리 내용 추가
- 음식점 이름으로 식당 정보를 찾을 수 있다. - 유효한 입력일 경우 로컬 스토리지에 식당을 추가한다. - 음식점 목록에 있는 한 식당을 삭제한다
- 자식 클래스에게 상속할 메서드 protected 키워드 사용 - 커스텀 이벤트를 실행시키는 추상화 메서드 제네릭 타입 적용
- 자주가는 음식점인지 아닌지에 대한 상태 변경 - 자주가는 음식점 목록만 필터링
- 음식점 상세 정보 - 모든 음식점, 자주 가는 음식점 구분 탭 - 레이아웃, 폰트 크기, 굵기 공통 CSS 속성 추가
- 탭이 변경될 경우, currentTab 상태를 변경시키고 다시 렌더링 - 음식점 목록 컴포넌트에서 변경된 상태를 전달받고, 상황에 맞게 목록을 렌더링
- 음식점 아이템, 음식점 상세 정보 컴포넌트에서 동일한 카테고리 이미지 UI를 사용하기 때문에, 재사용할 수 있도록 컴포넌트 분리
- 음식점 상세 정보를 보여준다 - 음식점 목록 컴포넌트에서 이벤트를 보내고, 이벤트와 함께 온 세부 정보를 사용해서 렌더링한다 - 음식점 상세 정보를 여는 이벤트가 발생할 때마다, document에 이벤트 핸들러가 쌓이는 문제가 있어, addEventListener의 3번째 인자에 once를 true로 설정
- 음식점 목록 구분 탭 컴포넌트 구현 여부 반영
- 유요한 음식점 추가 데이터인지 타입 확인하는 메서드 추가 구현 - 음식점 추가 데이터를 가져오는 동작, 도메인 로직을 호출하고 도메인의 응답에 대해 이후 동작을 하는 메서드 분리
- 음식점 목록, 아이템, 상세 정보, 탭 구분 컴포넌트를 Restaurants 폴더로 이동
- 자주 가는 음식점을 볼 경우 해당 컴포넌트가 보여지지 않도록 동작하는 메서드 구현 - 유효한 필터링, 정렬 값인지 타입 확인하는 메서드 구현
- 새로운 식당을 추가하기 전, 식당 상세정보 모달을 렌더링하기 전 타입 확인 - 필터링, 정렬 옵션이 변경되기 전 타입 확인
src/utils/types.ts
Outdated
return Object.keys(item).every((key) => candidates.includes(key)); | ||
}; | ||
|
||
export const isValidOption = <T extends string>(options: T[], value: string): value is T => { |
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.
오 generic 활용 👍
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.
활용도가 높게 isArrayElement 같은 좀 더 일반적인 이름이어도 좋겠습니다. (위에 isRestaurantItemType 도 동일합니다.)
isArrayElement<T extends string>(array: string[], element: string): element is T
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.
확실히 isValidOption
보다는 추천해주신 네이밍이 여기저기서 더 범용적으로 사용될 수 있을 것 같네요! 함수 네이밍을 잘 짓는게 생각보다 너무 어려운 것 같아요..😅
{ | ||
name: "하루식당", | ||
category: "중식", | ||
distance: 20, | ||
isFavorite: false, | ||
description: | ||
"우아한테크코스 웹 프론트엔드 리뷰어 하루가 운영하는 식당. 사장님의 리뷰가 정말 좋은 것으로 유명하다. 😊", | ||
link: "", | ||
}, |
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.
이렇게 감동적인 테스트 코드는 처음이에요... 감사합니다 🙇
cypress/e2e/ResturantAddTest.cy.ts
Outdated
}, | ||
{ | ||
invalidData: { | ||
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.
위 테스트케이스는 13글자, 요구사항은 1~10글자로 확인했어요.
11글자의 케이스로 재구성해보면 어떨까요?
11글자, 12글자, 13글자, ... 모든 글자수에 대해 테스트를 작성하는 것은 비용대비 효과가 떨어지니까, 가장 유의미한 테스트케이스를 골라서 작성하는 것이 중요한데요, 보통 경계조건
에 대해 작성하는 것이 일반적입니다.
아래의 다른 invalid 케이스들도 경계조건을 검증하도록 변경되면 좋겠습니다.
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.
경계값을 중심으로 테스트 코드를 작성하는 것은, 프리코스 공통 피드백에서도 확인할 수 있었던 내용이였는데 잊었던 것 같네요..😅
- 식당 이름 : 11자
- 식당 설명 : 301자
로 테스트를 진행하는 것으로 수정해보았습니다!
cypress/e2e/ResturantAddTest.cy.ts
Outdated
{ | ||
invalidData: { | ||
name: "해리식당", | ||
category: "중식", | ||
distance: "", | ||
description: "", | ||
link: "", | ||
}, | ||
errorMessage: ERROR_MESSAGES.invalidDistance, | ||
}, |
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.
각 invalid 케이스 별로 이름을 지어주거나, 주석을 활용해서 구분을 해주면 어떨까요? 지금은 테스트 케이스만 보고는 어떤 것 때문에 invalid인지 한눈에 파악하기 어려워보여요.
코드를 작성한 순간이 요구사항에 대한 이해도가 가장 높은 때이고, 시간이 지나면, "내가 이 케이스를 왜 작성했더라. 뭘 확인하려고 했더라." 싶은 때가 있습니다. 제 코드를 보는 다른 분도 비슷하게 느끼실 수 있고요. 그래서 지금은 너무 잘 보이더라도 더 명시적으로 작성하는 방향이 좋습니다. 후에, 잘 작성된 주석이나 이름이 반가운 때가 있을 거에요.
distance: "", // distance 지정 누락
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.
코드를 작성한 순간이 요구사항에 대한 이해도가 가장 높은 때이고, 시간이 지나면, "내가 이 케이스를 왜 작성했더라. 뭘 확인하려고 했더라." 싶은 때가 있습니다.
오..정말 공감되면서 지금 저에게 딱 필요한 피드백인 것 같네요! 제가 작성한 코드라도 나중에 보면 정말 남이 작성한 것 처럼 왜 이렇게 짰는지 잊어버리거나 이해가 되지 않을때가 많았는데 테스트 코드 같은 경우는 주석을 활용해보는 것이 좋은 방법이 될 수 있겠군요. 특히나 예외 처리를 많이 해야하는 입력 폼과 관련된 테스트에서는 정말 필요할 것 같아요. 단위 테스트를 작성했을 때는,
// given
// when
// then
주석을 활용했었는데, E2E에서도 주석을 충분히 활용해볼 수 있을 것 같아요. 좋은 피드백 감사드립니다. 🙇♂️
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.
이 파일 이름에 케이싱이 잘못되었나봐요 OPtionSelectorContainer.ts
E2E 테스트 실행하려 서버 띄우니까 아래와 같은 에러 메시지가 뜨네요
[tsl] ERROR in src/index.js(11,8)
TS1261: Already included file name 'src/components/OptionSelector/OptionSelectorContainer.ts' differs from file name 'src/components/OptionSelector/OPtionSelectorContainer.ts' only in casing.
The file is in the program because:
Imported via "./components/OptionSelector/OptionSelectorContainer" from file '/Users/365kim/Desktop/Projects/2024-reviewer/javascript-lunch/src/index.js'
Root file specified for compilation
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.
헉..파일 이름에 오타가 있었군요 수정하도록 하겠습니다!
cypress/e2e/RestaurantDetail.cy.ts
Outdated
cy.get(".restaurant-list-container").children().first().click(); | ||
cy.get("#restaurant-detail-wrapper").should("have.class", "modal--open"); | ||
}); | ||
|
||
it("음식점 목록 중 하나의 음식점을 클릭하면, 해당 음식점 상세 정보를 보여주는 모달이 렌더링된다.", () => { | ||
cy.get("div.restaurant-detail").should("contain", "도스타코스 선릉점"); | ||
}); |
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.
음식점 목록의 첫번째 음식점을 클릭하면,
이라는 표현은 어떨까요? beforeEach의 처음에 first()
를 놓치면 전체적으로 맥락을 이해하기 어려울 것 같아요.
그리고 역시 첫 번째 요소가 "도스타코스 선릉점(맛있겠다..)"임을 알아야 이해되는 테스트로 보여요. 같이 개선해보면 좋을 것 같아요 :)
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.
이런 방법으로 테스트 코드를 작성하면 디폴트 데이터가 바뀔 때마다 테스트 코드도 함께 수정해줘야겠군요...변경에 취약한 테스트 코드를 작성한 것 같아요.
- 도스타코스 선릉점인걸 몰라도
- 디폴트 데이터가 변경되어도
테스트 코드가 통과되는데 아무 문제가 없도록 클래스 이름을 확인하는 것으로 테스트 케이스를 수정해봤습니다!
); | ||
}); | ||
|
||
it("새로고침을 해도 새로 추가한 음식점 정보가 유지된다.", () => { |
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.
모두 삭제한 이후 새로고침 했을 때는 상태가 유지되지 않고 있어요. 해당케이스도 테스트케이스로 구성 후 보완되면 좋겠습니다 :)
Screen.Recording.2024-03-19.at.1.37.55.AM.mov
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.
이 부분은 전혀 생각하지 못하고 있었네요..! 점심 뭐 먹지 미션을 진행하면서 사용자가 제일 처음 방문할 경우,
- 디폴트 데이터를 보여준다
- 아무런 식당이 없다는 메시지를 전달하는 UI를 보여준다
이 2가지 중 어떤걸 선택할지에 대한 고민을 했었는데, 전자를 선택했었습니다. 현재 보여지고 있는 식당 목록의 갯수가 0인 경우 디폴트 데이터를 보여주도록 하는 로직이 있어서 상태가 유지되지 않았던 것 같아요.
it("사용자가 모든 음식점을 삭제하고, 새로고침할 경우 디폴트 데이터가 다시 보여진다.", () => {
//...
이 테스트도 추가해보았습니다!
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.
해리, 2단계 작업해주신 것 잘봤습니다 :)
테스트 코드는 버그를 예방하는 수단인 동시에,
기능의 의도와 기대하는 행동을 문서화할 수 있는 좋은 개발 명세이기도 합니다. 해리의 테스트 코드를 읽으며 잘 작성된 명세를 읽는 듯 했어요. 👍 관심사 별로 모듈 분리해주신 것도 좋았습니다.
다양한 타입스크립트 문법도 필요에 따라 적극적으로 활용해주시고, 미션의 주요 요구사항 중 하나인 컴포넌트 재사용까지 잘 소화해주신 것으로 확인했습니다.
코멘트 몇 가지 남겼는데, 천천히 보시고 완료 되시면 리뷰 재요청 부탁드려요 :)
고생하셨습니다 👏👏👏
- 첫 번째 음식점 클릭시 모달이 렌더링 된다는 제목으로 테스트 케이스 수정 - 모든 음식점을 삭제할 경우 디폴트 데이터가 렌더링 되는지 테스트 추가
- 처음 방문할 경우 디폴트 데이터가 이름순으로 정렬되어있는지 테스트 추가
const { name, category, distance, description, isFavorite, link } = this.detailInfo; | ||
const { name, category, distance, description, isFavorite, link } = | ||
this.detailInfo; |
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.
prettier 설정이 바뀐 건가요? 일부 개행도 같이 잡히네용
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.
안녕하세요 하루! step2도 잘 부탁드립니다. 🙇♂️
배포 링크
점심 뭐 먹지 step2
실행 방법
npm run test
고민
재사용 가능한 컴포넌트
학습 목표였던, 재사용 가능한 컴포넌트를 고민해보면서
"2군데 이상에서 동일하게 사용된다면 공통 컴포넌트로 만들고 필요한 곳에서 재사용해보자."라는 나름대로의 기준을 가지게되었습니다.
Modal
위 2가지는 모두 모달 위에 UI가 렌더링되기 때문에, 모달을 공통 컴포넌트로 만들고 재사용 해보기로 했습니다.
공통 모달 컴포넌트는
위 2가지 책임을 가지고 있습니다.
CategoryImage
위 2가지 상황에서 카테고리 이미지가 사용되는데, 두 이미지 모두 UI가 동일해 공통 컴포넌트로 만들고 재사용 해보기로 했습니다.
공통 카테고리 이미지 컴포넌트는 카테고리에 따라서 다르게 보여질 이미지를 결정하는 책임을 가지고 있습니다.
FavoriteIcon
위 2가지 상황에서 별표 아이콘이 사용되는데, 두 아이콘 모두 UI가 동일하고 도메인이 가지고 있는 음식점 데이터의
isFavorite
의 상태를 변경시키는 책임이 같아서 공통 컴포넌트를 만들고 재사용 해보기로 했습니다.음식점 상세 정보 모달에서 별표 아이콘을 클릭할 경우,
isFavorite
상태가 변경되고filled <-> lined
로 UI가 변경되는데 모달 뒤에 있는 음식점 목록에도 이 변경사항이 바로 반영되면 좋을 것 같아 별표 아이콘을 클릭할 때마다 음식점 목록 컴포넌트가 다시 렌더링 되도록 했습니다. 렌더링 효율 측면에서 좋은 방법은 아닌 것 같으나, 이번 미션에서는 렌더링 효율 관련 고민을 하는 것은 학습 목표와 멀어지는 고민인 것 같아 우선isFavorite
의 상태 변경을 UI에 바로 반영하는 것에 초점을 맞추기로 했습니다.RestaurantTextInput
이 2가지 상황은 모두
input
태그 UI를 가지고 있고 음식점 추가 폼을 제출할 때, 입력 유형에 따라 유효성 검증을 하고 예외가 발생할 경우input
태그 밑에 예외 상황에 대한 에러 메시지를 출력하는 책임이 동일해서 공통 컴포넌트로 만들고 재사용 해보기로 했습니다.step2에서 고민해본 재사용 가능한 컴포넌트 목록들은 위와 같습니다. step1을 진행할 때 구현한 컴포넌트인
들도 step2에서 동일하게 활용했습니다. 😊
E2E
학습 목표였던, 사용자 관점에서 중요하다고 생각하는 기능을 스스로 정의하고 E2E 테스트로 검증해보기를 고민해보면서 중요하다고 생각하는 기능을 나름대로 정리해보고, 그 기능을 중심으로 E2E 테스트를 진행해 보았습니다.
음식점 추가 기능 테스트
음식점 목록 테스트
음식점 상세 정보 테스트