[1단계 - 나만의 유튜브 강의실] 도비(김정혁) 미션 제출합니다. #3

zereight · 2021-03-04T17:55:13Z

안녕하세요. 리뷰어님!
서니(@sunhpark42) 와 함께 페어를 진행하여 리뷰를 받게된 도비입니다.

데모페이지

src/js/key.js에 export const MY_KEY = 'aaaaa'; 로 Youtube Data API v3 키를 입력하여 사용하실 수 있습니다!

🎯 step1 검색 기능

질문 있어요!

innerHTML에서 이벤트를 줄 수 있는 방법이 없을까요..? element를 이벤트가 달린 상태로 생성하고 싶은데 appendChild로 하자니 코드가 너무 더러워지는것 같습니다. ㅜㅜ
리덕스를 사용하여 상태관리가 되도록 하였는데요. 상태관리에 대한 피드백도 주시면 감사하겠습니다!

ysm-dev

UI 피드백

disable된 버튼은 hover스타일을 없애고 클릭될것같은 cursor: pointer 보다는 cursor: not-allowed 같은 cursor스타일을 줘도 좋을것같아요 👍

영상을 재생 후에 팝업을 닫으면 백그라운드에서 계속 재생되어서 소리가 그대로 들리는데 이런부분은 어떻게처리해야할까요? 👀
영상 아래의 텍스트 영역이 클릭이벤트가 걸려있지 않은데에도 불구하고 cusor가 pointer네요!

innerHTML에서 이벤트를 줄 수 있는 방법이 없을까요..? element를 이벤트가 달린 상태로 생성하고 싶은데 appendChild로 하자니 코드가 너무 더러워지는것 같습니다. ㅜㅜ

event attribute를 inline으로 줄수도있지만 callback 함수가 global scope에 존재해야해서 좋은 방법은 아닙니다.

const test = () => alert('test')
$dom.innerHTML = `<div class="test2" onclick="test()">test</div>`

그리고 innerHTML도 보안이나 성능상 현업에서는 자주 사용하지 않습니다. (어떤 보안문제가있는지도 공부해보시면 좋을것같아요)
실제로는 Vanila JS로 작성한다면 대부분의 경우 코드가 더러워지더라도 document.createElement와 appendChild 를 사용하는것이 안전합니다.
이러한 한계때문에 Facebook에서는 React에서 사용되는 JSX라는 문법을 만들기도 했고, hyperscript, htm 이러한 헬퍼 함수들도 존재합니다.
내용이 방대하니 '이런것도 있구나.' 정도로 알아두시고 나중에 JSX문법과 React를 배울때 좀더 들여다보시면 좋을것같습니다. 👍

리덕스를 사용하여 상태관리가 되도록 하였는데요. 상태관리에 대한 피드백도 주시면 감사하겠습니다!

훌륭합니다! Redux의 API를 참고해서 자바스크립트로 구현하려고 많이 고민하신 흔적이 보이네요 👍
상태가 변경되면 모든 컴포넌트에 render함수를 호출하고 각 render함수에서 상태가 변경되었는지 확인하고있는데,
render함수에서 상태가 변경되었는지 조건문으로 확인하는게 아니라, 상태가 변경되었을 때만 render함수를 부르도록 만들면 코드가 좀더 간결해질것같네요! 지금은 render함수가 조금 복잡해보여요.

ysm-dev · 2021-03-06T04:27:55Z

src/js/model/LocalStorageManager.js

+    this.key = {
+      videos: LOCALSTORAGE_KEYS.VIDEOS,
+      searchHistory: LOCALSTORAGE_KEYS.SEARCH_HISTORY,
+    };


LocalStorage라는 브라우저의 API를 사용하는 util 라이브러리처럼보여요!
util함수로 빼고 이러한 도메인과 관련된 값들은 도메인과 관련된 파일에서 상수로 관리하면 좋을것같아요.

넵넵! 페어도 같은 피드백을 받아서 localstorage관련 유틸로 따로 뺴주었습니다.

ysm-dev · 2021-03-06T04:29:53Z

src/js/model/LocalStorageManager.js

+  setItem(key, array) {
+    localStorage.setItem(this.key[key], JSON.stringify(array));
+  }


array만 받을수있는 함수 말고 어떤 값이든 받을 수 있도록(어떤값이든 localStorage에 저장될수 있도록 좀더 범용적으로 바꿀수있을까요?

만약 JSON.stringify함수에 직렬화 할 수 없는 값(ex. undefined, function, Symbol, cyclic object) 등이 들어오면 어떻게 될까요? 그런상황은 어떻게 예외처리 해야할까요?

localStorage에 array로 저장하는것과 object로 저장하는것에 어떤차이가있을까요?

예외상황을 처리하기위해서, cyclic object같은 경우 decycle되게 유틸에서 만들었고, undefined, function, Symbol타입의 값들은 JSON.stringify에서 undefined가 반환되므로, undefined가 아닐때만 값을 셋팅해주도록 수정하였습니다!

지금 videos는 object의 배열 ( [{}, {}, {}] )로, searchHistory는 string의 배열 ( ["..", "..", ".."] )으로 저장하고 있는데요!
해당 key값이 가지고 있는 value가 여러개면 array형태로 저장하는것 아닌가요?.?

해당 키가 있는지 없는지 찾을때 객체로 저장할때와 배열로 저장할때 시간복잡도가 어떻게다를까요?

아하 무슨뜻인지 이해했습니다! 배열일때에는 최악의 경우 O(n)이 걸리나, videoId를 키값으로 하여 객체로 저장하면 O(1)만에 찾을 수 있겠군요!

수정했습니닷!

ysm-dev · 2021-03-06T04:31:17Z

src/js/model/Video.js

+  parseVideoEmbedUrl() {
+    return `https://www.youtube.com/embed/${this.videoId}`;
+  }
+
+  parseChannelUrl() {
+    return `https://www.youtube.com/channel/${this.channelId}`;
+  }
+
+  parseVideoUploadDate(date) {
+    const newDate = new Date(date);
+
+    return `${newDate.getFullYear()}년 ${newDate.getMonth()}월 ${newDate.getDate()}일`;
+  }


parse 라는 동사를 잘못사용하고있는것같아요 👀
parse, parsing이 어떤 동작인지 좀더 자세히 공부해보면 좋을것같습니다! 👍

하하... 파싱이 아니고 create~로 수정해두었습니다! 감사합니다!

ysm-dev · 2021-03-06T04:32:30Z

src/js/model/Video.js

+    return `${newDate.getFullYear()}년 ${newDate.getMonth()}월 ${newDate.getDate()}일`;
+  }
+
+  toJson() {


고유 명사들은 보통 모두 대문자로 써주는게 일반적입니다. (ex. ISO, HTML, JWT, etc)
toJSON

앗.. 감사합니다! 수정했습니다!

ysm-dev · 2021-03-06T04:36:03Z

src/js/model/Video.js

+  createTemplate() {
+    const fragment = document.createDocumentFragment();
+    const clip = createElement({ tag: 'article', classes: ['clip', 'd-none'] });
+    // clip.classList.add(...['clip', 'd-none']);


👀 커밋할때에는 항상 내가 커밋하는 코드를 다시 리뷰하는 습관을 들이시는게 좋습니다! :)

헉... 한번 더 체크하겠습니다!

ysm-dev · 2021-03-06T04:46:36Z

src/js/model/Video.js

+      textContent: this.channelTitle,
+    });
+    channelUrl.href = this.channelUrl;
+    channelUrl.target = '_blank';


https://github.com/woowacourse/javascript-youtube-classroom/pull/1/files#r588232590

여기에서 관련 궁금증을 해결하였습니다!
target='_blank'로 새로 열린 사이트에는 window.opener가 존재하는데 이는 보안에 취약할 수 있으므로,
noopener를 사용하여 window.opener가 새로 열린 사이트에서 존재하지 않도록 해야하는군요!

noreferrer은 분석 및 추적 소프트웨어 수치를 왜곡시킬 수 있다는 단점을 보아서? 아직 적용은 시키지 않았습니다.
둘다 적용하는게 좋을까요?

noopener만 추가하시면 될것같아요!

ysm-dev · 2021-03-06T04:50:06Z

src/js/model/YoutubeAPIManager.js

+  async requestVideos() {
+    const url = this.createRequestUrl(this.searchTerm, this.pageToken);
+
+    const res = await fetch(url, { method: 'GET' })


fetch api의 default method는 뭘까요? 👀

GET입니다. ㅎㅎ 생략해도되겠군요!

ysm-dev · 2021-03-06T04:53:55Z

src/js/redux/action.js

+export const updateVideosToBeShown = (items) => {
+  const videos = [];
+
+  items.forEach((item) => {
+    videos.push(new Video(item));
+  });
+
+  return {
+    type: UPDATE_VIDEOS_TO_BE_SHOWN,
+    payload: {
+      videos,
+    },
+  };
+};


Suggested change

export const updateVideosToBeShown = (items) => {

const videos = [];

items.forEach((item) => {

videos.push(new Video(item));

});

return {

type: UPDATE_VIDEOS_TO_BE_SHOWN,

payload: {

videos,

},

};

};

export const updateVideosToBeShown = (items) => {

return {

type: UPDATE_VIDEOS_TO_BE_SHOWN,

payload: {

videos: items.map(item => new Video(item))

},

};

};

이정도는 사실 선호의 차이일 수도 있는데, 저는 개인적으로 추가로 변수선언하지않고 작성할 수 있다면 추가 선언을 지양하는 편이예요 👍

예전 리뷰어님한테서는 변수로 선언하라는 피드백을 받아서..! 🥶
map으로 videos에 받아서 넘겨주는 방식으로 코드 길이만 줄였습니다 ㅎㅎ

ysm-dev · 2021-03-06T04:57:33Z

src/js/model/LocalStorageManager.js

+  getItem(key) {
+    return JSON.parse(localStorage.getItem(this.key[key]));
+  }


마찬가지로 JSON.parse에 파싱할수 없는 값이 들어오면 어떻게될까요? 어떻게 예외처리해야할까요?

에러를 throw하기 때문에 try/catch으로 예외처리를 해주어서 undefined를 return하도록 수정했습니다!

ysm-dev · 2021-03-06T05:33:09Z

src/js/model/YoutubeAPIManager.js

+  createRequestUrl() {
+    const pageTokenQuery = this.pageToken ? `pageToken=${this.pageToken}` : '';
+
+    return (
+      `https://www.googleapis.com/youtube/v3/search?` +
+      `part=snippet&key=${MY_KEY}
+      &q=${this.searchTerm}&maxResults=${MAX_RESULT}&type=video&${pageTokenQuery}`
+    );
+  }


URLSearchParams라는 브라우저 API를 사용해도 좋을것같아요 👍

URLSearchParams 을 사용하여 URL를 만들어주었습니다!

ysm-dev · 2021-03-06T05:48:14Z

cypress/integration/youtube_classroom_search_modal.spec.js

+    cy.get('#youtube-search-input').type(searchTerm);
+    cy.get('#youtube-search-button').click();
+
+    cy.wait(10000);


cypress의 intercept라는 API에 대해 알아보세요 👍
https://docs.cypress.io/api/commands/intercept.html#Waiting-on-a-request

cypress의 intercept는 cypress 코드에서 request를 직접보내어서 테스트 하는것 아닌가요?.?
사용자 행동에 기반하여 테스트하고 있어서.. 사용자가 검색버튼을 click했을 경우 충분한 시간을 가진 후에 결과를 체크하도록 만들었습니다!
intercept를 사용하려면 youtubeAPI를 request하는 함수의 기능만 테스트할 수 있을것 같은데요 ㅜㅜ.
제가 잘못 알고 있는건가요...!

만약 유튜브서버의 문제로 응답이 10초안에 오지 않으면 어떻게될까요?

사실 저 cypress코드서는 10초 이상이 걸릴 경우에 테스트가 통과되지 않습니다 ㅜㅜ
비동기 테스트할 방법을 찾다가.. 울며겨자먹기로 cy.wait를 사용했는데요!
이에 관해서 계속 알아보고 있습니다. 혹시 힌트가 있을까요? 🙄

위에 드렸던 링크가 힌트입니다.

앗.. 제가 docs를 잘못읽었네요!
요청을 감지하여 사용할 수 있는 것이었군요..! 수정하니까 완전! 테스트하기 용이해졌습니다. 감사합니다! >_<

zereight · 2021-03-06T09:42:20Z

리뷰어님 코멘트 감사합니다!
말씀 주신 not-allowed속성 적용했고 innerHTML에 관한 이슈들을 다시 짚어보았습니다. 감사합니다!

그런데 render함수에서 상태가 변경되었는지 조건문으로 확인하는게 아니라, 상태가 변경되었을 때만 render함수를 부르도록 만들면
이 말씀은, store의 dispatch()에서 특정 state를 검사해서 바뀌었으면 구독중인 subscriber중 몇몇만을 호출되도록 구현하라는 의미일까요? 어떻게 개선해야할지 계속 고민중에 있습니다!

페어와 리팩토링 하는 동안 코드수정이 몇번 있었습니다!

모달 외부 클릭 시, video-search-overlay라는 modal-inner의 형제 element를 두어서 modal이 close되게 구현
iframe에 srcdoc 속성을 두어 좀 더 빠르게 동영상이 로딩되는 것처럼 보이게 하기

ysm-dev · 2021-03-06T10:11:18Z

src/js/model/YoutubeAPIManager.js

+  createRequestURL() {
+    const requestURL = `https://www.googleapis.com/youtube/v3/search?`;
+    const searchParams = new URLSearchParams("part=snippet&type=video");
+
+    searchParams.set('key', MY_KEY);
+    searchParams.set('maxResults', MAX_RESULT);
+
+    if(this.pageToken) {
+      searchParams.set('pageToken', this.pageToken);
+    }
+
+    return `${requestURL}${searchParams}`


const params = new URLSearchParams({ a: 1, b: 2, }) params.toString()

이렇게 작성할수도있습니다

아하 꿀팁 감사합니다! 👍👍👍👍👍
코드에 적용해보았습니다!

ysm-dev

고생하셨습니다! 👏 다음단계에서 봬요!

sunhpark42 and others added 30 commits March 2, 2021 15:00

Chore: cypress, eslint, prettier 초기 세팅

d08078e

Docs: Readme.md 에 1단계 테스트 항목 추가

6881e98

Test: 검색어에 맞게 데이터를 가져오는 메서드를 확인하는 테스트 작성

4e80623

Test: 사용자가 요청한 결과가 화면에 잘 뜨는지 확인하는 테스트 코드 작성

140b537

Test: 최근 검색어가 제대로 추가되는지 확인하는 테스트코드 작성

b854bad

Test: 사용자가 검색어를 입력하지 않았을때(공백 포함) 아무반응도 일어나지 않는지 확인하는 테스트 코드 작성

16417c7

Test: 사용자가 영상 저장 버튼 클릭 시, 해당 버튼이 사라지고, 저장 영상 개수가 1증가하는지 확인하는 테스트 코드 작성

d57d137

Test: 남은 테스트 관련 스켈레톤 코드 작성

72fc5c3

Feat: id, class name 추가

269d20e

Feat: 유튜브 API를 통해 영상을 받아오는 기능 구현

f19e625

Feat: 기본 구조 설계 및 모달 온오프 기능 구현

d4bc11a

Feat: 상태 관리를 위한 리덕스 추가

fdeef5a

Feat: youtube request를 위한 모듈 추가

641b659

Feat: video 모듈 추가

8ec211f

Feat: 입력한 키워드로 검색한 결과를 화면에 보여주는 기능 추가

7d8511a

Fix: 날짜가 NaN으로 표시되는 이슈 수정

89dcbee

Fix: 검색된 비디오가 2차원배열이 되는 이슈 수정

7841d0f

Feat: 검색 결과 화면 그리드 설정 및 스크롤 추가

7b53e72

Feat: 비디오 검색 시 검색어를 최근 검색어에 추가하는 기능 구현

d6ea05b

Test: 최근 검색어 테스트 코드에서 입력값 초기화 및 검색 단어 추가

1360c3d

Test: 사용자가 최근 검색어를 클릭하면, 해당 검색어로 검색이 되는지 확인하는 테스트 코드 추가

f51c01a

Feat: 사용자가 최근 검색어 클릭 시, 해당 검색어로 검색되는 기능 추가

b1d73ae

Docs: Update README.md

42c8041

Test: 스크롤이 끝까지 제대로 내려가지 않는 이슈 수정

8e8eaa1

Feat: 검색 결과에서 스크롤을 내리면, 영상을 추가로 불러오는 기능 추가

50becb0

Docs: update README.md

554ef12

Test: 최근 검색어 입력 시, 최근 검색어 중복 처리를 확인하는 테스트 코드 추가

e099ce2

Feat: 최근 검색어 중복 시, 해당 단어 가장 앞으로 이동 및 검색하는 기능 추가

c357626

Docs: Update README.md

a1d4d79

Test: 검색 결과가 없는 경우, 결과 없음 이미지를 보여주는 것을 확인하는 테스트 코드 작성

363dee6

ysm-dev reviewed Mar 6, 2021

View reviewed changes

sunhpark42 added 2 commits March 6, 2021 14:32

Fix: 에러코드와 맞지 않는 메세지 출력 수정

b20795d

Refactor: 공백 입력시 require을 이용하여 안내 및 modal close 기능 보완

b12e421

ysm-dev suggested changes Mar 6, 2021

View reviewed changes

ysm-dev reviewed Mar 6, 2021

View reviewed changes

sunhpark42 added 12 commits March 6, 2021 15:24

Refactor: localStorage 관련 함수를 유틸로 분리 및 예외 처리

6fbeeac

Refactor: 메서드 네이밍 수정

a853363

Refactor: createElement에 default parameter 설정

129d72c

Refactor: YoutubeAPIManager에서 fetch의 method 명시 생략

2d2aefd

Refactor: Channel Url로 페이지 이동시 noopenner 속성 추가

c7373e2

Refactor: video 생성시 forEach 를 map 으로 변경

30744a4

Refactor: Date 변환시 유효한 형식인지 확인

a7272d7

Refactor: URLSearchParams를 이용하여 request URL 생성하도록 변경

c73a2a5

Feat: iframe에 srcdoc 속성 추가

9024306

Fix: error 발생 시 스켈레톤톤 UI가 사라지지 않는 에러 수정

122b32b

Refactor: 변수 명 변경 및 상수 처리 안된 부분 상수화

4230004

Feat: modal open 시 input focusing 기능 추가

b9b4adf

zereight force-pushed the step1 branch from ef9f443 to b9b4adf Compare March 6, 2021 08:50

ysm-dev mentioned this pull request Mar 6, 2021

[1단계 - 나만의 유튜브 강의실] 지그(송지은) 미션 제출합니다. #7

Merged

zereight added 2 commits March 6, 2021 18:27

Refactor: JSON.parse 예외처리

eeaff53

Refactor: 클릭 불가한 버튼 cursor: not-allowed 처리

b24b3a1

ysm-dev reviewed Mar 6, 2021

View reviewed changes

zereight added 4 commits March 6, 2021 21:28

Test: 비동기 테스트 intercept 사용하여 구현

68f5e5e

Refactor: URLSearchParams 선언방식 수정

2613c59

Refactor: localstorage에 video를 array가 아니라 object로 저장하도록 수정

87b23a8

Fix: 초기 localStorage 값 수정

3574264

ysm-dev approved these changes Mar 9, 2021

View reviewed changes

ysm-dev merged commit 4607a1b into woowacourse:zereight Mar 9, 2021

ysm-dev mentioned this pull request Mar 9, 2021

[1단계 - 나만의 유튜브 강의실] 곤이(김기융) 미션 제출합니다 #17

Merged

[1단계 - 나만의 유튜브 강의실] 도비(김정혁) 미션 제출합니다. #3

[1단계 - 나만의 유튜브 강의실] 도비(김정혁) 미션 제출합니다. #3

Conversation

zereight commented Mar 4, 2021 • edited Loading

🎯 step1 검색 기능

질문 있어요!

ysm-dev left a comment • edited Loading

Choose a reason for hiding this comment

UI 피드백

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zereight Mar 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zereight Mar 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zereight Mar 6, 2021 • edited Loading

Choose a reason for hiding this comment

zereight commented Mar 6, 2021 • edited Loading

Choose a reason for hiding this comment

zereight Mar 6, 2021 • edited Loading

Choose a reason for hiding this comment

ysm-dev left a comment

Choose a reason for hiding this comment

zereight commented Mar 4, 2021 •

edited

Loading

ysm-dev left a comment •

edited

Loading

zereight Mar 6, 2021 •

edited

Loading

zereight Mar 6, 2021 •

edited

Loading

zereight Mar 6, 2021 •

edited

Loading

zereight commented Mar 6, 2021 •

edited

Loading

zereight Mar 6, 2021 •

edited

Loading