-
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단계 - 음식점 목록] 우디(류정우) 미션 제출합니다. #56
Conversation
… 음식점들로 각각의 음식점을 렌더하는 클래스로 변경
생성자들의 조합으로 앱을 구현할 수 있을거라 생각했지만 그러지 못했다. 전역에서 상태를 관리한다면 가능할 것 같다는 생각이 든다
식점의 id를 사용해 전체 음식점이 담긴 배열을 업데이트하는 기능 구현
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단계 구현하시느라 고생많으셨습니다:)
관련해서 리뷰 남겼고, 확인 부탁드려요~
우디가 PR에 작성해주신 것을 보면서, 미션을 수행하기 위해서 많은 고민을 하셨구나 싶었습니다. 이렇게도, 저렇게도 해보면서 어떤점이 좋았고, 어떤 점이 별로였고 하는 것들을 고민한 시간을 가진 것 같아서 좋네요👍
우디가 아쉬웠거나, 궁금한 점 들이 있어서 읽어보았는데요,
전체적으로 UI에 대한 분리가 제대로 이루어지지 못했던 것 같아요 대표적으로 modal에 대해
음식점 추가 모달
과음식점 디테일 모달
이 다른 태그로 작성되었어요ㅠㅠ
- UI 분리가 제대로 이루어진다는 것은 무엇일까요?
- 재사용과 관련된 거라면 modal에 대해서 modal 컴포넌트의 재사용성을 높일 수 있는 방법은 무엇이 있을까요? 이 부분만 더 고민해 보셔도 좋을 것 같아요.
왜 컴포넌트들을 클래스로 구현했어야 핬나?
컴포넌트들을 인스턴스 화 하지않고 생성자를 사용해 생성만 하는 이유가 뭐지?
UI에 대한 분리가 전혀 이루어지지 않고 있는데 어떻게 해결하면 좋을까?
이런 질문들도 있었는데 왜 컴포넌트들을 클래스로 구현했어야 했나~ 는 리뷰 코멘트로 남겼습니다. 다음 질문은 그것과도 관련있을 것 같아요. 꼭 생성자를 사용해 생성만 해야 할까요? 생성해두고 메서드를 추가해주는 방법은 없을까요? 아니면 컴포넌트를 꼭 생성자를 이용해서 생성해 주어야 할까요? 등등 에 대한 고민을 더 해보시면 좋겠습니다~
2단계 구현하느라 수고하셨습니다~!
+) 현재 예외처리가 empty만 되어있어서 특수문자(!, @등)를 입력하는 경우 앱이 정상적으로 동작하지 않는데요, 왜 동작하지 않는지 확인해 보시고 이 부분도 어떻게 해결하면 좋을 지 생각해보셔용~!
src/App.js
Outdated
sortOption: 'name', | ||
navTab: '모든 음식점', | ||
}; | ||
|
||
const restaurantsData = store.getLocalStorage(); |
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.
const restaurantsData = store.getLocalStorage(); | |
import { Restaurant } from '../types'; | |
const store = { | |
setLocalStorage(restaurants: Restaurant[]) { | |
localStorage.setItem('lunch_app_restaurants', JSON.stringify(restaurants)); | |
}, | |
getLocalStorage() { | |
const data = localStorage.getItem('lunch_app_restaurants'); | |
if (data) { | |
return JSON.parse(data); | |
} | |
}, | |
}; | |
export default store; |
이 함수가 utils 폴더에 들어있고, 이름도 getLocalStorage로 범용성을 띄고 있는데요, 실제 코드 내용을 보면 "음식점" 목록 만 받아올 수 있는 함수로 보여요. 이때 utils 의 이 함수를 이용해 사용자의 이전 탭, 이전 필터를 유지해야 해서 그 정보를 로컬스토리지에 저장하려면 어떻게 해야 할까요? utils 가 가져야 하는 범용성에 대해서도 생각해보시면 좋을 것 같아요!
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.
store의 메서드들이 key와 item을 주입받는 형식
으로 범용성을 높혔습니다
제네릭을 사용해서 item
에 타입을 적용해주고 싶었지만, 해당 메서드를 사용하는 App파일이 js로 작성
되었기 때문에
store.ts
파일도 store.js
로 수정했어요🥲
} | ||
|
||
onClickAddRestaurantButton() { | ||
$('.add-restaurant-form').reset(); |
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.
현재 코드에서는 구현되지 않았지만, 초기화 해야할 경우를 몇가지 생각해봤었습니다
submit
이벤트가 발생했을 때취소 버튼
을 눌렀을 때- 모달창이 열려있는 상태에서
esc 키
를 눌렀을 때 모달 배경
을 눌렀을 때
등등 폼을 초기화하는 메서드를 다양한 곳에서 사용해야 했어요
반면에 모달 창을 열 때 초기화를 해준다면 해당 메서드를 단 한번 사용하는 것으로 항상 초기화를 해줄 수 있었습니다
위의 이유로 입력 완료후가 아닌 입력 전 초기화를 해줬습니다!
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.
👍👍, 추가로 1-4 일때 결국 발생하는 이벤트가 close 라면 closeModal이라는 함수를 하나 이용해서, 1-4 에 동일한 동작을 함께 넣어주는 것도 가능할 것 같아요.
src/App.js
Outdated
} | ||
|
||
onClickNavTab(e) { | ||
const navElements = $$('.upper-tab > div'); |
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.
nav Element인데 선택자가 DOM 구조가 변경되면, 이 부분도 같이 수정해줘야 할 것 같아요. class 이름을 부여해서 관리해 보는 것은 어떨까요?
src/App.js
Outdated
if (clickedElement.innerText === '자주 가는 음식점') { | ||
$('.restaurant-filter-container').classList.add('hidden'); | ||
} else { | ||
$('.restaurant-filter-container').classList.remove('hidden'); | ||
} |
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.state.navTab 을 관리하고 있다면, this.state.navTab에 의해 관리가 되거나, id, class 명 등 변화가 적은 선택자를 사용하는것이 좋을 것 같아요. innerText로 관리하게 되면, 탭 이름과 같은 경우 수정될 가능성이 높은데, "자주 가는 음식점" => "즐겨찾기" 와 같이 변경된다고 할 때, 이 경우 html, js 에서 모두 수정을 해줘야 합니다. 그러다가 실수로 한 곳이 변경되지 않을 수도 있어서, 보다 변경가능성이 적은 것을 이용하면 좋을 것 같아요.
src/App.js
Outdated
this.state = { ...this.state, ...obj }; | ||
const key = Object.keys(obj)[0]; | ||
|
||
if (key === 'navTab' && obj[key] === '자주 가는 음식점') { | ||
return this.renderLikedItems(); | ||
} | ||
|
||
this.renderFilteredItems(this.state.filterCategory, this.state.sortOption); |
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.
obj는 항상 하나의 key에 대해서만 관리되나용? setState는 객체를 인자로 넘겨받고있지 때문에, this.setState({navTab: ~~, sortOption: ~~}) 과 같이 여러개의 state를 한번에 변경하는 것도 충분히 사용할 수 있을 것 같아요.
이미 윗 줄에서 this.state를 변경했기 때문에 인자로 받은 obj를 key값 배열로 변환하여 첫번째 값을 이용해주기 보다는,
this.state.navTab === '자주 가는 음식점' 과 같이 신뢰할 수 있는 값을 이용하는것이 더 좋을 것 같습니다~
export default class CreateRestaurantModal { | ||
constructor(onSubmitAddRestaurantForm, toggleAddRestaurantModal) { | ||
$('.add-restaurant-modal').innerHTML = html; | ||
|
||
$('.add-restaurant-form').addEventListener('submit', onSubmitAddRestaurantForm.bind(this)); | ||
$('.modal-close-button').addEventListener('click', toggleAddRestaurantModal); | ||
} | ||
} |
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.
쓱 훑어봤을 때 모든 컴포넌트에서 모든 작업을 contructor에서 하고 있는 것 같아요. 그래서 우디가 느끼기에도 왜 컴포넌트들을 클래스로 구현했어야 핬나?
라는 생각을 하신 것 같아요. class와 일반 함수 사이에는 어떤 차이점이 있을까요? 왜 우리는 어떤 것들에는 class를 사용하고, 어떤 것들에는 일반 함수를 사용할까요? 이 지점에서 먼저 고민해보면 좋을 것 같습니다.
현재와 같은 구조라면 Components 라고 꼭 class로 구현하기 보다, renderRestaurantModal(); 과 같이 하나의 함수로 구현되어도 괜찮을 것 같아요.
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.
어떤 객체에 속성이 존재하고, 메서드가 해당 속성과 연관있는 기능을 수행할 때,
어떤 객체가 인스턴스로서 재사용 될 때 클래스를 사용하기에 적절하다고 생각해요
제가 작성한 클래스들은 위의 경우를 만족하고 있지 않네요
서니 말대로 하나의 함수로 구현하는 것도 괜찮았던 것 같아요
현재는 이벤트에 대한 모든 메서드를 App으로부터 넘겨받고 있지만, 컴포넌트가 스스로 해결할 수 있는 경우에 대해서는 컴포넌트 안에서 구현하고 싶다고 생각하고 있어요
그렇게 된다면 new 생성자
로 생성하기만 하지 않고, 변수에 담아 하나의 인스턴스로서 컴포넌트를 관리하게 될 것 같아요
때문에 현재로서는 클래스가 아닌 함수로 구현하는 것이 적절해보이지만 클래스로 남겨두고 싶어요🥹
onChangeCategory(e) { | ||
const category = e.target.value; | ||
|
||
this.setState({ filterCategory: category }); | ||
} |
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.
App 의 하위에서 App의 값을 직접 변경하는 것은 위험할 수 있지 않을까요?
src/utils/dom.ts
Outdated
export const $ = (element: string) => document.querySelector(element); | ||
export const $$ = (element: string) => document.querySelectorAll(element); |
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.
인자 이름은 element보다는 queryString 같은게 더 잘 어울릴 것 같아요. element는 DOM 요소 같은 느낌이 약간 듭니당
updateRestaurant(id: string, liked: boolean) { | ||
const targetRestaurantIndex = this.#restaurants.findIndex((restaurant) => restaurant.id === id); | ||
|
||
this.#restaurants[targetRestaurantIndex].liked = liked; | ||
|
||
return deepCopy(this.#restaurants); | ||
} |
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.
deepCopy를 이용해주신 이유가 궁금합니다
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.
불변성을 지키기 위해서 deepCopy를 사용해줬습니다
전개 연산자 등을 사용해서 얕은 복사를 하는 경우에는 원시 타입에 대해서만 불변성을 확보할 수 있어요
음식점 리스트의 경우는 객체를 담은 배열로, 다차원 객체에 해당하기 때문에 deepCopy가 필요하다고 생각했습니다!
src/App.js
Outdated
onClickRestaurant(e) { | ||
const restaurantId = e.target.closest('li').id; | ||
const restaurants = this.restaurants.getRestaurants(); | ||
const restaurantIndex = restaurants.findIndex((restaurant) => { | ||
return restaurant.id === restaurantId; | ||
}); | ||
|
||
if ([...e.target.classList].includes('favorite-icon')) { | ||
return this.onClickStarIcon(restaurants, restaurantId, restaurantIndex); | ||
} | ||
|
||
new RestaurantDetailModal( | ||
restaurants[restaurantIndex], | ||
this.onClickDeleteButton.bind(this), | ||
this.toggleRestaurantDetailModal | ||
); | ||
$('.modal-open-button').addEventListener('click', this.toggleModal); | ||
$('.modal-close-button').addEventListener('click', this.toggleModal); | ||
} |
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.
요 부분에서 이벤트 위임을 사용해주신 것 같네용. closest 를 이용해서 돔 요소를 찾아주고 계시는데, 이 부분은 역시나 변경에 취약한 부분이 아닌가 합니다. 보다 명확하게, 어떤 이벤트를 실행해야 하는지가 정의되어있으면 좋을 것 같아용
+ target과 currentTarget에 대해서 학습해 보셔도 좋을 것 같아요.
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.
만약 모든 음식점 아이템에서 li 요소에 대해 click 이벤트를 등록한다면 currentTarget
을 사용해서 바로 원했던 DOM 요소를 가져올 수 있을거라고 생각해요
하지만 현재 전체 리스트(ul)에 이벤트를 등록
했기 때문에 target
을 이용해서 실제 이벤트가 발생한 요소를 확인해야만 그 부모 요소를 찾을 수 있다고 생각했어요
예를 들어 현재 코드에서는 음식점의 이름을 눌렀을 때,
currentTarget
은 ul 태그
를
target
은 아래의 h3 태그
를 가리키게 될 거에요
<h3 class="restaurant__name text-subtitle">${name}</h3>
음식점의 이름만으로는 부모 li 를 찾을 수 없으니 closest
가 사용될 수 밖에 없다고 생각했는데, 확실히 서니 말대로 변경에 매우 취약하다는 생각도 드네요
위의 문제는 어떻게 해결할 수 있을까요??
지금은 선택자가 아닌, 클래스 선택자를 사용하는 방법으로 변경했습니다
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.
이 경우에, 이벤트 위임을 사용하고 싶다면 closest를 사용하는 것을 변경할 필요는 없을 것 같아요. 우디가 사용해주신 선택자가 아닌, 클래스 선택자를 사용하는 방법으로 변경
으로도 충분할 것 같습니다~!
…오도록 변경 DOM 구조가 변경됐을 때, 해당 코드도 변경이 필요하기 변경이 적은 클래스 선택자를 사용
인자로 받은 obj를 key값 배열로 변환하여 첫번째 값을 이용해주기 보다, this.state.navTab === '자주 가는 음식점' 과 같이 신뢰할을 사용하도록 수정
… 권한을 줘서 이벤트 콜백 함수를 구현하지 않고, App으로부터 이벤트에 대한 콜백 함수를 넘겨받도록 변경
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.
안녕하세요 우디:) 전반적으로 코드가 뭘 표현하고 싶은지 확실해진 것 같아요. 미션 수행하느라 고생많으셨습니다 :) 오늘 또 다른 미션의 1단계 리뷰 요청이 올라오겠네요 ㅎㅎ 화이팅입니다~
추가로 코멘트 남겼고, 추가로 질문하고 싶은 지점이 있다면 편하게 말씀주세요. 고생하셨습니다~
it('', () => { | ||
cy.get('.modal-open-button').click(); | ||
}); | ||
}); |
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 테스트가 구현이 안된걸까요?
테스트에 대한 설명도 없고, assertion 도 없어서, 테스트코드라고 부르기 어려울 것 같습니다.
/// <reference types="cypress" /> | ||
// *********************************************** | ||
// This example commands.ts shows you how to | ||
// create various custom commands and overwrite | ||
// existing commands. | ||
// | ||
// For more comprehensive examples of custom | ||
// commands please read more here: | ||
// https://on.cypress.io/custom-commands | ||
// *********************************************** | ||
// |
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.
사용하지 않는 코드라면 아예 삭제해 주세요. support 에 있는거 다 삭제해도 테스트 코드는 돌아갑니당. e2e.ts 파일도 마찬가지로 사용하지 않는다면 삭제하는게 좋을 것 같아요~
{ | ||
"name": "Using fixtures to represent data", | ||
"email": "hello@cypress.io", | ||
"body": "Fixtures are a great way to mock data for responses to routes" | ||
} |
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.
이것도 사용안하는 데이터라, 삭제해주셔도 될 것 같네용
@@ -23,12 +22,15 @@ export default class App { | |||
navTab: '모든 음식점', | |||
}; | |||
|
|||
const restaurantsData = store.getLocalStorage(); | |||
const restaurantsData = store.getLocalStorage('lunch_app_restaurants'); |
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.
👍
여기에 로컬스토리지 키값이 늘어나더라도, 여러군데에서 사용되더라도 키 값이 겹치지 않게끔 상수로 관리해주면 더 좋을 것 같습니다.
@@ -42,20 +44,14 @@ export default class App { | |||
} | |||
|
|||
onClickNavTab(e) { | |||
const navElements = $$('.upper-tab > div'); | |||
const navElements = $$('.nav-button'); |
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.
어떤 요소를 찾는지 보다 명확해 져서 좋네용👍
@@ -42,20 +44,14 @@ export default class App { | |||
} | |||
|
|||
onClickNavTab(e) { | |||
const navElements = $$('.upper-tab > div'); | |||
const navElements = $$('.nav-button'); |
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.
어떤 요소를 찾는지 보다 명확해 져서 좋네용👍
const store = { | ||
setLocalStorage(key, item) { | ||
localStorage.setItem(key, JSON.stringify(item)); | ||
}, | ||
|
||
getLocalStorage(key) { | ||
const data = localStorage.getItem(key); | ||
|
||
if (data) { | ||
return JSON.parse(data); | ||
} | ||
}, | ||
}; | ||
|
||
export default store; |
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.
요건 추가로 생각해보면 좋은 것 들인데요, JSON.parse의 인자로 '' 나, undefined가 들어가면 어떻게 될까요? 한번 실행해 보시고, 어떻게 하면 더 좋을지 고민해보세용~!
배포 페이지
안녕하세요 서니!
벌써 주말이 다가왔네요😆
내일부터 즐거운 주말 보내세요!
E2E 테스트에 대해서는 적용시키지 못했어요ㅠㅠㅠㅠㅠㅠ
작은 단위에서부터 연습을 한 후에 현재 미션에 적용 할 수 있을거라고 생각해서 추후에 꼭 적용해보려고 합니다
먼저 1단계에서부터 지금까지 어떤 과정으로 지금의 코드가 되었는지 간략하게 공유하고 싶어요!
1
컴포넌트에 대한 개념은 얕게라도 알고 있었지만 어떻게 미션에서 적용시켜야 할 지를 잘 몰랐습니다
특히 왜 전혀 움직이지 않는 HTML을 컴포넌트로 만들어야할지 스스로 납득하기 어려웠어요
때문에, 동적으로 변하는 HTML에 대한 부분만 따로 파일로 분리해서 렌더링을 해줬습니다
구현을 하다보니 카테고리 등에 대해 상수로 선언해 둘 일이 있었고, 정적인 HTML이라도 선언된 상수에 따라 반복되는 태그에 대해서는 자바스크립트로 템플릿을 생성 한 후 렌더링하도록 적용시켜봤습니다
2
컴포넌트는 HTML 템플릿을 생성하고 렌더를 할 뿐인가?
App에서 이벤트 등록과 콜백을 모두 선언하다보니 크기가 커지기도 했고, 컴포넌트에도 어떤 역할을 부여하고 싶었습니다
컴포넌트 파일을 열어봤을 때, 어떻게 해당 컴포넌트의 역할을 알려줄 수 있을까?
위의 생각으로부터 컴포넌트안에서 이벤트를 등록하고 해당 이벤트의 콜백 함수를 구현하도록 변경했습니다
하지만 컴포넌트 안에서 이벤트 콜백 함수를 구현하니 아래와 같은 문제들이 있었어요
3
때문에 App파일에서 이벤트에 대한 콜백 메서드를 구현해두고 컴포넌트에 해당 메서드 넘겨주는 방식을 사용했어요
컴포넌트에 넘겨주는 인자를 최소화할 수 있었고, 컴포넌트 파일을 봤을 때 해당 컴포넌트가 어떤 역할을 담당하는지 훨씬 알기 쉬웠어요
4
음식점의 즐겨찾기 버튼과 상세 음식점 모달에 대한 이벤트를 각각의 음식점 컴포넌트에서 등록하고, 넘겨받은 메서드를 호출하는 방식으로 구현했었어요
그 결과 너무 많은 dom 조작이 발생했어요
때문에 음식점 목록에 이벤트를 등록하고 이벤트 버블링을 통해 dom 조작을 최소화 해줬습니다
지금 시점으로는 컴포넌트의 재사용성에 대해서 아쉬운 부분이 많은 것 같아요
RestaurantItem
컴포넌트는 모든 음식점, 자주 가는 음식점을 렌더할 때 재사용하고 있지만, 전체적으로 UI에 대한 분리가 재대로 이루어지지 못했던 것 같아요대표적으로 modal에 대해
음식점 추가 모달
과음식점 디테일 모달
이 다른 태그로 작성되었어요ㅠㅠ처음부터 페어와 함께 또는 스스로 고민하며 코드를 완성해 나갔기 때문에 제가 작성했던 코드에 대한 문제점을 직접 느껴보고, 그것을 해결하기 위해 고민하는 의미있는 시간을 보낼 수 있었어요
하지만 그렇다고해서 현재 코드에 대해 스스로 납득하기는 힘들었던 것 같아요
왜 컴포넌트들을 클래스로 구현했어야 핬나?
컴포넌트들을 인스턴스 화 하지않고 생성자를 사용해 생성만 하는 이유가 뭐지?
UI에 대한 분리가 전혀 이루어지지 않고 있는데 어떻게 해결하면 좋을까?
등등
서니와 다양한 이야기를 나눠보고 싶어요!