-
Notifications
You must be signed in to change notification settings - Fork 71
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단계 - 나만의 유튜브 강의실] 지그(송지은) 미션 제출합니다. #32
Conversation
* chore: eslint, prettier, cypress 세팅 * docs: todo 작성 * chore: cypress 세팅 * test: 클릭한 탭의 색 변경 테스트 작성 * feat: dom 조작 라이브러리, View 기본 클래스 작성 * feat: dom 조작 라이브러리, View 기본 클래스 작성 * refactor: dom 라이브러리, View 클래스 수정 * feat: 네비게이션 탭 이동 시 버튼 색상 변경 기능 구현 * test: 동영상 검색 버튼 클릭 시 검색 모달 창 열리는 테스트 작성 * feat: 동영상 검색 버튼을 누르면 검색 모달 창이 열림 * test: 검색 모달 창의 x 버튼 클릭 시 모달이 닫히고, 볼 영상 목록으로 돌아가는 테스트 작성 * feat: 검색 모달 창의 x 버튼을 누르면 모달 창이 닫히고, 볼 영상 목록으로 돌아가는 기능 작성 * chore: api key 저장 * test: 현재 검색한 검색어가 최근 검색어 목록에 남는 테스트 작성 * feat: 검색 시 최근 검색어 3개가 남는 기능 구현 * feat: 검색 시 api 결과를 받아서 Video 인스턴스를 생성하는 기능 구현 * feat: 검색 실행 시 검색 결과를 10개씩 가져와서 보여주는 기능 구현 * test: 검색 결과를 가져오는 동안 skeleton UI를 보여주는 테스트 작성 * feat: 검색 결과를 가져오는 동안 skeleton UI를 보여주는 기능 구현 * test: 검색 결과가 없는 경우 결과 없음 이미지를 보여주는 테스트 작성 * feat: 검색 결과가 없는 경우 결과 없음 이미지를 보여주는 기능 구현 * test: 검색 후 스크롤을 내리면 검색 결과를 10개씩 더 보여주는 테스트 작성 * feat: 검색 후 스크롤을 내릴 경우 검색 결과를 10개씩 더 가져오는 기능 구현 * feat: 모달 창을 열면 가장 마지막에 검색한 검색 결과 동영상들을 보여주는 기능 구현 * refactor: Video 모델 필드 프라이빗, 날짜 출력 형식 변경 * style: 모달 검색 클립 저장 버튼 추가 * feat: 검색 결과 동영상 저장 기능 구현 * refactor: 네비게이션 탭 custom dom library event로 변경 * feat: 저장한 동영상들을 볼 영상 목록에 추가하는 기능 구현 * test: 검색 결과 동영상을 저장하면 동영상을 볼 영상 목록에 추가하는 테스트 작성 * feat: 동영상 저장 시 해당 동영상의 저장 버튼 비활성화, 저장 개수 업데이트 기능 구현 * fix: 모달 스크롤 시 최근 검색어를 유지하고, 모달을 닫을 때 검색 내역 삭제하도록 수정 * fix: 검색어를 입력하지 않을 시 경고창 띄우도록 수정 * feat: 최근 검색어 클릭 시 해당 검색어로 검색하는 기능 구현 * fix: 모달 스크롤 시에만 nextPageToken 값을 유지하도록 수정 * test: 최근 검색어를 누르면 해당 검색어로 검색을 실행한 결과를 보여주는 테스트 작성 * fix: 검색 후 모달 결과 창의 스크롤를 최상단으로 이동 * refator: 테스트 코드에서 중복되는 부분 함수로 분리 * style: - EOL 수정 - 함수 컨벤션 통일 - 주석 제거 * style: buttom type에서 submit 제거 * refactor: Nav Tab에서 버튼 별로 기능 분리 * refactor: api URL 생성 함수 분리 * stlye: 태그 속성 추가 * refactor: - localStorage key 값 상수화 - json string 검증 * chore: vscode 세팅 gitignore 추가 * chore: vscode 세팅 gitignore 추가+ * chore: vscode 세팅 gitignore 추가 * refactor: URL 제작 방식 URLSearchParams로 수정 * refactor: - dom library 불필요한 리턴 제거 - scroll throttle 함수 분리 * refactor: callback을 async-await로 변경 * refactor: - img 태그에 alt 추가 - dom library Error 상황 추가 - cypress api 요청 기다리는 함수, 실제 키워드 검색 테스트 추가 * refactor: localStorage array 저장 방식 변경 Co-authored-by: zigsong <wldms5764@gmail.com>
}); | ||
|
||
it('✅ 본 영상을 체크하면 볼 목록에서 해당 영상이 사라지고, 본 영상 목록에 추가된다. snackbar를 띄운다.', () => { | ||
setSavedVideoIds(['vRXZj0DzXIA', 'I3U0QAXeOW4', 'BS7tz2rAOSA']); |
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.
test에서 API를 지속적으로 사용하는 것이 아까워서(!)
그냥 localStorage에 저장된 영상 목록들을 임의로 심어주었습니다.
update() { | ||
this.searchModalView.updateChips(this.store.state.recentKeywords); | ||
this.searchModalView.updateSavedCount(this.store.state.savedVideoIds); | ||
} |
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.
store
의 상태가 바뀌면 store
를 구독하는(register된) observer
들(여기서는 controller)에서 update
를 실행합니다.
this.store.update( | ||
{ | ||
[STORE_KEYS.SAVED_VIDEO_IDS]: videoId, | ||
[STORE_KEYS.WATCHED_VIDEO_IDS]: videoId, | ||
}, | ||
isDelete, | ||
); |
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.
savedVideoIds
에 videoId
를 추가하라는 요청일 수도, 삭제하라는 요청일 수도 있어서
두 번째 인자로 isDelete
(bool)를 넣어주었습니다.
약간은 임시방편의 느낌이 있어서, 조언 주시면 감사하겠습니다!
const App = () => { | ||
const store = new Store(); | ||
|
||
const youtubeController = new YoutubeController(store); | ||
const searchModalController = new SearchModalController(store); | ||
|
||
youtubeController.init(); | ||
searchModalController.init(); | ||
|
||
store.register(searchModalController); | ||
}; | ||
|
||
window.addEventListener('DOMContentLoaded', 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.
페어에게 배운 방식인데, DOMContentLoaded
는 초기 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.
일반적으로 자바스크립트 파일을 body 태그 끝에서 부르는 이유는
DOM Element가 그려지기 전에 JavaScript 에서 DOM에 접근했을때에 나는 오류를 방지하기 위함이며,
자바스크립트를 파싱하고 실행하는 동안 다른 작업들이 블로킹상태가 되기 때문입니다.
괜찮은 방법입니다. 👍
src/js/models/Store.js
Outdated
save(key, value, isDelete) { | ||
const updateLocalStorage = { | ||
[STORE_KEYS.RECENT_KEYWORDS]: updateRecentChips, | ||
[STORE_KEYS.SAVED_VIDEO_IDS]: isDelete | ||
? popSavedVideoId | ||
: pushSavedVideoIds, | ||
[STORE_KEYS.WATCHED_VIDEO_IDS]: isDelete | ||
? popWatchedVideoId | ||
: updateWatchedVideoIds, | ||
}; |
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.
인자로 받아오는 key, value는 각각 localStorage의 key-value값입니다.
isDelete
가 true이면 배열에서 pop
을 실행합니다.
get computed() { | ||
return { | ||
unWatchedVideoIds: this._state.savedVideoIds.filter( | ||
(videoId) => !this._state.watchedVideoIds.includes(videoId), | ||
), | ||
}; | ||
} |
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.
'볼 영상' 탭에는 저장된 영상들 중 아직 보지 않은 영상 (= savedVideoIds
- watchedVideoIds
)들의 목록을 보여주여야 하는데, controller에서 아직 보지 않은 영상(unWatchedVideos
)을 많이 사용하여서 store가 computed property로 반환하게끔 만들었습니다.
괜찮다고 생각하지만 다른 곳에서 참고한 것이 아니라 저와 페어의 생각대로(?) 짠 것이라, 조언을 듣고 싶습니다!
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.
computed 라는 API는 뭔가 Vue에서 본 것같은 API네요!
괜찮은것같습니다. 다만 filter와 includes 모두 배열을 전부 순회하는 함수라 두 배열이 모두 커질경우
computed 에 접근할때마다 두 배열을 이중 순회하기때문에 성능저하가 일어날수도있을것같아요 👀
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.
많이 찾아봤지만 js로는 위 방법 외에는 적당한 방법이 없는 것 같아 그대로 두었습니다.
성능저하의 문제가 있을 수 있다는 점은 알고 있지만 그렇다면 어떤 방법이 있을까요? 🙄
아니면 이 정도는 그냥 허용해도 되는 것인지(?) 의견 여쭙고 싶습니다!
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.
안녕하세요 지그와 페어 프로그래밍을 진행한 심바입니다.
질문이 있어서 코멘트를 남기게 되었습니다.
const savedVideoIdsSet = new Set(this._state.savedVideoIds)
savedVideoIdsSet.delete(...this._state.watchedVideoIds)
const unWatchedVideoIds = Array.from(savedVideoIdsSet)
이렇게 Set 자료형을 이용하면 성능을 높일 수 있다고 생각합니다.
하지만 다른 부분에서는 Set을 사용하지 않다가 이 부분에서만 사용하게 되면 코드의 통일성이 깨지는 건 아닌지(?) 의문입니다.
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.
Set.prototype.delete 를 저렇게 사용하는게 맞나요?
적절한 위치에 적절한 자료구조를 선택하는것은 좋은 방법입니다. 새로운 라이브러리를 가져다 쓰는것도 아니고 내장 API이기 떄문에 통일성이 깨진다고 생각되진 않아요.
코드는 그대로두고, javascript benchmark 같은 테스트사이트에서 두 배열의 길이가 아주 커졌을때 두 코드의 성능이 어떻게 달라지는지 확인해보시면 좋을것같습니다 👍
src/js/views/SavedVideosView.js
Outdated
$('#main-videos').setEvent('click', (e) => { | ||
if (!e.target.classList.contains('pack-button')) return; | ||
|
||
const buttonPack = { | ||
videoWatched: this.bindWatchedEvent.bind(this), | ||
videoDelete: this.bindDeleteEvent.bind(this), | ||
}; | ||
|
||
Object.entries(e.target.dataset).forEach(([key, value]) => { | ||
buttonPack[key](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.
각 동영상의 버튼 (✅: videoWatched
, 🗑: videoDelete
)마다 click
이벤트 발생 시 서로 다른 동작을 하고자 Object
를 사용했습니다. 가독성 측면에서 괜찮은 것일까요?
+ 이벤트 위임을 적용하여 새롭게 저장되는 동영상 클립들의 버튼들에도 이벤트를 달도록 했습니다.
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.
e.target.dataset
이 어떤 object 형태인지 이 코드에서는 전혀 알 수가 없어서 조금
읽기 어려운거 같아요.
bindSaveEvent() { | ||
$('.clip-save-btn').setEvent('click', (e) => { | ||
e.target.setAttribute('disabled', true); | ||
const videoId = e.target.dataset.videoId; | ||
|
||
this.emit('clickSaveButton', videoId); | ||
this.updateSavedCount(); | ||
$('#modal-videos').setEvent('click', (e) => { | ||
if (e.target.classList.contains('clip-save-btn')) { | ||
this.emit('clickSaveButton', e.target); | ||
} |
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.
이벤트 위임을 적용하여 스크롤을 내려서 api로 새롭게 불러오는 동영상 클립들의 '저장' 버튼에도 이벤트를 달도록 했습니다.
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.
기존 YoutubeController가 너무 방대해져, SearchModalController(검색 모달 창에서의 비즈니스 로직을 담당)와 YoutubeController 두 가지로 나누었습니다. 기능이 많아졌을 때 Controller를 이렇게 나눠서 쓰는 것이 괜찮은지 여쭤보고 싶습니다.
- UI를 컴포넌트단위로 나누듯이 하나의 컨트롤러가 너무 많은 일을 담당하고있다고 생각되면 관심사를 분리하고 격리할 수 있는 만큼 격리하는 것은 좋은것 같습니다 👍
Video의 saved 속성과 watched 속성을 Video 모델에 넣기 힘들다고 판단하여 모든 것들(savedVideoIds, watchedVideoIds)을 localStorage.js에서 관리하고 있는데, 동작하게끔 작성하다보니 localStorage.js의 함수가 꽤나 많아졌습니다. video의 id를 push와 pop하는 상황마다 함수를 분리하였는데, 어떻게 생각하시는지 궁금합니다!
- localStorage.js에 있는 함수들이 꽤나 비슷한 역할들을 하고 있고 각 함수들이 너무 파편화되어있는것같습니다. (즉, 각 함수들이 대부분 선언되고 한번만 사용되는 것같아요.)
- 좀더 중복을 제거하고 추상화해서 범용적인 함수로 바꿔볼수있지않을까요?
SearchModalView와 SavedVideosView에 각각 이벤트 위임의 방식을 사용하여(구체적인 부분에 코멘트 달아 놓았습니다.) 새롭게 추가되는 동영상 클립들의 버튼에도 이벤트가 붙도록 했습니다. 이벤트 위임을 각각 그런 방식으로 쓰는 게 맞는지 알고 싶습니다 🤓
- 맞습니다 👍
@@ -12,11 +11,15 @@ describe('simba-tube', () => { | |||
|
|||
const interceptSearch = (keyword) => { | |||
cy.intercept({ | |||
url: 'https://www.googleapis.com/youtube/v3/search', | |||
url: 'https://zig-youtube-api.netlify.app/youtube/search', |
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.
나중에 급하게 바꾸다 보니 놓친 것 같네요 😂 상수로 빼보도록 하겠습니다!
const newVideos = [ | ||
...items.map((item) => new Video(item.id.videoId, item.snippet)), | ||
]; |
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.
여기서 Array spread를 한 이유는 무엇인가요? 👀
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.
앗 👀 다른 방식으로 쓰다가 뭔가 바꾸면서 spread를 빼지 않았나봅니닷 수정하겠습니다!
async scrollVideo() { | ||
this.searchModalView.renderSkeletonTemplate(); | ||
|
||
const response = await searchRequest(this.keyword, this.nextPageToken); |
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.
API 요청에 실패했을 때에 에러처리는 어디서하나요? 👀
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.
request 함수에서는 error를 throw하고, 사용자에게는 snackbar를 띄워주는 방식으로 처리했습니다!
} | ||
|
||
async searchVideo(keyword) { | ||
if (isEmptySearchKeyword(keyword)) { |
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.
좀더 일반적인 isEmptyString
라는 이름으로 짓고 trim()을 사용해서 빈 문자열로 검색되지 않도록 처리하면 좋을것같아요 👍
|
||
this.store.update({ [STORE_KEYS.SAVED_VIDEO_IDS]: videoId }); | ||
|
||
const videoToSave = this.videos.find((video) => video.id === videoId); |
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.
find 함수의 반환타입은 뭘까요?
videoToSave가 undefined일수도있을까요?
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.
undefined
가 나올 수 있겠다고 생각은 했는데 에러 처리를 안 했네요 😵 반영하겠습니다!
src/js/views/SavedVideosView.js
Outdated
setTimeout(() => { | ||
$('.empty-videos').show(); | ||
}, VALUE.CLIP_TRANSITION_TIME); |
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/js/views/SavedVideosView.js
Outdated
$('#main-videos > article').removeClass('fadein', 'fadeout'); | ||
packButton.toggleClass('opacity-hover'); | ||
videoClip.addClass('fadeout'); | ||
|
||
setTimeout(() => { | ||
videoClip.hide(); | ||
}, VALUE.CLIP_TRANSITION_TIME); |
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/js/views/SearchModalView.js
Outdated
@@ -38,44 +41,39 @@ export default class SearchModalView extends View { | |||
'scroll', | |||
throttle(function (event) { | |||
const { scrollTop, scrollHeight, offsetHeight } = event.target; | |||
const isBottomOfScroll = scrollTop === scrollHeight - offsetHeight; |
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.
isScrollEnded
정도가 괜찮을것같네요 👍
src/js/models/Store.js
Outdated
} | ||
|
||
notify() { | ||
if (this._observers.length > 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.
이 조건문이 꼭 필요하나요? 👀
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.
다른 곳에서 그대로 사용해서 쓰다보니 그렇게 된 것 같습니다.
필요하지 않을 것 같네요. 삭제하겠습니다!
get computed() { | ||
return { | ||
unWatchedVideoIds: this._state.savedVideoIds.filter( | ||
(videoId) => !this._state.watchedVideoIds.includes(videoId), | ||
), | ||
}; | ||
} |
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.
computed 라는 API는 뭔가 Vue에서 본 것같은 API네요!
괜찮은것같습니다. 다만 filter와 includes 모두 배열을 전부 순회하는 함수라 두 배열이 모두 커질경우
computed 에 접근할때마다 두 배열을 이중 순회하기때문에 성능저하가 일어날수도있을것같아요 👀
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.
2단계까지 고생하셨습니다!
다음단계에서 봬요! 👏👏👏👏👏
학습 로그Observer pattern
기타이벤트 위임
|
안녕하세요 리뷰어님! 심바와 함께 유튜브 미션 진행한 지그입니다.
2단계 리뷰도 잘 부탁드리겠습니다! 🙇♂️
zig-tube 데모 사이트
📝 step2 기능
snackbar
를 통해 보여준다.✅ 버튼 클릭 시 본 영상 탭으로 이동
🗑 버튼 클릭 시 영상 삭제
🙄 고민한 부분들
YoutubeController
가 너무 방대해져,SearchModalController
(검색 모달 창에서의 비즈니스 로직을 담당)와YoutubeController
두 가지로 나누었습니다. 기능이 많아졌을 때 Controller를 이렇게 나눠서 쓰는 것이 괜찮은지 여쭤보고 싶습니다.Store.js
파일을 만들어 앱 전체에서 사용하는 데이터들(recentKeywords
,savedVideoIds
,watchedVideoIds
)을 모아놓고 observer pattern을 적용해 보았습니다. 참고한 블로그 주소입니다.index.js
에서Store
를 생성 후 각 controller에 넣어주었는데, 사실 store와 observer 패턴을 적용해보기 위한 코드가 되어버려서(개발을 위한 패턴이 아닌, 패턴을 위한 개발 느낌 😂) 조금 아쉬운 느낌도 있습니다. store의 상태가 바뀔 때마다 store를 subscribe한 observer(=controller)들에서 뭔가 기능을 수행하도록 하려다가, 너무 많은 업데이트가 발생하는 것 같아 controller가 store의 상태 변화를 감지하고 동작하는 것이 아닌, 직접 기능하는 부분들이 생겨났습니다. 코드 부분 부분 좋지 않은 부분들이 보이면 지적 부탁드리곘습니다!Video
의saved
속성과watched
속성을Video
모델에 넣기 힘들다고 판단하여 모든 것들(savedVideoIds
,watchedVideoIds
)을localStorage.js
에서 관리하고 있는데, 동작하게끔 작성하다보니localStorage.js
의 함수가 꽤나 많아졌습니다. video의 id를push
와pop
하는 상황마다 함수를 분리하였는데, 어떻게 생각하시는지 궁금합니다!SearchModalView
와SavedVideosView
에 각각 이벤트 위임의 방식을 사용하여(구체적인 부분에 코멘트 달아 놓았습니다.) 새롭게 추가되는 동영상 클립들의 버튼에도 이벤트가 붙도록 했습니다. 이벤트 위임을 각각 그런 방식으로 쓰는 게 맞는지 알고 싶습니다 🤓