-
Notifications
You must be signed in to change notification settings - Fork 0
KDT5_송수연 2차과제제출_4 #72
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
base: main
Are you sure you want to change the base?
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.
고생하셨습니다.
전체적으로 잘 구현해 주셨습니다.
컴포넌트 분리 및 기존 강의에서 제공하는 코드도 어떤 동작을 하는지
이해하고 사용하시는 것으로 보입니다.
추가적으로는 검색 결과가 10개 미만인 경우에
MovieListMore 컴포넌트가 보이는 문제가 있는 것 같습니다.
EX) zozo 로 검색 시 load more 버튼이 보이지 않아야 할 것 같습니다.
core 폴더 내부 song.js 는 파일명이 어떤 것을 의미하는지 애매한 것 같습니다.
commit 도 나누어 작성해 주시면 좋을 것 같습니다.
| }) | ||
| movieStore.subscribe('loading', () => { | ||
| this.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.
subscribe로 store의 값이 변화 될 때 재렌더링을 위해 작성하신 부분인데
구독하는 대상이 3개나 있는데 loading 하나로 하여도 충분하지 않을까요?
(data fetch 시작과 종료에 따라 재렌더링이 이루어지면 될 것 같습니다.)
| super({ | ||
| tagName: 'button' | ||
| }) | ||
| movieStore.subscribe('pageMax', () => { |
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.
구조상 pageMax 값이 동일하더라도
할당이 이루어져 재렌더링이 일어나지만
의미적으로는 page 의 변화를 바라보고 있어야 할 것 같습니다.
현재 구조에서는 아래처럼 될 것 같아요.
| movieStore.subscribe('pageMax', () => { | |
| movieStore.subscribe("page", () => { | |
| const { page, pageMax } = movieStore.state; | |
| if (page === 1) { | |
| this.el.classList.remove("hide"); | |
| return; | |
| } |
| ` | ||
|
|
||
| const inputEl = this.el.querySelector('input') | ||
| inputEl.addEventListener('input', () => { |
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.
event 받아서 처리하는 방법도 있을 것 같습니다.
| inputEl.addEventListener('input', () => { | |
| inputEl.addEventListener("input", (event) => { | |
| movieStore.state.searchText = event.target.value; | |
| }); |
| @@ -0,0 +1,391 @@ | |||
| html { | |||
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.
변수 선언은 html이 아닌
:root 로 하시는게 일반적 입니다.
| html { | |
| :root { | |
| --color-black: #1d2f69; | |
| --color-white: #fff; | |
| --color-white-50: rgba(255, 255, 255, 0.5); | |
| --color-white-30: rgba(255, 255, 255, 0.3); | |
| --color-white-20: rgba(255, 255, 255, 0.2); | |
| --color-white-10: rgba(255, 255, 255, 0.1); | |
| --color-white-5: rgba(255, 255, 255, 0.05); | |
| --color-primary: #ff639c; | |
| --color-hover: rgb(255, 0, 179); | |
| --color-area: #1c212e; | |
| } |
| .the-loader { | ||
| width: 30px; | ||
| height: 30px; | ||
| margin: 30px; |
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.
축약으로 주셔도 좋습니다. 좌우에 margin 50%를 주어
로딩 컴포넌트가 가운데에 뜨는 것이 더 좋지 않을까 싶습니다.
| margin: 30px; | |
| margin: 30px; | |
| margin-right: 50%; | |
| margin-left: 50%; |
| this.el.classList.add('container', 'about') | ||
| this.el.innerHTML = /* html */ ` | ||
| <div | ||
| style="background-image: url(${photo};" |
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.
인라인 스타일을 사용하지 않고 구현 할 수 있는 방법을 고려해 보시면 좋을 것 같습니다.
| ` | ||
|
|
||
| await getMovieDetails(history.state.id) | ||
| console.log(movieStore.state.movie) |
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.
개발 완료 후에 console.log는 지워주시는 것이 좋습니다.
|
|
||
| this.el.innerHTML = /* html */ ` | ||
| <div | ||
| style="background-image: url(${bigPoster})" |
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.
동일하게 인라인 스타일로 주지 않고 구현하는 방법을 고려해 보시면 좋을 것 같습니다.
| photo: 'https://ifh.cc/g/7QrFfq.png', | ||
| name: 'SongSuYeon', | ||
| github: 'https://github.com/suyeon9937', | ||
| repository: 'git@github.com:suyeon9937/practice-movie.git' |
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 설명 부분은 |
| store.state.message = Error | ||
| } | ||
| } catch (error) { | ||
| console.log('searchMovies error:', error) |
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.
console.error 사용해 보시면 좋을 것 같습니다.
No description provided.