[1단계 - 영화 목록 불러오기] 우디(류정우) 미션 제출합니다. #44
Conversation
첫번째 페이지가 아닐 경우, 기존의 영화 목록에 새로 받아온 데이터를 추가한다 현재 페이지가 토탈 페이지와 같을 때, 페이지에 대한 상태 값을 마지막 페이지를 나타내는 값으로 변경한다
외부에서 render를 재사용하지 않는 경우에는 초기화만 수행한다
|
마지막 미션까지 파이팅하세요~ |
There was a problem hiding this comment.
안녕하세요
@evencoding 👋
저는 리뷰어 포코라고합니다!
반갑습니다!
소문의 포코라니.. 착하게 살아야겠어요 😭
피드백
Store
상태의 CRUD를 모두 모아놓은 느낌인데요.
대중적인 Store의 느낌은 들지 않는 것 같아요. 🤔
차라리 Proxy만 써도 Store는 충분히 대체할 수 있지 않을까요?
차차 이해하면 좋겠지만 결국 Store라는 것을 이미 아는 사람이 봤을때.. 뭔가 이질감이 든다랄까요?
컴포넌트들이 수행하는 로직들과 미묘한 차이점이 있는 로직들을 잘 나눠보시고요.
Store 내부에 있는 메서드와 상태의 컨벤션도 일관성을 갖추셔야할 것 같습니다.
Proxy
Proxy를 학습 목적으로 사용하는 것은 공감하는데 그래도 필요한 이유가 있을까요? 💭
이번 미션에서는 API를 얼마나 효율적으로 호출하고 대응하느냐가 중요할 것 같거든요!
일관성
전반적으로 일관성이 부족한 것 같아요.
컨벤션, 포맷팅, 코드 사용에 대한 일관성을 최대한 맞춰보시면 좋겠습니다.
�그럼 수정 후 다시 리뷰 요청해주세요!
.prettierrc
Outdated
There was a problem hiding this comment.
"singleQuote": true처럼 몇몇 설정은 프리티어 확장 프로그램을 설치했을 때, 기본 설정으로 적용되는데, 이걸 말씀하시는 걸까용??
하지만 저는 종종 프리티어의 기본 설정을 임의로 변경해 주기도 했습니다
만약 다른 사람과 프로젝트를 공유할 때, 그 사람의 프리티어 기본 설정이 어떻게 되어 있는지 모르기 때문에, 프로젝트 별로 명시해 두는 편이 좋다고 생각했어요
이미 기본 설정 값은, 따로 설정 할 필요가 없다고 생각하시나요?
There was a problem hiding this comment.
넵
저는 기본 설정이 똑같다면 따로 명시할 필요는 없다고 생각합니다 ㅎㅎ
특히 Prettier는 설정할 Config도 몇개 없어요!
그리고 프로젝트와 공유를 할때 기본 설정을 위해 밖아놓는다는 것은 사실 공감이 잘 안되네요!
| cy.intercept( | ||
| { | ||
| method: 'GET', | ||
| url: /^https:\/\/api.themoviedb.org\/3\/movie\/popular*/, | ||
| }, | ||
| { fixture: 'movie-popular.json' } | ||
| ); |
| cy.get('.item-list').children().should('have.length', 20); | ||
| }); | ||
|
|
||
| it('검색 시에 query를 포함하는 영화 목록이 렌더링 되고, "더보기" 클릭시에 해당 쿼리를 가진 데이터가 추가로 렌더링 되는지 확인한다. 마지막 페이지에 도달했을 때 "더보기" 버튼이 사라진다.', () => { |
There was a problem hiding this comment.
테스트를 하는데 과하게 많은 코드가 작성되는데 의도가 있으실까요?
테스트 코드를 작성한다는 것은 좋지만 공수가 엄청나서 테스트 코드 유지가 어려울 것 같아요
There was a problem hiding this comment.
마지막 페이지에 도달했을 때 "더보기" 버튼이 사라진다.부분을 검사하기 위해서는 "더보기" 클릭시에 해당 쿼리를 가진 데이터가 추가로 렌더링 되는지 확인한다.가 먼저 확인되어야 한다고 생각했어요
완전히 같은 부분은 다시 쓰고 뒤에 하나의 테스트를 더 추가하는 방식으로 작성되서 하나로 통일시켜 줬는데 이 부분은 조금 더 고민해보겠습니다
|
|
||
| --- | ||
|
|
||
| ## 객체 역할 책임 협력관계 |
| | MoreButton | 영화의 정보를 추가적으로 불러온다 | 데이터를 추가적으로 불러온다 | 추가된 데이터를 무비 리스트에 전달한다 | | ||
| | WholeScreenMessageAlert | 사용자에게 메세지를 전달한다 | 예외 상황 발생 시, 해당되는 메세지를 전체 화면에 나타낸다 | ItemList의 자리에 삽입된다(ItemList), ItemView의 자리에 삽입된다(Store) | | ||
|
|
||
| ## 컴포넌트 |
There was a problem hiding this comment.
이런 설명보다는 도식화가 좋겠죠?
내부에 기능까지 명세화 할 필요는 없습니다!
src/components/movieItem.js
Outdated
| const commonTemplate = ({ title, vote_average, poster_path }) => ` | ||
| <img | ||
| class="item-thumbnail" | ||
| src="https://image.tmdb.org/t/p/w500/${poster_path}" |
src/Store.js
Outdated
| this.movies['results'] = | ||
| page === 1 ? results : [...this.movies.results.filter(({ title }) => title), ...results]; | ||
| this.movies['nextPage'] = total_pages === page ? -1 : page + 1; | ||
| this.movies['isError'] = false; |
There was a problem hiding this comment.
동적인 키 활용이 전혀 없는데 계속 this.movies['someKey']와 같은 문법을 사용할 이유가 있을까요?
There was a problem hiding this comment.
그렇..네요?
별 이유가 없었습니다🫠
추후 디버깅 하는데에 어려움을 겪을 수 있으니 수정하겠습니다!
There was a problem hiding this comment.
음 이 파일은 camelCase이고 구성이 다른 컴포넌트와 다르군요?
There was a problem hiding this comment.
맞아요
사실 현재 movieItem과 WholeScreenMessageAlert은 컴포넌트�가 되기에 애매하다고 생각했어요
템플릿에 가깝죠
물론 이 두 파일의 경우도 다른 컴포넌트 파일들와 비슷하게 Class 형태로 작성하는 것이 가능하지만 굳이 그렇게 해야할 이유가 없다는 생각이 들었어요
하지만, 추후의 확장성을 생각하거나 코드의 일관성을 위해서는 컴포넌트 내부의 모양을 마치 블루 프린트처럼 일관성있게 관리하는 것도 좋을 것 같다고 생각해요
이 부분에 대해서는 포코의 생각을 들어보고 싶어요
컴포넌트로 만들 필요성이 느껴지면, 그 때 컴포넌트화 하는 방식으로 진행하는 편이 좋은 걸까요?
There was a problem hiding this comment.
질문 자체가 모순 아닐까요..?
이미 컴포넌트 디렉터리 하위에 포함시켰으니.. 코드를 리뷰하는 재 입장에서는 컴포넌트로 간주하고 바라볼 수 밖에 없답니다 😭
| function movieItem(movie) { | ||
| const item = ` | ||
| <li> | ||
| <a href="#"> |
There was a problem hiding this comment.
템플릿으로 주어졌던 내용이라 따로 고민해보지 않았었네요
단순히 step2에서 이미지를 클릭하기 위한 용도로 넣어놨나보다~정도 생각했던 것 같아요😅
하지만 이미지를 클릭한다고 해도 같은 페이지에서 특정 위치로 이동을 하는 것도 아니고, url을 이동하는 것이 아닌데, 굳이 필요한가? 하는 생각이 드네요
실제로 눌러보면 항상 페이지의 최상단으로 이동하니 예상하는 동작과 다르구요
굳이 하이퍼링크가 필요하다고 한다면, 마우스가 없는 상황 등 특수한 경우에도 Tab키를 사용해서 이미지를 포커싱 할 수 있도록 하기 위함이 아닌가 생각했습니다
There was a problem hiding this comment.
<a>는 .. HTML에서 정말 가장~~~ 중요한 태그입니다.
잘 활용해보세요!
src/index.js
Outdated
| @@ -0,0 +1,7 @@ | |||
| // import Movie from './domain/Movie'; | |||
…에서 nonexistent로 변경
There was a problem hiding this comment.
리뷰 요청이 늦어져서 죄송해요😭
App, Proxy, Store를 이렇게도 바꿔보고 저렇게도 바꿔보고 되돌리기는 몇 번 하고보니 멘탈이 조금 흔들렸나봐요🫠
유지보수 하기 너무너무 어려운 코드를 작성했었나봐요..
현재 코드에서 가장 유지보수를 어렵게 한다고 생각했던 부분은
각 태그의 순서를 생각해 줘야 한다는 부분이었어요
컴포넌트에서 태그를 생성하고 insertAdjacentElement를 사용하다보니,
어떤 컴포넌트에 대해 재렌더링을 해야할 때, 순서를 맞춰주기 위해 변화가 일어나지 않았던 태그에 대해서도 다시 렌더링을 한다던지 하는 문제들이요
자잘한 부분들은 수정이 이루어졌지만, 커다란 무언가에 막혀서 제자리걸음을 하고있는 느낌이네요😭
작은 조언 하나 던져주시면 감사하겠습니다🥲
src/Store.js
Outdated
| const emptyArray = Array.from({ length: 20 }).map(() => { | ||
| return { title: null }; | ||
| }); |
There was a problem hiding this comment.
Array.from 을 자주 쓰다보니 별 생각없이 사용하게 되네요
포코 의견에 동의해요!
작성하기 전에, 역할에 맞는 메서드가 뭘까 한번 더 생각해봐야겠어요!
src/components/ItemList.js
Outdated
| import Store from '../Store'; | ||
| import WholeScreenMessageAlert from './WholeScreenMessageAlert'; | ||
| class ItemList { | ||
| // 렌더링 방식 변경하기... |
There was a problem hiding this comment.
주석에도 컨벤션이 있네요😲
다른 경우에 대해서도 공부해 봐야겠어요!
src/index.js
Outdated
| @@ -0,0 +1,7 @@ | |||
| // import Movie from './domain/Movie'; | |||
src/statusCode.js
Outdated
| switch (status_code) { | ||
| case 9: | ||
| return MESSAGE.SERVICE_OFFLINE; | ||
| case 24: | ||
| return OVER_REQUEST_TIME; | ||
| case 17 || 37: | ||
| return MESSAGE.SESSION_EXPIRE; | ||
| case 43: | ||
| return MESSAGE.SERVER_PROBLEM; | ||
| default: | ||
| return '알 수없는 오류입니다.'; |
There was a problem hiding this comment.
오오
const commandHandler = {
w: moveFront,
s: moveBack,
a: moveLeft,
d: moveRight,
};
const move = (command) => {
commandHandler[command]();
};if 문을 없애고 가독성을 높이기 위해, 사용자의 입력에 대해서 위와 같이 처리하는 것을 선호했었는데 이런 경우에도 충분히 활용해 볼 수 있겠네요!
src/statusCode.js
Outdated
| switch (status_code) { | ||
| case 9: | ||
| return MESSAGE.SERVICE_OFFLINE; | ||
| case 24: | ||
| return OVER_REQUEST_TIME; | ||
| case 17 || 37: | ||
| return MESSAGE.SESSION_EXPIRE; | ||
| case 43: | ||
| return MESSAGE.SERVER_PROBLEM; | ||
| default: | ||
| return '알 수없는 오류입니다.'; |
There was a problem hiding this comment.
const errorMessage = {
9: MESSAGE.SERVICE_OFFLINE,
24: MESSAGE.OVER_REQUEST_TIME,
17: MESSAGE.SESSION_EXPIRE,
37: MESSAGE.SESSION_EXPIRE,
43: MESSAGE.SERVER_PROBLEM,
};
export const statusCodeToErrorMessage = (status_code) => {
return errorMessage[status_code] || MESSAGE.UNKNOWN;
};위 처럼 변경 해 봤는데요
객체의 특성상, 17, 37는 같은 메세지를 반환하는데에도 불구하고 각각 프로퍼티로 넣어줘야 하더라구요
지금은 17, 37에 2개의 숫자에 대해서만 같은 메세지를 반환하지만, 만약 1, 2, 3, 4, 5, 6, 7, 8 혹은 그 이상의 숫자에 대해서 같은 메세지를 반환한다면 어떨까요?
그럴 때는 if 문, switch 문이 더 좋을 수 있겠다는 생각이 들어요
| | MoreButton | 영화의 정보를 추가적으로 불러온다 | 데이터를 추가적으로 불러온다 | 추가된 데이터를 무비 리스트에 전달한다 | | ||
| | WholeScreenMessageAlert | 사용자에게 메세지를 전달한다 | 예외 상황 발생 시, 해당되는 메세지를 전체 화면에 나타낸다 | ItemList의 자리에 삽입된다(ItemList), ItemView의 자리에 삽입된다(Store) | | ||
|
|
||
| ## 컴포넌트 |
| const { isProblem, template } = this.template(); | ||
|
|
||
| if (isProblem) { | ||
| this.$ul.innerHTML = ''; |
There was a problem hiding this comment.
ul태그에 안에 삽입하는 것이라면 따로 비워주지 않아도 괜찮다고 생각합니다
하지만 ul태그에 삽압히면 item-list 클래스의 영향을 받기 떄문에
ul을 비워주고, ul태그의 앞에 형제로서 삽입하도록 해줬습니다
src/components/Header.js
Outdated
| init($target) { | ||
| $target.insertAdjacentElement('beforeend', this.$header); | ||
| this.render(this.$header); | ||
| } |
There was a problem hiding this comment.
잘 이루어지지 않았지만, 관련이 있는 구문끼리 메서드로 묶어주고 싶다고 생각했어요
생성자 안에서 모든걸 처리하는 형식이 되어서요
예를 들어 event를 등록하는 부분을 bindEvent로, render를 담당하는 로직을 render매서드로 묶는 등의 방식으로요
어떤 방식으로 분리해주면 좋을지 찾아가는 과정이었습니다😭
src/components/Header.js
Outdated
| } | ||
|
|
||
| template() { | ||
| return `<h1><img src="${logo}" alt="MovieList 로고" data-type="logo" /></h1> |
There was a problem hiding this comment.
해당 이미지 태그를 찾기 위한 �유니크한 값을 지정해주고 싶었어요
id를 사용하는 편이 자연스러운 것 같아 수정했습니다!
| const { value } = currentTarget.querySelector('input'); | ||
|
|
||
| if (value.length === 0) { | ||
| alert('1글자 이상 입력해 주셔야 합니다.'); |
There was a problem hiding this comment.
현재 alert창이 뜨고 닫아도 input태그의 포커스가 유지되고 있는데 , 이 점을 말씀하신 걸까요?
어떤 부분으로�의 포커스 이동을 말씀하신건지 한번 더 여쭤보고 싶어요🥹
There was a problem hiding this comment.
@evencdoing
아주아주 훌륭합니다.
일부 아쉬운 부분은 있긴해요.
하지만 말씀하신대로 이렇게도 바꿔보고 저렇게도 바꿔보고 되돌리기는 몇 번 하고보니 멘탈이 조금 흔들렸나봐요🫠이 과정속에서 배우는게 많을겁니다.
결과는 중요치 않아요.
우테코는 작은 실패를 반복하며 다양한 크루들과 집단지성을 발휘하기 위한 공간입니다.
그러니 지금처럼 직접 선택하고 이리저리 삽질을 하고 삽질했던 내용을 정리하고 또 학습하면서 성장하시면 됩니다.
제가 요즘 아주 아주 좋아하는 명언이 있는데요.
우테코 레벨1이 끝날때쯤 내가 내린 선택들로 인생이 채워지고 있다는게 내 인생의 유일한 자부심과 같은 기분을 느끼셨으면 좋겠습니다.
이런 저런 힙한 기술, 그럴싸해보이는 용어, XXX 따라만들기에 휘둘리마시고 레벨1 순수 JS에서 만큼은 직접 요구사항을 분석해서 선택한 기술과 방법으로 가득 채우는 학습을 하셨으면 좋겠습니다!
어차피 레벨2에 진입하면 리액트 기반의 비슷한 코드가 양산되게 될겁니다.
그러니 지금 아주 잘하고 계신겁니다 👏
앞으로도 지금처럼 해주시면 됩니다.
그럼 스텝2에서 뵐게요 👋
질문이 있다면 코멘트, DM, 스텝2 PR 중 편한 방법으로 남겨주세요!
src/components/Header.js
Outdated
| return `<h1><img id="logo" src="${logo}" alt="MovieList 로고" data-type="logo" /></h1> | ||
| return `<h1><img id="logo" src="${logo}" alt="MovieList 로고" /></h1> |
| @@ -15,6 +15,8 @@ | |||
| <h2 class="movie-list-title"></h2> | |||
|
|
|||
| <ul class="item-list"></ul> | |||
|
|
|||
| <button class="btn primary full-width"></button> | |||
src/components/Header.js
Outdated
| this.init($target); | ||
| $target.insertAdjacentElement('beforeend', this.$header); | ||
|
|
||
| this.$header.addEventListener('click', this.onClickEvent); | ||
| this.$header.querySelector('.search-box').addEventListener('submit', this.onSubmitEvent); | ||
| this.render(); | ||
| this.bindEvent(); |
| } | ||
|
|
||
| render($target) { | ||
| this.$button.innerText = '더 보기'; | ||
| $target.insertAdjacentElement('beforeend', this.$button); | ||
| } | ||
|
|
||
| bindEvent() { | ||
| this.$button.addEventListener('click', this.onClickMoreButton); |
There was a problem hiding this comment.
onClickMoreButton보다는.. onClickMoreButton 이후 일어나는 액션에 대해 함수명을 지어주는게 어떨까요🤔
src/storeProxy.js
Outdated
|
|
||
| const $itemView = document.createElement('section'); | ||
|
|
||
| const storeProxy = ({ listTitle, itemList, moreButton }) => { |
src/components/ItemList.js
Outdated
| import Store from '../Store'; | ||
| import WholeScreenMessageAlert from './WholeScreenMessageAlert'; | ||
| class ItemList { | ||
| // 렌더링 방식 변경하기... |
There was a problem hiding this comment.
이건 컨벤션까지는 아니고요 😄
다양한 플러그인과 연동이 되는 아주 훌륭한 주석이랍니다!
| function movieItem(movie) { | ||
| const item = ` | ||
| <li> | ||
| <a href="#"> |
There was a problem hiding this comment.
<a>는 .. HTML에서 정말 가장~~~ 중요한 태그입니다.
잘 활용해보세요!
src/components/Header.js
Outdated
| init($target) { | ||
| $target.insertAdjacentElement('beforeend', this.$header); | ||
| this.render(this.$header); | ||
| } |
src/statusCode.js
Outdated
| switch (status_code) { | ||
| case 9: | ||
| return MESSAGE.SERVICE_OFFLINE; | ||
| case 24: | ||
| return OVER_REQUEST_TIME; | ||
| case 17 || 37: | ||
| return MESSAGE.SESSION_EXPIRE; | ||
| case 43: | ||
| return MESSAGE.SERVER_PROBLEM; | ||
| default: | ||
| return '알 수없는 오류입니다.'; |
src/domain/Movie.ts
Outdated
| interface IMovieProps { | ||
| curPage: number; | ||
| } | ||
|
|
||
| interface IFindMovieProps extends IMovieProps { | ||
| query: string; | ||
| } |
🎯데모 페이지
안녕하세요 포코!
클린과 함께 미션을 진행한 우디입니다
소문의 포코와 페어 리뷰를 할 수 있다니 설레이네요🥹
미션에서의 목표
이번 미션에서는 컴포넌트에게도 일을 시켜보고 싶었어요
이전 미션에서 제가 구현한 컴포넌트는
App 클래스로부터 전달받은 콜백함수로 이벤트를 등록할 뿐이었거든요그 결과
App 파일은 무거워지고, prop drilling이 심해졌죠🥲어떻게?
이전 미션에서 왜
App 클래스에서 모든 이벤트에 대한 콜백 함수를 구현해야 했을까..그 이유는 도메인에 있었다고 생각해요
도메인이 App 클래스에 인스턴스 변수로 존재했기 때문에, 도메인에 접근이 필요한 로직이라면 반드시
App 클래스에 존재해야했어요그럼 이번에는 어떻게 해야할까?
가장 먼저 든 생각은 도메인을 전역에 두는 것이었어요
모든 컴포넌트에서 도메인에 접근할 수 있으니, 해당 컴포넌트 안에서 앱에 필요한 어떤 로직이든 구현할 수 있을테니까요
하지만 위의 방식은 위험하다는 생각이 들었어요 (조금 막연한 생각이 들기도 합니다ㅠㅠ 정리되면 질문드릴게요옹!)
도메인이 생성자에 의해 여러 컴포넌트에서 인스턴스 변수로서 선언된다면, 도메인 클래스의 프로퍼티 값 등이 변경되었을 때 동기화가 되지 않는 등의 문제가 있기 때문이에요
때문에 Store라는 객체를 전역에 선언해서 상태와, 도메인 인스턴스를 할당하고 컴포넌트들은 Store 객체에 접근하는 방식을 사용했어요
이러한 방식 덕분에 proxy를 사용해 상태가 변경되면 다시 렌더가 되도록 구현할 수 있었습니다
컴포넌트의 렌더 방식
현재 컴포넌트들의 렌더링 방식에 대해 막연하게 아쉽다는 생각이 들어요
상태 변화가 일어났을 때, 태그들을 반복적으로 다시 생성해서 특정 dom에
insert하다보니 렌더링의 순서에 굉장히 신경써야하고, 변경이나 확장에도 취약하다고 생각해요또 코드 내에서 렌더링이 일어나는 부분이 여러 곳에 퍼져있어서 제가 짠 코드임에도 불구하고 파악이 쉽지 않았어요
반드시 리펙토링하고 넘어가고 싶은 부분이라고 생각해요