-
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
[1단계 - 음식점 목록] 윤생(이윤성) 미션 제출합니다. #8
Conversation
- 컴포넌트 구조 설계 - 각 컴포넌트 상태와 핸들러 설계
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.
안녕하세요 윤생~
만나서 반갑습니다~~
전체적으로 코드가 일관되고 깔끔하게 작성되어 있어서 코드 레벨에서 코멘트 남길 부분이 많지 않았네요 👍 👍 👍
코멘트 남긴 내용 외에 추가로 얘기하고 싶은 부분이 있는데요.
- 폴더구조
윤생이 PR 본문에 남겨주신 컴포넌트 계층 구조만 보더라도 각 컴포넌트가 같은 레벨의 컴포넌트로 보이지 않는데요.
이런 부분들은 폴더구조를 통해서도 드러내주시면 좋을거 같아요.
- 네이밍
1번과 연결되는 내용인데요.
지금 윤생이 사용하고 계신 컴포넌트들을 보면 일반적으로 컴포넌트라고 부르는 재사용 가능한 모듈들이 아니라 특정 도메인, 로직에 종속적으로 사용되고 있는 컴포넌트들로 보여요.
이러한 컴포넌트들은 이름을 통해서 어디에서 사용하는 컴포넌트인지 명확하게 드러내 주시는 것도 좋을거 같네요.
한 가지 예시로 Modal
이라는 컴포넌트는 일반적으로 통용되는 Modal
과는 다르게 현재 새로운 음식점을 입력 받는 곳에서만 쓰일 수 있을거 같아요.
++) 여담으로 상하좌우에서 등장하는 UI에는 Modal보다는 Drawer라는 이름을 많이 사용합니다
위 처럼 계층화 되어 있다보니, 상위에서 관리해야 할 상태를 하위에서 사용해야 되면 props를 통해 폭포수 처럼 내려 주어야 되었습니다…(리액트의 Props Drilling 문제와 비슷하였습니다.) 리액트에서 하위 컴포넌트가 props가 많아 복잡해지는 것 처럼 저희 앱도 복잡해졌습니다. 이렇게 개발하면 불가피한 문제일까요? 더 좋은 해결 방법이 있을까요?
각각의 컴포넌트가 재사용이 가능한 모듈이 아닌 특정 도메인이나 로직에 종속되어 있기 때문에 발생하는거 같아요.
현재 상태로도 개선하는 방법이 여러가지 있겠지만 React like하게 작성하신다면 각각의 컴포넌트들을 재사용 가능한 모듈로 작성해보시는 것도 도움이 될거 같네요.
컴포넌트가 과연 무엇일까요? 느낌적으로는 무언가 레고블럭 같은 느낌이지만 막상 남에게 설명하려면 잘 와닿지 않네요 …ㅠㅠ 또 컴포넌트를 분리할 때 어떤 점을 고민해보면 좋을까요?
일반적으로 컴포넌트는 독립적으로 재사용 가능한 모듈을 말하는데요. 이게 윤생이 원했던 대답은 아닐거라는 생각이 드네요 😅
컴포넌트를 생각할 때 button, li와 같은 HTML Element단위로 생각을 해보시면 좋을거 같아요.
하나의 페이지를 구현하기 위해서 top-down 방식으로 전체를 만들어 나갈 수도 있겠지만 bottom-up 방식으로 구현을 한다면 HTML Element단위로 필요한 요소가 뭐가 있는지 고민해보고 만드신 후, 이러한 컴포넌트를 조합해 나가는 식으로 페이지를 구현하는 것을 CDD(Component Driven Development)라고 하는데요.
위와 같은 방식으로 코드를 작성한다면 분리할 때 어느 점을 고려할지 어느정도 설명이 되었을거 같아요.(안 되었다면 추가로 코멘트 남겨주세요)
이번 리뷰가 조금 늦어졌는데요. 다음 번 리뷰 요청 때는 빠르게 리뷰 남기도록 하겠습니다!
미션 진행하느라 고생 많으셨습니다 윤생! 👍 👍 👍
__tests__/unit.test.js
Outdated
new RestaurantList({ $parent: document.body, restaurants, category, sortBy }).render(); | ||
const restaurantList = document.querySelectorAll('.restaurant'); |
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.
거리 순으로 정렬됐는지 확인하기 위해 DOM을 파싱해와야 할까요?
UI 로직과 도메인 로직을 분리해서 테스트 해보셔도 좋을거 같아요
__tests__/render.test.js
Outdated
describe('RestaurantListItem 랜더링 테스트', () => { | ||
test('개별 요소 렌더링 테스트', () => { |
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/type.ts
Outdated
export interface Component<T> { | ||
$component: HTMLElement; | ||
state: T; | ||
setState: (state: T) => void; | ||
render: () => void; | ||
} |
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.
이 Component
는 다른 type들과 성격이 조금 달라보이네요. 별도의 파일로 만들어주셔도 좋을거 같아요
src/type.ts
Outdated
export interface Component<T> { | ||
$component: HTMLElement; | ||
state: T; | ||
setState: (state: T) => void; | ||
render: () => void; | ||
} |
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.
감사합니다~! 얼핏 공부한 C++ 템플릿이 생각나 적용해보았습니다!
src/App.ts
Outdated
toggleModal: this.toggleModal, | ||
}).render(); | ||
|
||
if (modalHide === 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.
strict하게 비교하는 것도 좋지만 이런 부분은 truthy, falsy한 값으로 비교를 해도 좋을거 같네요
src/components/RestaurantList.ts
Outdated
this.$component.append(fragment); | ||
}; | ||
|
||
getShowRestaurants = () => { |
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.
노출되는 Restaurant를 얻기 위해 이런 네이밍을 하신거 같은데요.
Show
보다는 sorted filtered 이런 이름들이 더 자연스럽게 느껴지는거 같아요
src/components/RestaurantList.ts
Outdated
const fragment = document.createDocumentFragment(); | ||
|
||
this.getShowRestaurants().forEach((restaurant) => { | ||
new RestaurantListItem({ $parent: fragment, restaurant }).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.
render
라는 메서드 이름만 보면 화면에 그려줄거 같은데 실제로는 fragment에 insert하는 역할을 수행하게 되는거 같네요.
보다 자연스럽게 느껴질만한 이름을 고민해보셔도 좋을거 같아요
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
는
- 가끔은 렌더를 하거나 (
innerHTML
) - 가끔은
fragment
에 붙이거나 (유조가 말씀해주신 것 처럼요) - 가끔은 하위 컴포넌트의
render()
를 호출하는..
등의 문제를 가지고 있네요. 아무래도 주먹구구식으로 개발했었던 것 같네요.
이 부분은 깊게 고민해보겠습니다.
src/components/Modal.ts
Outdated
<label for="category text-caption">카테고리</label> | ||
<select name="category" id="category" required> | ||
<option value="">선택해 주세요</option> | ||
${CATEGORIES.slice(1) |
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
이라는 숫자로는 CATEGORIES의 순서 말고는 아무런 정보도 얻을 수 없는데요.
의미가 있는 숫자라면 상수화를 해주셔도 좋을거 같네요
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/RestaurantListPage.ts
Outdated
}; | ||
|
||
getRestaurants = () => { | ||
return JSON.parse(localStorage.getItem('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.
"restaurants"와 같은 key 값들은 상수화 해서 사용해주시면 좋을거 같네요
src/components/Modal.ts
Outdated
|
||
submitForm = (e: Event) => { | ||
e.preventDefault(); | ||
const restaurants = JSON.parse(localStorage.getItem('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.
그리고 localStorage를 사용하다 보면 일종의 전역변수처럼 어디서나 같은 값에 접근할 수 있게 되는데요.
그렇다고 해서 아무데서나 접근해서 사용하는건 지양하시는 쪽으로 프로그래밍 하시면 좋겠네요!
++) 지금 윤생의 코드가 잘못되었다는 건 아니고 생각난 김에 얘기해봤습니다!
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.
넵! 감사합니다~!
추가적으로 말씀을 못드렸는데, 저희는 일단 문제에서 음식점 리스트를 등록하고 받아오는 로직을 서버에서 받아온다 생각하고 작성하였습니다! (미션을 하면서 서버를 구현해 진행하는게 학습 목표는 아니라, localStorage
가 미션을 위한 음식점 리스트 서버라고 생각하고 작성하였습니다.)
만약 그래서 localStorage
에 접근하는 코드가 서버에서 데이터를 받아오는 코드라면, 저렇게 쓰는게 맞을지, 아님 어디서 데이터를 가져오면 좋을지도 고민해보고 있습니다. 이 부분에서 주실 피드백이나 문제점도 있으시다면 알려주시면 좋겠습니다!!
- Header -> GNB(Global Navigation Bar)
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가 의미를 제대로 담지 못하고 있다고 생각합니다.
댓글에도 달아 놨지만 render 자체가 컴포넌트 별로 귀에 걸면 귀걸이, 목에 걸면 목걸이 식으로 동작하는 것 같습니다. 따라서 결론 내린 건…
컴포넌트가 항상 render를 해야 할까? RestaurantListItem 은 요소로 취급하고 render는 RestaurantList에서 하면 어떨까? 라는 생각을 해봤습니다. (물론 아직 고민만 하고 있습니다.) 유조의 의견은 어떠신가요?
네이밍으로 render라는 메서드가 하는 동작을 설명하지 못 하고 있다면 가장 단순하게는 메서드의 이름을 변경하는 것도 좋을거 같아요.
코드를 작성할 때 가장 중요한건 일관성 있는 코드인데요. 일관성을 해치게 될 경우 이후 코드에서 부분마다 다르게 작성되어 코드를 읽는 사람으로 하여금 인지 부하를 유발하게 되는데요.
이 때문에 새로운 코드베이스에서 작업을 하게 되는 경우 기존의 컨벤션과 룰에 따라 납득이 가지 않거나 비효율적인 부분이 존재하더라도 기존의 컨벤션과 룰을 벗어나지 않도록 작업을 하는 경우도 많습니다.
이런 관점에서 윤생이 작성하신 render
라는 메서드는 일관성이 떨어진다고 생각했어요. 모두 render
라는 이름을 가지고 있지만 render
가 하는 역할을 서로 다르기 떄문인데요.
"모든 Class가 일관되게 render라는 메서드를 가지고 있다." 보다는 "모든 render 메서드는 일관된 동작을 한다."가 더 설득력 있고 자연스럽게 느껴질거 같아요.
어느 부분에 더 무게를 두고 작성했을 때 저와 같은 제 3자가 코드를 읽을 때 더 자연스러울지 고민해보시면 좋겠네요!
두 번째로는 addRestaurantDrawer 렌더링 시 기존에 선택한 카테고리가 초기화되는 문제입니다.(드롭다운 메뉴 선택 후, 추가 버튼 누르면 기본값으로 초기화됨) 상위 컴포넌트가 렌더링이 됨에 따라 하위가 다시 렌더링 되면서 초깃값을 받는 게 문제가 되는데요.. 그래서 다음과 같이 생각해보았습니다.
새로운 구조로 애플리케이션을 작성할 때 기존의 좋은 구조를 참고하는건 자연스러운 일이라고 생각합니다. 윤생이 고민하고 계신 방법들 모두 그럴싸한 방법들이라고 생각 되는데요.
이왕 React like하게 구현을 시작하신 김에 React에서는 이런 re-render를 어떻게 방지하고 있는지도 같이 찾아보시면 좋을거 같네요! (React를 만들라는 얘기는 아닙니다ㅋㅋ)
어떤 방법이 좋다는 정답은 없는 부분이니 고민을 너무 오래 가져가진 마시고 직접 구현해보시면서 생각하신 방법을 적용했을 때 어떤 장단점이 있는지 느껴보셨으면 좋겠네요!
이번 step의 요구사항들은 대부분 충족하고 있는거 같아요!
다음 step으로 넘어가서 새로운 요구사항들과 함께 지금 고민하고 계신 내용들을 버무려봐도 좋겠네요!
고생 많으셨습니다 윤생! 👍 👍 👍
import RestaurantListItem from '../src/components/RestaurantListItem'; | ||
import RestaurantList from '../src/pages/RestaurantListPage/RestaurantList'; | ||
import RestaurantFilterContainer from '../src/pages/RestaurantListPage/RestaurantFilterContainer'; | ||
import GNB from '../src/components/GNB'; |
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.
GNB
를 Global Navigation Bar의 약자로 사용하신거 같은데요. 이런 단어는 실제 코드나 컴포넌트를 작성할 때 직접적으로 잘 사용되지 않아서 낯설게 느껴지네용
src/components/Modal.ts
Outdated
submitForm = (e: Event) => { | ||
e.preventDefault(); | ||
const restaurants = JSON.parse(localStorage.getItem('restaurants') ?? '[]'); | ||
restaurants.push(this.getFormValues()); | ||
localStorage.setItem('restaurants', JSON.stringify(restaurants)); | ||
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.
아닙니다 부끄러울 필요까지는 없을거 같아요
개행도 일종의 코딩 컨벤션이라고 생각하고 적용해보시면 좋을거 같네요!
src/components/Modal.ts
Outdated
this.render(); | ||
} | ||
|
||
setState = (newState: ModalState) => { |
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.
추가로 화살표 함수로 선언한 메서드와 일반 함수로 선언한 메서드가 생성자 함수(new)를 통해 Class를 생성할 때 어떻게 다르게 동작하는지도 찾아보시면 좋을거 같네요!
안녕하세요 유조! 반갑습니다 :)
이번 페어 미션은 @bassyu(쵸파) 와 진행하였습니다!
이번 미션은 다음과 같은 목표로 진행하였습니다~!
📍 학습 목표
어플리케이션을 컴포넌트 단위로 모듈화하여 개발
UI를 컴포넌트 단위로 생각하고 개발하는 연습
컴포넌트 단위로 생각하고 개발하려 하다보니, 딱 떠오르는게 리액트 였습니다. 그래서 리액트를 비슷하게 나마 따라해보았어요. 아래 설명을 작성해놓겠습니다!
재사용할 수 있는 컴포넌트를 고민해보기
이 부분은 많이 달성하지 못한 것 같아요. 특히
Modal
같은 컴포넌트는 너무 크다고 생각하는데 분리하려는데 생각보다 시간이 얼마 남지 않아서 추후로 미뤘답니다 😂웹 UI 환경에서의 테스트 기초
컴포넌트 단위 테스트 (1단계)
컴포넌트 단위 테스트는 어떤 식으로 해야 할지? 감이 잘 오지 않았습니다. 도메인 로직과 UI 로직을 따로 테스트 해보라고 하였는데, 저희가 구현한 방식은 두 로직이 묶여있어서 어떻게 해야할지 고민하다가 컴포넌트 안에서 도메인 기능을 하는 테스트는
unit.test
에, 렌더링 등의 테스트는render.test
에 넣었습니다!TypeScript의 기본 문법을 익히며 필요성을 경험
처음 타입스크립트를 경험해보아서 필요성을 좀 많이 느꼈습니다.. 특히 생각보다 저희가 내는 타입 실수가 굉장히 많았습니다! 그리고 직접 구현해보니 다른 객체지향 언어들에서 쓰는 타입 시스템이랑 굉장히 비슷하다는 걸 느꼈습니다. (C++의 템플릿, java의 인터페이스)
컴포넌트 설계
컴포넌트는 모든 컴포넌트가 다음과 같이 구성되어있습니다.
이러한 컴포넌트가 계층 구조로 이루어져 있습니다. 또한 상태도 컴포넌트 자체가 직접 관리하게 됩니다.
컴포넌트 계층 구조는 다음과 같습니다.
고민 해본 내용
이상입니다!