-
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
[1단계 - 나만의 유튜브 강의실] 지그(송지은) 미션 제출합니다. #7
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.
다른 크루의 코드를 참고하여 dom.js를 custom library로 구현했는데, 쓰다보니 파일이 방대해져 오히려 가독성과 효율(?)이 떨어지는 것 같다는 생각이 들었습니다. 페어와 더욱 고민하여 내용과 사용 방식을 바꿔봐야겠지만, 리뷰어님께서는 해당 라이브러리를 어떻게 생각하시는지 여쭤보고 싶습니다.
- 동일한 피드백을 드린적이있어서 링크로 대체합니다! ([2단계 - 행운의 로또 미션] 곤이(김기융) 미션 제출합니다. javascript-lotto#40 (review))
SavedVideosView에서 처음에 저장된 동영상의 id들을 가지고 해당 동영상들을 불러오는 request를 더 보내는데, 이전에 이미 api를 통해 검색 결과로 불러온 동영상들을 저장하고 다시 불러올 때 또 다시 api를 호출해야 하는 부분에서 조금 아쉬운 점이 있습니다.
- HTTP Cache 에 대해 공부해보시면 좋을것같아요.
많은 기능들을 구현하다보니 YoutubeController가 조금 방대해진 느낌이 있습니다. 추후 단계를 진행하기 전에 controller도 view와 마찬가지로 분리하는 방식이 괜찮을지 여쭤보고 싶습니다!
- 크다고 생각되면 분리하는건 좋은 선택입니다 👍
api 키를 담은 파일을 gitignore에 추가해서 github 데모 페이지를 만들 수 없어 netlify를 이용했습니다. 다만 완전하지는 않은 방안이라 데모 페이지의 콘솔에서 api key가 노출되어 있긴 합니다 😂 빠르게 배포하려다 보니 최선의 해결책을 찾지 못한 것 같습니다.
- 별도의 서버가 없이 프론트엔드만 있는 프로젝트에서는 완벽하게 API KEY를 숨길 수 없습니다. 이렇게 개인이 key를 발급받아 사용하는 Third-party API key의 경우에는 별도 서버에 해당 키를 두고, 그 서버를 proxy로 api를 요청하거나, 혹은 개인이 발급받은 API key를 그냥 노출하고 API 키에 다른 보안 정책(referer 등)을 추가하는 방법 밖에 없습니다. (아마 Youtube API key를 발급 받을 때 여러가지 보안 정책을 추가할 수 있을거예요)
수고하셨습니다 ! 👍
src/css/ui/modules/modal.css
Outdated
@@ -21,7 +21,7 @@ | |||
transition: top 0.25s ease; | |||
width: 960px; | |||
margin: auto; | |||
overflow: auto; | |||
/* overflow: auto; */ |
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/css/ui/modules/skeleton.css
Outdated
100% { | ||
background-position: 320px; | ||
} | ||
} |
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/css/ui/modules/youtubeCard.css
Outdated
@@ -1,15 +1,17 @@ | |||
.video-wrapper { | |||
display: grid; | |||
grid-template-columns: repeat(auto-fill, 236px); | |||
/* grid-template-rows: repeat(auto-fill, 254px); */ |
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 publishedAt() { | ||
const options = { year: 'numeric', month: 'long', day: 'numeric' }; | ||
return this._publishedAt.toLocaleDateString('ko-KR', options); |
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/request.js
Outdated
const requestURL = pageToken | ||
? `${SEARCH_URL}&key=${youtubeKey}&q=${keyword}&pageToken=${pageToken}` | ||
: `${SEARCH_URL}&key=${youtubeKey}&q=${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.
#3 (comment)
비슷한 피드백이 있어서 링크 남깁니다 👍
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.
URLSearchParams
를 사용하여 수정했습니다. 새로운 내용 알아갑니다. 감사합니다 🤓
src/js/views/SearchModalView.js
Outdated
this.modalVideos.setInnerHTML( | ||
` | ||
<div class="empty"></div> | ||
<img class="not-found" src="./src/images/status/not_found.png"></img> |
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.
img 태그를 작성할때는 항상 alt 속성을 같이 작성하는 습관을 들이시면 좋습니다. 👍
src/js/utils/dom.js
Outdated
@@ -0,0 +1,73 @@ | |||
export const $ = (function () { | |||
const constructor = function (selector) { | |||
if (!selector) return; |
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.
document.querySelector 에 파라미터를 넘기지 않거나 string이 아닌 타입을 넣으면 어떻게될까요?
woowacourse/javascript-lotto#40 (comment)
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.
코멘트 참고해서 Error
를 던져주도록 수정해보았습니다! 감사합니다 🙃
cy.get('#search-btn').click(); | ||
cy.get('#modal-search-input').type('방탄소년단'); | ||
cy.get('#modal-search-button').click(); | ||
|
||
cy.get('#modal-search-input').clear().type('데이식스'); | ||
cy.get('#modal-search-button').click(); | ||
|
||
cy.get('#chip-1').should('have.text', '데이식스'); | ||
cy.get('#chip-2').click(); | ||
cy.get('#modal-search-input').should('have.value', '방탄소년단'); | ||
cy.get('#chip-1').should('have.text', '방탄소년단'); |
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.
cypress의 메소드를 잘 활용할 줄 몰라 넘겼는데 다른 크루에게 남겨주신 리뷰 참고하여 작성해보았습니다!
src/js/utils/localStorage.js
Outdated
} | ||
|
||
recentKeywords.unshift(keyword); | ||
localStorage.setItem('searchKeyword', JSON.stringify(recentKeywords)); |
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.
- 만약 JSON.stringify함수에 직렬화 할 수 없는 값(ex. undefined, function, Symbol, cyclic object) 등이 들어오면 어떻게 될까요? 그런상황은 어떻게 예외처리 해야할까요?
- localStorage에 array로 저장하는것과 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.
1번의 문제는 페어의 리뷰를 보고 먼저 수정했습니다!
2번의 경우 검색해보니, localStorage에 array로 저장하면 join이 된 채로 저장되기 때문에
stringify
를 쓰지 않고 저장한 후, 값을 가져올 때는 split
을 사용하는 방법도 있다는 것을 알게 되었습니다!
감사합니다 👾
cypress/fixtures/example.json
Outdated
"name": "Using fixtures to represent data", | ||
"email": "hello@cypress.io", | ||
"body": "Fixtures are a great way to mock data for responses to routes" | ||
} |
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.
😂
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.
고생하셨습니다! 👏 다음단계에서 봬요!
* [1단계 - 나만의 유튜브 강의실] 지그(송지은) 미션 제출합니다. (#7) * 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 추가 * refactor: URL 제작 방식 URLSearchParams로 수정 * refactor: - dom library 불필요한 리턴 제거 - scroll throttle 함수 분리 * refactor: callback을 async-await로 변경 * refactor: - img 태그에 alt 추가 - dom library Error 상황 추가 - cypress api 요청 기다리는 함수, 실제 키워드 검색 테스트 추가 * refactor: localStorage array 저장 방식 변경 * chore: mock data 추가 * fix: not found image display 방식 수정 * refactor: skeleton UI DOM tree에서 제거 Co-authored-by: 0imbean0 <tjsqls2003@gmail.com> * docs: 2단계 Todo 작성 * test: 저장된 동영상이 없으면 비어있음을 알리는 이미지를 보여주는 테스트 작성 * feat: 저장된 영상이 없으면 비어있음을 알려주는 이미지를 보여주는 기능 구현 * test: 영상을 100개 이상 저장 시 alert을 띄우는 테스트 작성 * feat: 영상을 100개 이상 저장 시 alert을 띄우는 기능 구현 * refactor: Store와 observer pattern 적용 * test: 동영상 저장 시 성공/실패 여부를 알려주는 snackbar를 띄우는 테스트 작성 * feat: 동영상 저장 시 성공/실패 여부를 알려주는 snackbar를 띄우는 기능 구현 * refactor: localStorage 저장 방식 변경 * test: 본 영상을 체크하면 버튼이 표시되고 snackbar를 띄우는 테스트 작성 * feat: 본 영상을 체크하면 버튼이 표시되고 snackbar를 띄우는 기능 구현 * test: 동영상 삭제 버튼 클릭 시 confirm을 띄우고, 동의 시 삭제하는 테스트 작성 * feat: 동영상 삭제 버튼 클릭 시 confirm을 띄우고, 동의 시 삭제하는 기능 구현 * test: 각 영상 탭에 맞는 영상 목록을 보여주는 테스트 작성 * feat: 각 영상 탭에 맞는 영상 목록을 보여주는 기능 구현 * style: snackbar css 변경 * style: clip transition css 추가 * refactor: 볼 영상 탭에서 본 영상 제거 * test: snackbar와 url 주소 변경 * fix: 본 영상 목록에서 삭제시 데이터가 남아있던 문제 해결 * refactor: 동동's API 서버 적용 * feat: 각 탭에서 동영상 목록이 비게 되면 텅 이미지를 보여주는 기능 구현 * refactor: validator 분리 * refactor: - 비즈니스 로직 순서 변경 - App 로드 방식 변경 - keyword 데이터 뷰에서 컨트롤러로 이동 - 이벤트 위임 적용 * refactor: localStorage 기본값 리턴 로직 수정 * fix: 탭 간 이동 또는 ✅ 버튼 체크 시 재생 중인 동영상 정지 * refactor: 페어 리뷰 반영 - api 요청시 에러 상황 처리 - 변수 및 함수명 변경 * refactor: - 사용하지 않는 기능 삭제 - 이미지 alt 명확하게 작성 - 함수명 수정 * style: snackbar css 수정 * refactor: 의미가 모호한 변수명 변경 Co-authored-by: jieun song <44080404+zigsong@users.noreply.github.com> Co-authored-by: zigsong <wldms5764@gmail.com>
학습로그custom dom library
HTTP Cache
promise-then보다는 async-await (ref)
throttle (ref)export default function throttle(callback, delay) {
let ticking;
return function () {
if (ticking) return;
ticking = setTimeout(() => {
ticking = null;
callback.apply(this, arguments);
}, delay);
};
}
|
안녕하세요! 페어 심바와 유튜브 미션 진행한 지그입니다.
기간에 맞추어 급하게 미션을 진행하느라 코드에 실수도 많고 깔끔하지 못한 부분들이 많을 것이라고 생각됩니다 😭
시간 내서 계속 리팩토링을 해 보도록 하겠습니다.
리뷰 잘 부탁드리겠습니다 😉
데모 페이지
검색 결과 스크롤
동영상 저장
최근 검색어 클릭
검색 결과 없음
🤔 고민한 내용
다른 크루의 코드를 참고하여
dom.js
를 custom library로 구현했는데, 쓰다보니 파일이 방대해져 오히려 가독성과 효율(?)이 떨어지는 것 같다는 생각이 들었습니다. 페어와 더욱 고민하여 내용과 사용 방식을 바꿔봐야겠지만, 리뷰어님께서는 해당 라이브러리를 어떻게 생각하시는지 여쭤보고 싶습니다.SavedVideosView
에서 처음에 저장된 동영상의 id들을 가지고 해당 동영상들을 불러오는 request를 더 보내는데, 이전에 이미 api를 통해 검색 결과로 불러온 동영상들을 저장하고 다시 불러올 때 또 다시 api를 호출해야 하는 부분에서 조금 아쉬운 점이 있습니다.많은 기능들을 구현하다보니
YoutubeController
가 조금 방대해진 느낌이 있습니다. 추후 단계를 진행하기 전에 controller도 view와 마찬가지로 분리하는 방식이 괜찮을지 여쭤보고 싶습니다!api 키를 담은 파일을
gitignore
에 추가해서 github 데모 페이지를 만들 수 없어 netlify를 이용했습니다. 다만 완전하지는 않은 방안이라 데모 페이지의 콘솔에서 api key가 노출되어 있긴 합니다 😂 빠르게 배포하려다 보니 최선의 해결책을 찾지 못한 것 같습니다.