[2단계 - 나만의 유튜브 강의실] 병민(윤병인) 미션 제출합니다. #146
Conversation
1. 가로 세로 길이를 padding이 아닌 width, height로 지정했다 2. 모달 안의 저장, 검색 버튼도 디자인을 적용했다
요구사항을 잘못 이해했다. 탭 형식이 아닌 필터링 형식이었다.
1. 반응형 css코드는 responsive.css파일 한곳에서 전부 관리한다 2. 반응형 코드를 전반적으로 수정한다
min으로만 작성하면 기본 css값이 미디어 쿼리에 작성한 css보다 높은 우선순위를 갖게된다. 이를 피하려면 min 1280px 미디어 쿼리에 기본값을 적어줘야 하는데, 이렇게 하면 너무 코드가 한쪽으로 쏠리게 된다. 그래서 기본값을 먼저 주고, 특정 구간에서만 css가 먹히도록 미디어 쿼리를 수정했다.
양이 얼마 없기 때문에 한곳에 모아놔도 괜찮다고 생각했다
스크롤을 너무 빠르게 내리면 영상이 너무 많이 불러들어와질 수 있기 때문이다
|
|
There was a problem hiding this comment.
병민님 안녕하세요, 2단계 수행하느라 고생하셨습니다.
몇가지 확인해주셔야 할 내용이 있습니다.
- 테스트 돌려보니 동작이 되기는 하는데, 뭔가 좀 이상한 요청들이 중간중간 끼어있어요. q(검색어) 없이도 검색요청을 수행하고 있는 것으로 보입니다.
- 검색요청 도중에 모달창을 닫으면 이런 오류가 발생합니다. 요청의 cancel이 필요해 보여요.
- 아직 검색결과의 끝에 도달했을 때에 처음부터 무한반복으로 요청하는 현상은 그대로네요. 응답 객체의 pageInfo의 totalResults를 참고하세요.
- 그밖에는 코멘트 남겼으니 확인해보시고, 가능한 부분부터 우선적으로 수정해주세요. 어렵거나 이상한 부분은 코멘트 남겨주시고요 :)
| const videoSet = storage.load({}); | ||
| const ids = videos.items.map(({ id }) => id.videoId); | ||
| const isSame = ids.every(id => hasProperty(videoSet, id)); | ||
| expect(isSame).to.be.true; |
There was a problem hiding this comment.
👍
그런데 애매하긴 한데, storage에서도 똑같이 ids를 추출해서 deepEqual을 비교했다면 어땠을까 싶네요.
| }); | ||
|
|
||
| it('검색 버튼을 눌렀을때 검색 모달이 뜬다', () => { | ||
| cy.get(testid`open-search-modal-button`).click(); |
src/js/app.js
Outdated
|
|
||
| export default class App { | ||
| constructor() { | ||
| this.storage = new VideoStorage(LOCAL_STORAGE_VIDEO_LIST_KEY, MAX_SAVABLE_VIDEOS_COUNT); |
There was a problem hiding this comment.
VideoStorage 클래스에 두 인수를 주입해주기로 결정한 이유는 뭘까요?
변경가능성 때문에 변수(상수)로 추출한 것은 이해합니다만, 두 값은 어찌되었든 전체 서비스 내에서 공통적으로 쓰이는 값이 아닌가요? 굳이 인수로 전달하지 않더라도 VideoStorage에서 직접 해당 상수를 가져다 쓰면 되는것 아닐까요?
나중에 다른 값을 전달할 가능성을 염두에 두고 이렇게 작업한 거라면, 결과적으로 오버엔지니어링입니다.
There was a problem hiding this comment.
네, 맞습니다. 현재 VideoStorage는 한곳에서만 쓰이고 있고, 나중에 값이 바뀌더라도 저 상수안에 있는 값을 바꾸면 될것같습니다.
오버엔지니어링 조심하겠습니다!
|
|
||
| const searchModal = new SearchModal(); | ||
| const searchModal = new SearchModal(this.storage, this); |
There was a problem hiding this comment.
this만 전달했어도 되지 않나 싶어요. App 내에 storage에 대한 접근 메소드를 마련해두고, SearchModal은 그 메소드만 사용하도록 해야 조금이나마 결합도를 줄일 수 있을 것 같습니다.
There was a problem hiding this comment.
리뷰어님 혹시 this만 넘겨주고, storage에 대한 접근 메소드가 아닌 바로 SearchModal안에서 this.delegate.storage 이런식으로 접근해서 사용하는건 어떤가요? 접근 메서드를 따로 만드는 이유가 있을까요?
There was a problem hiding this comment.
말씀드린 '결합도'를 낮추기 위함입니다. 지금은 서로가 서로에게 영향을 주는 구조라 독립적으로 개별 기능을 수행하기가 매우 어렵고, 어느 하나의 변경에 의해 다른 하나도 영향을 받게끔 되어 있어요. 이를 조금이라도 줄이는 것이 좋습니다.
더 자세히 설명드리기엔 객체지향적 이해가 많이 요구되어서 이정도로만...
There was a problem hiding this comment.
네, 느낌은 이해했습니다. 객체지향도 공부 해보겠습니다.
혹시 이런식으로 하는게 맞을까요?
class App {
constructor() {
this.storage = new VideoStorage();
this.searchModal = new searchModal(this);
}
...
getStorage() {
return this.storage;
}
}
class SearchModal {
constructor(delegate) {
this.delegate = delegate;
}
doSomething() {
const storage = this.delegate.getStorage();
...
}
}
There was a problem hiding this comment.
네 현재 구조에서 큰 변화 없이 App과 SearchModal의 결합도를 더 낮추기는 어려울것 같고 이정도가 최선일것 같아요.
src/js/app.js
Outdated
| this.$modalContainer.classList.add('hide'); | ||
| }; | ||
|
|
||
| openModal = () => { | ||
| this.$modalContainer.classList.toggle('hide'); | ||
| }; | ||
|
|
||
| handleFilterClick = ({ target }) => { | ||
| if (target.tagName.toLowerCase() !== 'button') return; |
There was a problem hiding this comment.
|
|
||
| const videoList = this.getVideoListFromLocalStorage(); | ||
| this.savedVideoMap = arrayToMap(videoList); | ||
| this.observer = this.loadMoreObserver(); | ||
| } | ||
|
|
||
| templateSkeletons(videoCount) { |
There was a problem hiding this comment.
지금 구현한 방식은 다음과 같습니다.
- 사용자가 검색하면 비디오가 그려진다
- 그리고 다시 검색을 하면 video-list안에 있는 모든 요소가 사라진다
이렇게 한 이유는, video-list안에 video-item만 골라서 지우는(skeleton은 냅두고) 것보다 자식들 전체를 싹 지우고 skeleton을 다시 그려주는게 비용이 덜 들어갈것 같았기 때문입니다.
이에 대한 리뷰어님의 생각이 궁금합니다!
There was a problem hiding this comment.
skeleton이 꼭 현재의 ul list 안에 있어야만 할까요?
별도의 ul로 하단에 빼놓으면 아예 삭제하고 새로 그리는 작업 자체가 불필요해지고 오직 클래스만으로 컨트롤 가능할것 같은데요
| const lastVideoItem = $('.video-item.skeleton', this.$videoList).previousSibling; | ||
| setTimeout(() => { | ||
| this.observer.observe(lastVideoItem); | ||
| }, 800); |
There was a problem hiding this comment.
스크롤을 쎄게 내리면 연속적으로 api통신이 이루어 져서 API할당량도 너무 빨리 닳고, 사용자 입장에서도 스크롤을 했을때 약간의 브레이크를 줌으로써 '다음 페이지로 넘어간다'는 느낌이 들게끔 하고싶었습니다. 리뷰어님은 어떻게 생각하시나요?
There was a problem hiding this comment.
'약간의 브레이크'라는게 과연 충분한지를 확신할 수 있는지가 관건이에요. 데이터 응답 시간은 그때그때 다를테고, 그에따라 화면에 렌더링이 이뤄지는 시간도 그때그때 다를건데, 예를 들어 데이터 응답이 1초가 걸리는 상황이라면, 0.8초만에 다시 스크롤요청을 하게 되면 연속해서 2개의 페이지를 받게 되겠죠. 브레이크를 건다는 아이디어 자체는 훌륭합니다만, 서버 및 네트워크 환경에 따라 달라질 수 있는 '시간'을 임의로 0.8초로 지정한 것이 문제라는 거예요. 그보다는 응답이 올 때까지 홀딩한다거나, 다음 리스트가 화면에 렌더링될때까지 기다린다는 식이 되어야 하지 않을까요?
| @@ -154,42 +110,37 @@ class SearchModal { | |||
| [...this.$videoList.querySelectorAll('.skeleton')].forEach($el => $el.remove()); | |||
There was a problem hiding this comment.
그냥 화면상에서만 숨겨도 되지 않나요? 같은 요소를 매번 삭제했다가 다시 그려줬다가 할 필요는 없지 않을까요...
There was a problem hiding this comment.
아네, 맞습니다. 사용하지 않는 함수였는데, 남아있었네요. 삭제하겠습니다 :D
| async entries => { | ||
| if (entries[0].isIntersecting && this.nextPageToken !== null) { | ||
| this.observer.unobserve(entries[0].target); | ||
| const title = this.$searchKeyWordInput.value; | ||
| const videos = await this.requestVideos(title); | ||
| videos && this.renderVideoItems(videos); | ||
| } | ||
| }, |
There was a problem hiding this comment.
request 및 기타 처리를 하는 함수를 별도로 만들어두고,
여기서는 조건 성립시 해당 함수를 호출해주는 역할만 남기면 좋을것 같아요.
There was a problem hiding this comment.
잘 이해가 가지 않습니다. 혹시,
const title = this.$searchKeyWordInput.value;
const videos = await this.requestVideos(title);
videos && this.renderVideoItems(videos);
이 부분을 따로 함수로 분리하는게 좋다는 의미인가요?
There was a problem hiding this comment.
네 맞습니다. 그렇게 하면 intersecting에 의하지 않은 요청 (예를 들어 검색버튼을 눌렀을 때 처음 수행하는 요청) 등에서도 재활용할 수 있겠죠.
src/js/storage/videoStorage.js
Outdated
| toggleWatchStatus(videoId) { | ||
| const videoSet = this.load({}); | ||
| videoSet[videoId] = { watched: !videoSet[videoId].watched }; | ||
| this.save(videoSet); | ||
| this.cache = videoSet; |
There was a problem hiding this comment.
cache를 남기는 목적은 cache를 활용하기 위함이 아닌가요?
그렇다면 한 번 load한 이후에는 cache만 활용해도 충분한 것 아닌가요?
매번 this.load()를 수행할 이유가 없어 보여요.
There was a problem hiding this comment.
네 맞습니다. 제가 cache와 localStorage간의 싱크에 대해서 약간 겁을 먹어서 그렇게 작성한것 같습니다. 지금 다시 생각해보니 cache를 계속 활용해도 문제 없어 보입니다. 감사합니다 :D
…3. 체널명을 링크로 만든다.|4. 날짜 타입을 semactic하게 변경한다
VideoStorage가 한곳에서만 쓰이고 있고, 값이 바뀐다면 상수값을 바꾸면 되기 때문에 이전의 구현은 오버엔지니어링이다
localName을 사용하면 소문자로 변경해 줄 필요가 없다
app에는 버튼을 클릭했을때만 keyup이벤트를 받는다. 이유는 아직 모르겠다.
for은 label, output에 붙는 고유 속성이기 때문에 다른 tag에서 사용하면 나중에 문제가 될 수 있다
코드가 더 간결해진다
Google Data API Guid에서는 query에 대한 추가적인 결과값이 있는 경우에만 nextPageToken을 돌려준다고 합니다. 그래서 nextPageToken의 존재 여부로 검색결과의 끝을 알 수 있을것 같습니다. 확인 바랍니다 :D |
지금 상황파악을 완전히 잘못하고 계신 것 같은데, 스텝1때부터 이미 nextPageToken으로 검색결과의 끝을 판단하고 계시지 않았나요? 그렇게 했더니 nextPageToken이 비어있을 때(끝지점에 도달했을때) 다시 스크롤 내려도 더이상의 요청 없이 멈추어 있던가요? 제가 테스트하기로는 다시 첫페이지 데이터를 요청하고 그 응답결과가 맨 뒤에 붙고, 이어서 다시 무한스크롤이 되고 있는데요... 솔직히 제가 이 얘기를 몇번을 반복해서 설명해드려야 하는지 모르겠고 답답합니다. 테스트 한 번만 해봐도, 네트워크탭 한 번만 열어봤어도 이런 말씀은 안하셨으리라 생각해요. 문제상황을 해결하시라고 말씀드린 겁니다. 2.mov. |
result.nextPageToken이 null이 아니라 undefined였다. 그런데 나는 null이라고 생각해서 비교를 잘못 했었다.
|
저는 검색 결과가 예를들어 한 24개만 나오는 그런 검색어를 못찾아서 제대로 테스트를 못하고 제 코드 상에서만 정상 작동 여부를 예측하고 수정했었습니다. 미리 이런 부분에 대해 소통을 하지 않고 제출한점, 제 불찰입니다. 영상에서 "닭백숙꼬끼" 로 검색을 하니 검색결과가 몇개 안나오는걸 확인 했고, 문제 상황도 확인 했습니다. 감사합니다! 수정된 부분 확인 부탁드립니다 🙏 |
결과가 없을때 null이 아니라 items가 빈 배열인 객체가 돌아온다.
There was a problem hiding this comment.
병민님 수고하셨습니다.
피드백 반영하신것 모두 확인했고 전체적으로 읽기 좋아졌네요. 다만 dataset을 활용하는 것이 늘 베스트는 아니고, class로 충분한 경우가 더 많은데, 다음 미션에서는 dataset 대신 class로 처리할 수 있는 것들은 그렇게 시도해보시면 좋겠습니다.
제가 다소 까칠하게 말씀드린것 같아 저도 반성하고 있습니다. 모쪼록 맘 상하지 않으셨길 바라며 다음에는 좀더 성숙한 리뷰어가 되도록 해볼게요.
고생하셨고, 승인 및 머지하겠습니다!
안녕하세요, 로이님 :D
PR을 늦게 드려 죄송하다는 말 먼저 드립니다 🙏.
API 할당량이 다 찬것 같아서, 데모 페이지 확인이 어려우실수도 있을것 같습니다. 가능한 빨리 구해서 해결하겠습니다!일전에 말씀하셨던 "더 이상 불러올 동영상이 없는 경우에는 로드하지 않는다" 부분은 로직 상으로는 고친것 같은데, 아직 제대로 테스트 해보지는 못했습니다. API 할당량 문제가 해결되는 대로 다시 확인해보겠습니다.혹시 API할당량이 다 차서 테스트가 어려우시면, DM부탁드립니다! 바로 API교체하겠습니다.
데모페이지
2단계 미션 요구사항
내 영상 목록 뷰
반응형 디자인
개선한 부분
질문
저는 localStorage에 videoId만 담았습니다. 메인 페이지에 보여지는 동영상 리스트는 이 videoId를 가지고 Youtube Data API를 호출한 결과입니다. 처음에는 그냥 videoId외의 다양한 정보들도 담아서, api호출 하지 않고 바로바로 화면에 그려줄까 생각도 했었습니다. 그러나, 해당 유투브 영상이 삭제되거나, 수정될 수 있다고 생각해서 유투브 서버의 실제 데이터와 가능한 싱크를 맞추는게 좋다고 생각했습니다. 이번 미션에서 이런 방식으로 동영상을 뿌려주는게 적절한지 여쭙고 싶습니다.
이번 리뷰도 잘 부탁드립니다 :D
감사합니다.