[2단계 - 영화 목록 불러오기] 해온(백솔비) 미션 제출합니다. #60
Conversation
There was a problem hiding this comment.
빠르게 2단계 미션을 구현해주셨네요! 고생 많으셨습니다👏👏👏
무한 스크롤도 매끄럽게 되면서 모달 영역도 잘 동작하네요!
아쉬운 UX 몇 개 남겨볼게요.
- 별점주기 모달이 뜬 상태로 백그라운드 스크롤이 가능한 현상
- 영화 검색 시 공백 입력 가능
또 코드 전체적으로 타입 단언이 너무 많이 사용되고 있습니다. 리뷰에도 남겼지만 해온은 타입 단언과 옵셔널 체이닝을 억제하는 코드를 많이 작성해보면 좋겠어요.
궁금한 점에 대해 이야기해볼까요?
반응형 미디어 쿼리
- 기업별로 상이하나, 크게 모바일/태블릿/데스크탑 세 가지로 분류합니다. PM/PD들이 규정한 디바이스나 종속적인 상황이 필요한 경우 직접 viewport를 설정하기도 해요.
- 많은 디바이스를 충족시켜야하는 특정 앱(시각 자료가 많은)에서는 정말 세부적으로 쪼갭니다. 가짓수는 5개, 6개, 많게는 그 이상도 있습니다.
modal test 시 중복되는 내용에 대한 처리
- cypress를 사용한다면 custom command를 활용하면 좋을 것 같네요!
- 그렇지 않다면 beforeEach를 비동기 콜백을 주입하여 사용할 수도 있어요. jest 참고
- 방법은 여러가지입니다! 추후 컴포넌트 레벨로 테스트한다면 이러한 렌더링 테스트도 묶어서 진행할 수도 있구요.
별도로 재요청은 드리지 않고 머지해도 되는 컨디션이라 판단되네요!
앞서 말한 타입 단언과 옵셔널 체이닝 억제, 코드 레벨에서의 리팩토링만 남은 기간 잘 진행해보면 좋겠습니다🤗
혹, 타입스크립트에 대해 리팩토링 진행 시 어려운게 생기면 코멘트 남겨주세요!
| export type Genre = { | ||
| id: number; | ||
| name: string; | ||
| }; | ||
|
|
||
| export interface Modal { | ||
| id: number; | ||
| index: number; |
There was a problem hiding this comment.
해온은 type과 interface의 사용을 어떻게 구분하고 있나요?
There was a problem hiding this comment.
사실 아직까지 interface와 type을 어떤 상황에서 써야하는지 명확하게 구분하지 못하겠습니다..! 아마 여기도 ‘타입’을 지정해야해! 생각때문에 무의식적으로 type을 사용한 거 같아요! 구글링해보니 ‘확장이 불가능한 타입을 선언하려면 type-alias를 사용하면 되고, 확장이 가능한 타입을 선언하려면 선언 병합이 가능한 interface를 사용하면 된다.’고 하는데 파랑은 명확한 구분 기준이 있으신가요? 궁금합니다!
| export const getSavedData = (key: string) => | ||
| JSON.parse(<string>localStorage.getItem(key)) || []; |
There was a problem hiding this comment.
JSON.parse 시 parse 불가능한 문자열이라면 에러가 발생합니다. 이에 대한 처리를 추가하면 어떨까요?
There was a problem hiding this comment.
네! 좋습니다. Try catch문을 사용해 parse가 불가능한 문자열일 경우 error로 잡아낼 수 있도록 고쳐보았습니다.
| export const genre = async () => { | ||
| const url = `${FetchUrl.GENRE_URL}`; | ||
| return await fetchData(url); | ||
| }; |
There was a problem hiding this comment.
getGenre 등의 네이밍은 어떨까요? type Genre로 선언이 되어 있기 때문에 "데이터를 가져오는" 행위에 대해 동사를 적용하면 좋을 것 같아요.
There was a problem hiding this comment.
맞아요…! 장르 데이터를 가져오는거니까 getGenre로 쓰는게 좋겠어요! 안그래도 type이랑 이름이 같아서 쓰면서도 헷갈렸는데 너무 좋네요!!
| getSelectedMovie(id: number): Movie { | ||
| return <Movie>this.movies.find((movie) => movie.id === Number(id)); | ||
| }, |
There was a problem hiding this comment.
id가 number로 이미 선언되어 있는데 Number로 감싸줬네요! 없어도 좋을 것 같습니다🤗
interface로 메서드의 타입을 선언해두셨다면, 메서드 선언 부에서 동일하게 타입을 적용하지 않아도 추론이 된답니다.
There was a problem hiding this comment.
밖에서 id를 받아올 때 string 형태로 받아왔는데, 그걸 밖에서 number로 변환 안했더라구요..! 그래서 movieHandler에서 Number를 해줘야 작동하는거였어요…! (어쩐지 Number를 안하면 계속 오류가 나더라구요…😂) movieHandler의 Number는 삭제해주었습니다!
| <div class="vote-stars"> | ||
| <img class="star-icon" data-order="1" src="${StarEmpty}" alt="start" /> | ||
| <img class="star-icon" data-order="2" src="${StarEmpty}" alt="start" /> | ||
| <img class="star-icon" data-order="3" src="${StarEmpty}" alt="start" /> | ||
| <img class="star-icon" data-order="4" src="${StarEmpty}" alt="start" /> | ||
| <img class="star-icon" data-order="5" src="${StarEmpty}" alt="start" /> | ||
| </div> |
| showMessage(order: number) { | ||
| let message = ""; | ||
|
|
||
| switch (order) { | ||
| case 1: | ||
| message = "최악이예요"; | ||
| break; | ||
| case 2: | ||
| message = "별로예요"; | ||
| break; | ||
| case 3: | ||
| message = "보통이에요"; | ||
| break; | ||
| case 4: | ||
| message = "재미있어요"; | ||
| break; | ||
| case 5: | ||
| message = "명작이에요"; | ||
| break; | ||
| default: | ||
| message = "별점을 눌러주세요"; | ||
| break; | ||
| } | ||
| return message; | ||
| } |
There was a problem hiding this comment.
message 변수 없이 switch-case 안에 return을 명시하면 더 깔끔해질 것 같아요!
There was a problem hiding this comment.
message 변수 없이 바로 return 하니까 훨씬 간결해지고 좋네요!
| renderScoreAndMessage(order: number) { | ||
| const $voteScore = $(".vote-score"); | ||
| const $voteMessage = $(".vote-message"); | ||
| const message = this.showMessage(order); | ||
|
|
||
| if ($voteScore) $voteScore.textContent = String(order * 2); | ||
| if ($voteMessage) $voteMessage.textContent = message; | ||
|
|
||
| this.renderStar(order); | ||
| } |
There was a problem hiding this comment.
이 메서드를 보면... 아래의 문제를 해결해야 될 것 같아요.
- 객체가
null | undefined일 때 타입 가드 혹은 조건문 등을 활용하여 옵셔널 체이닝과 단언 줄이기 - 변수화를 해야 하는 시점
String(order * 2)과this.showMessage(order)의 차이
또 showMessage 라는 네이밍보다는 getScoreMessage 등이 어떨까요?
There was a problem hiding this comment.
dom에서 선택자를 가져올 때 조건문을 이용해보았어요! 사용할 때 따로 null 체크를 하지 않아도 되도록 변경해보았습니다!
| const id = Number(this.getAttribute("modal-id")); | ||
| const order = getSavedData("modalData")[id]; |
There was a problem hiding this comment.
const id = Number(this.getAttribute("modal-id"));
이 코드는 class 내부의 getter로 만들면 좋을 것 같아요😊
There was a problem hiding this comment.
Vote 안에서 2번이나 반복되고 있었네요! gettter를 사용했습니다!
|
파랑! 말씀하신 부분은 모두 수정해서 커밋했습니다! |
|
@hae-on 괜찮습니다~!~! |
|
아앗 그렇군요... 그러면 로컬에서 push만 하고 마무리해도 될까요?? 🙄 |
|
@hae-on 해당 저장소에 해온의 브런치가 최신화되지 않는 차이점만 있으니, 수정된 내역을 남기고 싶다면 PR을, 아니라면 로컬 push만으로 작업해도 괜찮답니다🤗 |
배포 사이트
안녕하세요 파랑!
2단계 완료 후 돌아왔습니다~~!
여전히 저번 this.bind 문제는 여전히 해결을 하지 못했습니다 ㅠㅠㅠ
여러 방법을 시도해봤는데 아직까지 화살표함수나 bind 하는 방법빼고는 잘 모르겠네요...!
querySelector에서 this를 넣어줬던 건 빼보았습니다!
2단계 진행하다가 또 궁금한 점이 생겨서 밑에 작성해두겠습니다.
리뷰 항상 감사드려요~~!! 주말 잘 보내세요!! 🥳
궁금한 점
반응형을 작성하면서 미디어쿼리로 화면 크기를 나누어보았는데요! 보통 태블릿 사이즈, 모바일 사이즈 이렇게 나누는 것으로 알고 있습니다. 근데 이것도 절대적인 것인 아니고 편의상 나누는 것이 맞을까요? 중간중간마다 더 상세하게 나눠주고 싶은데, 실무에서는 어느정도로 상세히 나누는 편인지 궁금합니다!
modal test를 진행하는데 각 테스트마다
인기영화 받아오기 -> 영화 클릭하기 -> 모달 열기동작이 반복되고 있습니다. 중복되는 부분이 많아서 이를 beforeEach나 함수에 넣어서 사용해보려고 했는데요! 근데 모든 테스트마다 다 에러가 터지더라구요...🤯 이렇게 중복되는 부분을 없애는 것이 잘못된 방법인지 궁금합니다!