-
Notifications
You must be signed in to change notification settings - Fork 88
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단계 - 상세 정보 & UI/UX 개선하기] 우디(류정우) 미션 제출합니다. #100
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.
@evencoding 👋
코드가 너무나 많이 변경되어 깜짝 놀랐네요... 😭
그나마 친절하게도 미리 알려주셔서 감사합니다
피드백
App.js
너무나 많은 일을 한번에 몰아서 하고 있습니다
이건 꼭 개선이 하시는게 좋겠어요!
대부분의 로직을 App
에 방치해놓고 내려주는 형태이니... 확장성도 떨어지고 앞으로 코드가 더 좋아지기 어렵다는 생각이 드네요
비동기 통신
우선 fetch API
를 분리하신 점은 정말 훌륭합니다 👍
다만 예외 처리가 부족해요.
이 점을 꼭 고려하셔서 더 꼼꼼하게 하시며 에러 처리까지 연계하시면 더 견고하고 단단한 앱이 될겁니다 💪
이정도로 수정 방향을 드려볼게요~
다만.. 아직 미완성된 부분이 너무나 많아.. 정말 아쉽네요 😭
그럼 미완성된 기능과 리뷰 반영 후 다시 요청해주세요!
.DS_Store
Outdated
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.
더 신경쓰겠습니다🥲
<meta http-equiv="X-UA-Compatible" content="IE=edge" /> | ||
<meta name="viewport" content="width=device-width, initial-scale=1.0" /> | ||
<title>영화관</title> | ||
<script defer src="bundle.js"></script></head> |
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.
애플리케이션의 실행 혹은 배포를 위해 생성되는 dist폴더의 코드를 제가 임의로 수정해도 괜찮은가요??
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.
오호.. index.html
도 생성된 파일이군요?
tsconfig.json
Outdated
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.
OMG.. 😱
한번 정리하시는게 어때요
templates/common.css
Outdated
@media screen and (max-width: 672px) { | ||
.item-view h2 { | ||
width: 370px; | ||
} | ||
} | ||
|
||
/* https://andrew.hedges.name/experiments/aspect_ratio/ */ |
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.
값을 비율로 계산해 볼 수 있는 사이트네요
이 또한 원래 있던거라 크게 신경쓰지 않았던 것 같아요🥲
src/components/SkeletonCards.js
Outdated
return skeleton; | ||
} | ||
|
||
appear() { |
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.
hide
와 반대되는 의미로 바꾸는게 좋지 않을까요?
src/components/MovieView.js
Outdated
this.movieListTitle.changeInnerText('지금 인기 있는 영화'); | ||
} | ||
|
||
addMovies({ page, results: movies, total_pages }) { |
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.
서버에서 내려오는 snake_case
가 계속되어 아쉽네요
alt="${title}" | ||
/>` | ||
: `<div class="item-thumbnail no-image"> | ||
<span>No Image</span> |
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/MovieList.js
Outdated
const movieLi = movies.reduce((li, movie) => { | ||
return li + this.getMovieItemTemplate(movie); | ||
}, ''); | ||
|
||
return movieLi; |
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.
바로 반환해도 되지 않을까요?
import MovieView from './components/MovieView'; | ||
import MovieDetail from './components/MovieDetail'; | ||
|
||
const pageCounter = (firstPage) => { |
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.
class
의 기능을 온전히 활용하면 될텐데 Closure
라니 너무 뜬끔없군요..?!
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.
뭐 학습 목적으로 시도해보는건 좋습니다 ㅎㅎ
하지만 Class
를 사용한다면 이런 간단한 경우 전혀~~ 필요없는 기법일 수 있어요
src/App.js
Outdated
window.addEventListener('scroll', this.onScrollHandler.bind(this)); | ||
} | ||
|
||
onScrollHandler() { |
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.
여기서 스크롤 이벤트는 따로 분리하는게 좋겠어요 💭
step1부터 제가 꼼꼼하지 못했던 점에 대한 피드백을 여럿 받았네요🥲 step1에서 리펙토링을 하고 모두 되돌리고 하던 과정이 반복되어 조금 힘들었었어요 왜 혼날거라 생각했는지 모르겠네요🤣 첫 리뷰 요청 때 다 못했던 기능을 구현하고, 이 과정에서 앞으로 조금 더 고민하거나, 수행해야 할 내용은 아래와 같습니다
사실 위의 내용도 모두 끝낸 후 리뷰 요청을 하고 싶은데.. 잘 부탁드리겠습니다!! |
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.
@evencoding
언제나 내가 작성한 코드에 대한 아쉬움은 잘 지워지지 않죠..
저 또한 여러가지로 아쉬움이 많이 남고 더 많은 수정을 하셨으면 좋겠습니다만...
그래도 시간 투자하며 코드를 다시 작성하기 위한 훈련은 도전 자체로 의미있었습니다!
요구사항을 다 구현하지 못한건 저에게 죄송할 필요 없습니다!
다만.. 크론에게는 죄송해야.. 후후..
곧 방학일텐데 푹 쉬시며 재충전하시고 레벨2에서도 지금과 같은 강력한 실천력으로 성장하셨으면 좋겠네요.
이만 머지하겠습니다.
레벨1 기간 고생했어요!
src/App.js
Outdated
this.movieView.updateMovieListTitle(); | ||
document.dispatchEvent(new CustomEvent('updateMovieListTitle', { detail: { query: null } })); |
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/App.js
Outdated
$target.addEventListener('click', this.onClickHandler.bind(this)); | ||
$target.addEventListener('submit', this.onSubmitHandler.bind(this)); | ||
// $target.addEventListener('click', this.onClickHandler.bind(this)); | ||
// $target.addEventListener('submit', this.onSubmitHandler.bind(this)); |
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/Header.js
Outdated
|
||
return $input.value; | ||
const query = e.target[0].value; |
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.
[0]
보다 더 좋은 방법도 고려해보세요!
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.
아주아주 멋진 시도입니다
추상화 시도는 언제나 좋아요 👍
for (let i = 0; i < Number(score); i++) { | ||
$stars[i].src = star_filled; | ||
} | ||
[...$stars].forEach(($star, index) => { |
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.
굳이 [...$stars]
해줄 필요가 있을까요?
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.
오우 좋네용
@@ -0,0 +1,13 @@ | |||
const LOCAL_STORAGE_KEY = { | |||
APP: 'woowa_movie-review-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.
신개념 케이스군요 ㅋㅋ
|
||
return await this.getPopularMovies(page); | ||
} catch (e) { | ||
return e; |
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.
반환은 무슨 역할을 하나요?
@@ -114,11 +114,11 @@ class App { | |||
this.isLoading = false; | |||
} | |||
|
|||
displayErrorMessage({ status_code }) { | |||
displayErrorMessage({ status_code: statusCode }) { |
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의 요구 사항을 모두 구현하지 못했어요
step1에서 했던 코드는 잠시 주머니에 넣어두고 새로 미션을 시작했어요ㅠㅠ
step1의 코드는 리펙토링을 진행하려면 사실상 모든 것을 새로 만들어야 했고, 그럴거면 새로 시작하는게 낫겠다고 생각했습니다
이전의 코드는 전역에서 모든 상태를 관리했기 때문에 모든 컴포넌트들이 상태에 접근했기 때문에, 수정이 너무 어려웠어요
때문에 이번에는 상태를 최소화하고, 각 컴포넌트가 상태를 가지도록 했어요
또 컴포넌트 구조의 일관성을 높이고, 상하관계를 확실히 해주려고 노력했습니다
그러다보니 스켈레톤 코드를 hidden상태로 만들기위해 App -> MovieView -> MovieList -> SkeletonCards 과 같이 메서드 드릴링이 발생하긴 했지만 이전의 코드보다 훨씬 알기 쉬운 코드가 됐다고 생각합니다
이번에도 잘 부탁드리겠습니다!!