[2단계 - 상세 정보 & UI/UX 개선하기] 초코(강다빈) 미션 제출합니다. #171
Conversation
Co-Authored-By: MYONG JAEWI <[email protected]>
Co-Authored-By: MYONG JAEWI <[email protected]>
Co-Authored-By: MYONG JAEWI <[email protected]>
Co-Authored-By: MYONG JAEWI <[email protected]>
Co-Authored-By: MYONG JAEWI <[email protected]>
Co-Authored-By: MYONG JAEWI <[email protected]>
Co-Authored-By: MYONG JAEWI <[email protected]>
- mockingData 사용 - template을 컴포넌트화 Co-Authored-By: MYONG JAEWI <[email protected]>
Co-Authored-By: 00kang <[email protected]>
Co-Authored-By: 00kang <[email protected]>
Co-Authored-By: 00kang <[email protected]>
Co-Authored-By: 00kang <[email protected]>
Co-Authored-By: 00kang <[email protected]>
Co-Authored-By: 00kang <[email protected]>
mocking 데이터 제거 Co-Authored-By: 00kang <[email protected]>
Co-Authored-By: 00kang <[email protected]>
Co-Authored-By: 00kang <[email protected]>
Co-Authored-By: 00kang <[email protected]>
Co-Authored-By: 00kang <[email protected]>
검색 -> 더보기 버튼 클릭 -> 다시 검색 시, presentPage count가 유지되는 버그 수정
docs/REQUIREMENTS.md
Outdated
|
|
||
| ### 2. ⭐️ 별점 매기기 | ||
|
|
||
| - [ ] TMDB API 요청과는 관련 없습니다. |
There was a problem hiding this comment.
급하게 제출하느라고 놓쳤네요,,ㅎ 리뷰 재요청하기 전에 정리하도록 하겠습니다.
There was a problem hiding this comment.
배포한 사이트에 들어갔을 때 load 되지 않는 이미지들이 있는데 확인해보시면 좋을거 같네요~
src/App.ts
Outdated
| if (width <= 390) { | ||
| skeletonCount = SKELETON_UI_MOBILE; | ||
| } else if (width <= 834) { | ||
| skeletonCount = SKELETON_UI_TABLET; | ||
| } |
There was a problem hiding this comment.
반응형을 위해 이런 조건이 추가된거 같은데 매직 넘버 대신 상수를 사용해보시면 좋을거 같아요
There was a problem hiding this comment.
매직 넘버 대신 상수 사용하기!
매번 리뷰 코멘트에 달리는데 매 미션마다 하나씩 놓치는 제 자신이 부끄럽네요,,ㅎ
src/App.ts
Outdated
| #getSkeletonCount() { | ||
| const width = window.innerWidth; | ||
| let skeletonCount = this.#skeletonBySize; | ||
|
|
||
| if (width <= 390) { | ||
| skeletonCount = SKELETON_UI_MOBILE; | ||
| } else if (width <= 834) { | ||
| skeletonCount = SKELETON_UI_TABLET; | ||
| } | ||
|
|
||
| return skeletonCount; | ||
| } |
There was a problem hiding this comment.
제 local에서 코드를 실행했을 때 아래와 같은 에러가 나오는데 이 에러가 어디에서 왜 발생하는지 찾아서 고쳐보시면 좋겠네요 ㅎㅎ
| }, | ||
| }; | ||
|
|
||
| export async function fetchPopularMovies(pageCount: number) { |
There was a problem hiding this comment.
초코는 어떻게 관리하는게 좋다고 생각하시고 왜 그렇게 생각하시는지 먼저 들어보고 싶네요 ㅎㅎ
src/css/common.css
Outdated
| @@ -176,3 +180,50 @@ header .search-box > .search-button { | |||
| background: url('../images/search_button.png') transparent no-repeat 0 1px; | |||
| background-size: contain; | |||
| } | |||
|
|
|||
| @media only screen and (max-width: 834px) { | |||
There was a problem hiding this comment.
반응형 구현 쉽지 않으셨을텐데 고생 많으셨습니다 초코 ㅎㅎ 👍 👍 👍 💯
There was a problem hiding this comment.
조금 더 신경써서 유저의 화면에 항상 모든 영화 리스트가 한번에 보일 수 있게 해보셔도 좋을거 같습니다! (지금은 화면 크기에 따라 스크롤이 생기는 구간이 있네요)
There was a problem hiding this comment.
이것도 놓치고 있었군요.
UI/UX를 더 고려해서 리뷰요청 보내겠습니다!
src/css/modal.css
Outdated
| .my-vote-description { | ||
| } | ||
|
|
||
| @media only screen and (max-width: 390px) { |
There was a problem hiding this comment.
어떻게 구현하느냐에 따라 다르겠지만 웹 화면을 기준으로 먼저 구현을 했다면 화면 크기가 줄어듬에 따라 그에 맞는 CSS가 적용되게 구현하고, 모바일 화면을 기준으로 먼저 구현을 했다면 화면 크기가 늘어남에 따라 그에 맞는 CSS가 적용되도록 하도록 기존 스타일을 수정하는 방식으로 많이 진행되는거 같아요.
src/App.ts
Outdated
| }; | ||
|
|
||
| this.#observer = new IntersectionObserver(this.#handleIntersection, options); | ||
| const sentinel = document.createElement('li'); |
There was a problem hiding this comment.
여기에서 만든 sentinel(li tag)를 DOM에 넣어주는 부분이 없는게 원인이지 않을까 싶은데
There was a problem hiding this comment.
맞네요,,ㅎ intersection obersever API에 대한이해가 덜 된 상태에서 하려다보니 놓쳤던 것 같습니다.
| /* eslint-disable max-lines-per-function */ | ||
| #generateMoreButton() { | ||
| this.#removeMoreButton(); | ||
| #setupIntersectionObserver() { |
- 레이아웃 기준 재설정 - 헤더 넘침으로 인한 x축 스크롤 이벤트 해결
- overview가 없을 시 문구 삽입 - 모달이 오픈되었을 때 백그라운드 스크롤 금지 - 평점의 별 이미지와 텍스트 정렬 - 모달 내 별점 컨테이너의 요소 위치 고정
There was a problem hiding this comment.
안녕하세요 유조, 🍫초코🍫입니다
드디어 기능구현과 약간의 리팩토링을 마치고 재 리뷰요청을 드리네요.
몇 가지 질문 사항과 함께 배포 링크 공유드립니다!
🔗 배포 링크
✅ 현재 진행상황 공유
- 기능 요구사항 충족
- 디테일 모달 : 영화 데이터 보여주기
- 디테일 모달 : 별점 매기기
- 반응형 레이아웃
- 무한 스크롤
- UI/UX 개선사항
- 포스터 이미지, 검색 인풋박스, TOP 버튼 hover시 강조효과
- 화면 최상단으로 이동하는 TOP 버튼
- 모달 디테일 개선 : overview 없을 때, 정렬, 평점 영역 위치 고정, 모달 백그라운드에서 무한 스크롤 금지
🟩 목표하는 개선사항
- 이번주 금요일 오후 6시까지 머지를 받아야 하지만, 한 번 더 리팩토링할 의향 있습니다!! 좀 더 개선했으면 하는 부분들을 콕콕 짚어주세용
- 반응형 디자인 디테일 정교하게 작업
- 디테일한 에러 대응 작업
- 코드 메서드, 파일 구조 분리 작업
❓ 질문 사항
- 클래스로 코드를 작업하는 기준이 있으실까요?
오늘 한 크루에게 코드 관련 질문을 하다가 나온 이야기인데, 본인은 웹 환경에서는 캡슐화를 사용해야할 필요성을 못 느껴서 클래스를 사용하지 않는다고 하더라구요. 예를 들면 저의 경우에는 무비카드를 클래스로 만들었는데 어차피 웹에 계속 뿌려주는 요소인데 클래스를 사용해야 할까? 라는 질문이 돌아오더라고요. 저는 코드의 일관성?을 지키고자(물론 후반부엔 무너졌지만,,ㅠ) 클래스 형식을 사용하려고 했거든요!
- 유조는 클래스를 사용하고 하지 않는 기준이 있을까요?
- 한 프로젝트 내에서 어떤 코드는 클래스를 사용하고 어떤 코드는 객체로 사용하는 게 일관성을 해치지는 않을까요? 이에 대해 유조의 생각이 궁급합니다.
- 이번 미션의 포인트가 UI/UX 개선하기인데 저는 어떤걸 개선할 수 있을지 고민되더라고요. 평소에 좋은 UI/UX가 무엇인지 기록해 두고 추후에 참고하시나요?
| export const ERROR_2XX = '2'; // 200번대 오류 | ||
| export const ERROR_4XX = '4'; // 400번대 오류 | ||
| export const ERROR_5XX = '5'; // 500번대 오류 |
There was a problem hiding this comment.
status를 기반으로 오류를 처리할 때 얼마나 상세하게 처리해 줘야 할까요?
There was a problem hiding this comment.
이건 그 때 그 때 상황마다 달라지는데요. 지금과 같은 상황에서는 에러코드의 대역을 나눠 처리하는 것만으로도 충분할거 같아요.
지금은 백엔드 API가 이미 정해진 스펙으로 구현되어 있고, 그에 따라 수동적으로만 에러에 대한 정보를 받고 처리할 수 있는 상황인데요. 실제 백엔드 개발자들과 협업을 하면서 진행할 때는 에러에 대한 스펙도 같이 협의를 하기 때문에 어떤 방식으로 Error를 핸들링 할 지 같이 논의하는거 같아요.
보통은 401, 403, 500번대 에러 등 특정 에러 상황에 맞게 핸들링을 하고 서버로 전송 된 이후의 에러는 서버 쪽에서 내려주는 message에 의존하도록 구현하는거 같아요
| <a href="#"> | ||
| <div class="item-card"> | ||
| <img | ||
| class="item-thumbnail" | ||
| src="${posterPath}" | ||
| loading="lazy" | ||
| alt="${movie.title}" | ||
| /> | ||
| <p class="item-title">${movie.title}</p> | ||
| <p class="item-score">${movie.vote_average.toFixed(2)}<img src="./images/star_filled.png" alt="별점" class="star-start" /></p> | ||
| </div> | ||
| </a>`; | ||
| <div class="item-card", data-movieid="${this.#movieId}"> |
There was a problem hiding this comment.
모달을 오픈할 때 백그라운드의 스크롤이 최상단으로 이동하는 문제가 있었습니다.
모달을 생성할 때 <a href="#">를 삭제하니 해당 문제가 해결되었습니다.
저는 이 코드를 추후에 일어날 링크 이벤트(?)를 위해 #으로 처리하는 걸로 알고 있었는데 부수적인 이벤트가 또 있는 걸까요?
There was a problem hiding this comment.
href 속성은 어디로 이동할 지를 지시하는데요. #만 지정했을 때 브라우저가 해시(#)에 해당하는 값을 찾지 못한 상태에서 URL에 추가하여 페이지 최상단으로 이동하려는 기본 동작을 취하는 경우가 있어요.
There was a problem hiding this comment.
오 어림짐작하던 거였는데 알려주셔서 감사합니다!
src/css/common.css
Outdated
| } | ||
| } | ||
|
|
||
| @media only screen and (min-width: 390px) and (max-width: 834px) { |
There was a problem hiding this comment.
반응형 디자인이 너무 어려워요 ㅠㅠㅠ
저는 이런 식으로 특정 포인터를 잡아서 모바일, 테블릿, 노트북 환경으로 세팅을 해줬는데, 이 경우에는 반응형 레이아웃이 너무 휙휙 바뀌는 듯한 어색한 느낌이 들었습니다.
그래서 다른 크루에게 질문해보니 컨테이너 내부 요소들의 gap을 자동으로 두고, 컨테이너 좌우에 padding 등의 여백을 두고, 이 너비가 화면 너비와 맞닿을 때 레이아웃이 바뀌도록 했더라구요.
어떤 방식이 더 좋은 것일지 감이 안 오는데 현업에서는 어떤 방식을 사용하게 되나요??
There was a problem hiding this comment.
보통은 특정 break point를 가지게 한 다음 구현하는 게 일반적인거 같아요.
현업에서는 적응형 디자인도 많이 사용하는데 적응형과 반응형의 차이에 대해서도 한번 찾아보시면 좋겠네요 ㅎㅎ
src/css/modal.css
Outdated
| left: 278px; | ||
| } | ||
|
|
||
| @media only screen and (max-width: 390px) { |
There was a problem hiding this comment.
현재 834px부터 모바일 사이즈로 되어 있다보니, 모바일 버전 모달을 띄울 때 애매해져서 위와 같은 질문을 하기도 했습니다.! 반응형 디자인의 포인터/경계값을 조절해야 하는걸까요?
There was a problem hiding this comment.
반응형 디자인의 포인터/경계값을 조절해야 하는걸까요?
정확히 어떤 상황에서의 얘기일까요 질문을 잘 이해하지 못 했습니다 😅
There was a problem hiding this comment.
애매하게 말을 전한 것 같은데, 위에서 얘기한 반응형 디자인의 break point를 얘기한 거였어요. 🤣 무비카드 그리드와 모달, 헤더 3가지가 같이 변해야 되서 앞선 질문과 동일한 맥락이었어요
There was a problem hiding this comment.
경계값을 조절해도 될거 같고 사이즈 별로 Modal 같은 요소가 다르게 출력되게 해도 좋을거 같네요 ㅎㅎ
| import StarFilled from '../images/star_filled.png'; | ||
| import StarEmpty from '../images/star_empty.png'; | ||
|
|
||
| export default class Modal { |
There was a problem hiding this comment.
현재 Modal 클래스의 역할(모달 생성, 오픈, 클로즈, 영화 데이터 넣어주기, vote 영역 관리) 이 너무 방대한 것 같습니다. 이를 리팩토링한다면 모달 오픈, 클로즈 / 영화 데이터 넣어주기 + vote 관리 로 생각 중인데,
vote 관련 기능을 따로 분리해야 되는지도 궁금합니다.
+) 유조는 이렇게 코드를 분리할 때의 tip이나 기준이랄 게 있을까요?
There was a problem hiding this comment.
테스트를 기준으로 분리를 많이 하는거 같아요.
컴포넌트 단위의 테스트를 진행할 때 Testable 한 지를 기준으로 삼아보셔도 좋을거 같아요
There was a problem hiding this comment.
조금 더 자세히 적자면 지금 Modal이 하는 역할이 많기 때문에 Modal에 대한 테스트를 진행하기 위해서는 굉장히 많은 코드가 작성되어야 할텐데요. 말씀해주신 내용처럼 vote 영역을 따로 분리한다면 테스트를 작성할 때 vote 영역에서는 vote에 대한 동작만 테스트를 진행하면 되겠죠. 이런 식으로 컴포넌트 내에서도 여러 컴포넌트로 나뉠 수 있고 그에 따른 역할과 책임이 있을텐데 이 부분을 고려해서 분리해보시면 좋을거 같아요 ㅎㅎ
There was a problem hiding this comment.
오😯 테스트를 고려해서 나누어 본다. 그동안에는 관심사를 기반으로 분리하다고 했을 때 확 와닿지 않는 느낌이었는데 좋네요! 감사합니다.
There was a problem hiding this comment.
안녕하세요 초코 ~
피드백 반영하고 개선하시느라 고생 많으셨습니다 ㅎㅎ
추가 코멘트와 더불어 초코가 남겨준 질문들에도 답변 남겼으니 같이 확인 부탁드려요~
클래스로 코드를 작업하는 기준이 있으실까요?
class가 필요하면 class를 사용하고 그렇지 않으면 function을 사용합니다 ㅎㅎ
보통은 캡슐화가 필요하거나 확장성이 필요한 경우 class를 선택하게 되는거 같아요.
한 프로젝트 내에서 어떤 코드는 클래스를 사용하고 어떤 코드는 객체로 사용하는 게 일관성을 해치지는 않을까요? 이에 대해 유조의 생각이 궁급합니다.
어떻게 사용하느냐에 따라 다를거 같아요. 같은 역할을 수행하는 두 모듈이 있을 때 한 모듈은 클래스를 사용하고 한 모듈은 객체를 사용한다면 코드를 볼 때 일관성을 해치고 혼란스러울 수 있겠지만 다른 역할을 하는 모듈들에서 class를 사용하던 객체를 사용하던 큰 상관은 없을거 같네요
이번 미션의 포인트가 UI/UX 개선하기인데 저는 어떤걸 개선할 수 있을지 고민되더라고요. 평소에 좋은 UI/UX가 무엇인지 기록해 두고 추후에 참고하시나요?
기록까지는 아니지만 평소에 꾸준히 관심을 가지고 보는 영역이에요. 직접 만든 애플리케이션은 그만큼 애정이 가기 마련이지만 제작자의 시선이 아닌 사용자의 시선으로 애플리케이션을 살펴보며 불편한 부분들을 찾아서 주로 개선하는거 같습니다.
merge해도 충분한 수준으로 보이지만 초코가 목표하는 개선사항들이 있어 해당 내용까지 같이 보면 좋을거 같아 request changes를 드렸는데요. merge 원하시면 말씀해주세요 ㅎㅎ
적어주신 내용 외에 개선할만한 부분을 적어보면
- item을 클릭하고 modal이 나오기까지 loading 상태를 유저가 인지할 수 있으면 좋을거 같아요
- clickable한 요소들은 cursor 모양을 바꿔 유저가 쉽게 인지할 수 있게 해주시면 좋을거 같아요
- 현재 별점 매길 때 이미지가 제대로 보이지 않는데 이 부분도 고쳐보시면 좋을거 같아요
고생 많으셨습니다 초코~~ 👍 👍 👍
src/constants.ts
Outdated
| export const MOBILE_SIZE = 390; // mobile 사이즈 기준 | ||
| export const TABLET_SIZE = 834; // tablet 사이즈 기준 |
src/App.ts
Outdated
| #getSkeletonCount() { | ||
| const width = window.innerWidth; | ||
| let skeletonCount = this.#skeletonBySize; | ||
|
|
||
| if (width <= MOBILE_SIZE) { | ||
| skeletonCount = SKELETON_UI_MOBILE; | ||
| } else if (width <= TABLET_SIZE) { | ||
| skeletonCount = SKELETON_UI_TABLET; | ||
| return SKELETON_UI_MOBILE; | ||
| } | ||
|
|
||
| return skeletonCount; | ||
| if (width <= TABLET_SIZE) { | ||
| return SKELETON_UI_TABLET; | ||
| } | ||
| return this.#skeletonBySize; | ||
| } |
| import { ERROR_2XX } from '../constants'; | ||
|
|
||
| import ErrorRender from '../components/ErrorRender'; | ||
|
|
||
| class MovieStore { | ||
| #moviesData: any[]; | ||
| #moviesData: Movie[]; |
src/App.ts
Outdated
| #addSentinel() { | ||
| const ulElement = document.querySelector('ul.item-list'); | ||
|
|
||
| if (ulElement) { | ||
| const sentinel = document.createElement('li'); | ||
| sentinel.classList.add('sentinel'); | ||
| ulElement.appendChild(sentinel); | ||
| this.#observeSentinel(); | ||
| } | ||
| } |
| if (event.key === 'Escape') { | ||
| this.closeModal(); | ||
| } |
| @@ -114,6 +114,8 @@ export default class Modal { | |||
| this.#updateVoteText(voteForMovie); | |||
| } | |||
|
|
|||
| document.addEventListener('keydown', this.#handleKeyDown); | |||
There was a problem hiding this comment.
웹 페이지에서 키보드 이벤트를 처리하기 위해 사용되는 이벤트
keydown이벤트- 키를 누를 때 발생
- 키를 길게 누르면 이벤트는 계속해서 발생
keyup이벤트- 키를 누른 후 놓을 때 발생
- 한 번 키를 누르고 놓을 때말 발생하므로, 이벤트는 한번만 발생
| @@ -20,6 +20,7 @@ <h1> | |||
| <h2></h2> | |||
| <ul class="item-list"></ul> | |||
| </section> | |||
| <button id="goToTop" class="text-detail-contents">TOP</button> | |||
|
안녕하세요 유조, 🍫초코🍫입니다 🔗 배포 링크✅ 리팩토링 상황 공유
|
There was a problem hiding this comment.
이미지 닫는 태그가 소스에 딱 붙어있어서 그랬더라구요
src/components/Modal.ts
Outdated
| if (!starButtons) return; | ||
|
|
||
| starButtons.forEach((starButton, index) => { | ||
| starButton.setAttribute('src', index < voteForMovie / 2 ? StarFilled : StarEmpty); | ||
| }); |
There was a problem hiding this comment.
MovieInfo나 VoteHandler가 특정 영역(Modal)에 한정되어 사용되기 때문에 적절한 폴더링을 통해 사용되는 범위를 드러내봐도 좋을거 같아요
ex)
- components
- Modal
- Modal.tsx
- components
- MovieInfo
- VoteHandler
There was a problem hiding this comment.
계속 고민 되는 부분이었는데! 나중엔 나눠봐야겠어용 감사합니다
src/css/modal.css
Outdated
| left: 278px; | ||
| } | ||
|
|
||
| @media only screen and (max-width: 390px) { |
There was a problem hiding this comment.
경계값을 조절해도 될거 같고 사이즈 별로 Modal 같은 요소가 다르게 출력되게 해도 좋을거 같네요 ㅎㅎ
안녕하세요 유조, 🍫초코🍫입니다.
2단계 미션 상세 정보 & UI/UX 개선하기 미션 제출합니다!
현재 진행 상황이 미흡해서 리뷰 피드백과 동시에 추가적으로 기능 구현하도록 하겠습니다!
질문 사항은 코멘트로 남기겠습니다~~
🏃♀️ 실행 방법
🔗 배포 링크
바로가기
☑️ 현재 진행 상황