Skip to content
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단계 - 나만의 유튜브 강의실] 밧드(감우영) 미션 제출합니다. #102

Merged
merged 69 commits into from
Mar 15, 2022

Conversation

kamwoo
Copy link

@kamwoo kamwoo commented Mar 11, 2022

안녕하세요! 고생 많으십니다 🙂
아래 유튜브 강의실 데모 페이지 링크입니다.
데모 페이지


  • 구조는 진입 페이지 뷰, 모달 화면 뷰, 비디오 아이템 뷰로 나누었고 각 뷰의 이벤트를 담당하는 모듈을 하나 만들었습니다. 이번에는 mvc 패턴에서 최대한 벗어나보자는 생각으로 진행을 했습니다.
  • netlify로 유튜브 api 서버에서 데이터를 요청하고 받아오는 서버를 하나 두고 환경변수로 key를 설정하는 방식으로 진행했습니다.
  • 10개 단위로 비디오를 데이터를 받아와 렌더링을 하고, 스크롤이 바닥을 찍었을 때 pageToken을 query string에 추가해서 다음 10개를 받아와, 추가로 10개를 렌더링 하도록 했습니다.
  • 저장 버튼을 누르면 localStorage에 리스트로 저장하도록 했습니다.

검색 결과가 없을 때 이미지 경로를 못찾습니다. webpack으로 따로 설정을 해줘야할지, 외부 이미지 주소를 줄지 고민중에 있습니다. 왠지 간단한 문제일거라고 생각이 들어서 좀더 찾아보고 리뷰 후에 수정하도록 하겠습니다.


질문

  1. 스크롤이 바닥을 찍으면 10개를 추가로 렌더링하게 되는데, 매번 요청을 하기에는 너무 많은 요청이 들어갈 것 같다는 생각이 들어서 한 번에 30정도 받고, 10개씩 보여주는 방법이 있을 것 같다고 생각이 드는데 어떻게 생각하시는지 궁금합니다.

kamwoo and others added 30 commits March 8, 2022 15:12
Co-authored-by: jhy979 <jhy979@users.noreply.github.com>
Co-authored-by: jhy979 <jhy979@users.noreply.github.com>
Co-authored-by: jhy979 <jhy979@users.noreply.github.com>
Co-authored-by: jhy979 <jhy979@users.noreply.github.com>
Co-authored-by: jhy979 <jhy979@users.noreply.github.com>
Co-authored-by: jhy979 <jhy979@users.noreply.github.com>
- 썸네일 이미지 주소
- 제목
- 작성자
- 작성요일
- 비디오 id

Co-authored-by: jhy979 <jhy979@users.noreply.github.com>
Co-authored-by: jhy979 <jhy979@users.noreply.github.com>
Comment on lines 5 to 23
describe('API 요청을 하기 전', () => {
test('endPoint와 params로 requestURL을 만들 수 있다.', () => {
const endPoint = videoAPICaller.endPoint;
const params = {
q: 'woowa',
type: 'video',
};

expect(APIManager.createQueryString(endPoint, params)).toEqual(
`${videoAPICaller.endPoint}?q=woowa&type=video`
);
});
});

describe('API 요청이 끝나고', () => {
test('response 데이터를 썸네일 이미지 url, 제목, 작성자, 작성요일, id로 분리할 수 있다.', () => {
expect(videoAPICaller.parsingVideoData(videoData)).toEqual(parseData);
});
});

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 테스트는 search.test.js 보다는 각 APIManager, videoAPICaller 를 테스트하는 게 맞을 것 같습니다.
API 요청하기전/후 를 명시해놨지만 사실 테스트만 놓고 봤을 때, 전혀 상관없는 상황인 거 같아요.
createQueryString 를 테스트할 때 반드시 videoAPICaller.endPoint 가 필요한 게 아니라 endPoint 는 임의의 변수로 테스트해도 될 거 같고요!

Copy link
Author

@kamwoo kamwoo Mar 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

__test__폴더에서 각 파일에 기능 테스트로 분리했고, 파일이 있는 위치로 이동시켜 봤습니다.
분리하고 보니깐 기존에 테스트들을 모아놓은 것보다 확인하기 편하다는 느낌을 받았습니다.

API 요청하기전/후 를 명시해놨지만 사실 테스트만 놓고 봤을 때, 전혀 상관없는 상황인 거 같아요.

이 부분에서는 describe는 써야되겠고 뭐라고 할지 모르겠고 했던게 있었습니다. 애매하게 분리하기보다는 쓰지 않기로 했습니다.
104ef93

endPoint 는 임의의 변수로 테스트해도 될 거 같고요!

임의의 변수를 아무거나 endPoint로 줬습니다.
a479fbb

Comment on lines 4 to 5
describe('보고 싶은 영상을 검색 했을 때', () => {
test('입력 없이 버튼을 눌렀다면 error를 throw한다.', () => {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

사실 엄밀하게 보면 이것들도 명확한 테스트라곤 볼 수 없을 것 같습니다.
지금 테스트에서는 검색을 하지도 않고, 버튼을 누르지도 않았으니까요..!
isValidSearchInput > 빈 값일 경우 throw 를 한다..? 정도가 맞지 않을까 싶은데 어떻게 생각하시나요!?

Copy link
Author

@kamwoo kamwoo Mar 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

저도 기능 단위 테스트가 아니라 e2e 처럼 써져있다고 생각이 들었습니다.
다음 같이 수정했습니다. efbdab7 9c584ca

Comment on lines 1 to 13
export const DOM_STRING = {
MODAL_CONTAINER: '.modal-container',
DIMMER: '.dimmer',
VIDEO_LIST: '.video-list',
SEARCH_BUTTOM: '.search-input__search-button',
SEARCH_INPUT: '.search-input__keyword',
SEARCH_NO_RESULT: '.search-result.search-result--no-result',
HIDE: 'hide',
MODAL_OPEN_BUTTON: '#search-modal-button',
SKELETON: 'skeleton',
VIDEO_ITEM_SAVE_BUTTON: 'video-item__save-button',
VIDEO_ITEM: 'video-item',
};

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

클래스, 아이디, 태그명? (#, .)이 생략된 selector? 들이 섞여있는데 혼동될 여지는 없을까요?!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

실제로 사용하면서 헷갈린 적이 있었습니다..
css 클래스 상수를 CLASS_NAME_STRING으로 분리했습니다.
7e2d432

title: item.snippet.title,
url: item.snippet.thumbnails.medium.url,
channelTitle: item.snippet.channelTitle,
isLastPage: isLastPage,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아실 것 같지만, key,value 가 같을 경우 생략가능합니당

Suggested change
isLastPage: isLastPage,
isLastPage,

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

넵 🙂

}
}

async onvideoListScroll(inputValue) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(소곤소곤)

Suggested change
async onvideoListScroll(inputValue) {
async onVideoListScroll(inputValue) {

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😓😓

Copy link

@yongholeeme yongholeeme left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

세세하게 수정해주셔서 감사합니다 ;)
간단하게 몇가지 코멘트 남겨봤는데, 이 부분만 의견달아주시고 추가 수정부분은 옵셔널입니당ㅎ

코멘트 외에 추가로 수정해보면 좋을 점

  • 추가 list 를 불러오는 시점을 좀 더 앞당겨볼 것 (스크롤이 맨 바닥에 닿을 때 요청하면 UX 상 좀 느리게 느껴짐)
  • 더이상 불러올 리스트가 없을 경우 UI 제공
  • scrollHeight 대신 다른 방법으로 인피니티스크롤 구현해보기

@kamwoo
Copy link
Author

kamwoo commented Mar 15, 2022

추가 list 를 불러오는 시점을 좀 더 앞당겨볼 것

이 부분은 스크롤을 강하게 했을 때 요청이 너무 많이 빠르게 갈까봐 의도한 부분이 있었습니다. 자연스럽게 추가 비디오 아이템들이 보이게 오프셋을 추가했습니다.

더이상 불러올 리스트가 없을 경우 UI 제공

반영하지 못한 부분인데, 어떻게 보여주면 좋을지 생각이 안들었습니다. 지금 더이상 불러올 리스트가 없을 때 스크롤을 해도 추가 요청이 안들어가게 되어있어서 추후에 보완해보도록 하겠습니다!

scrollHeight 대신 다른 방법으로 인피니티스크롤 구현해보기

찾아보니깐 IntersectionObserver을 무한 스크롤을 구현할 때 많이 사용하는 것을 알게 되었고, 적용해보느라 굉장히 애를 먹었습니다. 이걸 사용하려면 코드를 거의 대부분 수정해야되겠다고 생각했고, 부담이 있어서 오프셋을 주고 throttle 시간을 조절해서 최대한 자연스럽게 무한 스크롤이 되도록 수정했습니다!

@yongholeeme
Copy link

이 부분은 스크롤을 강하게 했을 때 요청이 너무 많이 빠르게 갈까봐 의도한 부분이 있었습니다. 자연스럽게 추가 비디오 아이템들이 보이게 오프셋을 추가했습니다.

throttle을 제외하고 api 에 대한 loading 상태를 두어 해당 api 메서드가 요청이 되어 loading 이라면 early return (더 요청되지 안도록)하게끔 해 요청을 막아봐도 좋을 거 같아요.

반영하지 못한 부분인데, 어떻게 보여주면 좋을지 생각이 안들었습니다. 지금 더이상 불러올 리스트가 없을 때 스크롤을 해도 추가 요청이 안들어가게 되어있어서 추후에 보완해보도록 하겠습니다!

그냥 단순 텍스트도 좋아요~ 그냥 마지막 리스트입니다? 더 이상 불러올 영상이 없습니다 정도?

찾아보니깐 IntersectionObserver을 무한 스크롤을 구현할 때 많이 사용하는 것을 알게 되었고, 적용해보느라 굉장히 애를 먹었습니다. 이걸 사용하려면 코드를 거의 대부분 수정해야되겠다고 생각했고, 부담이 있어서 오프셋을 주고 throttle 시간을 조절해서 최대한 자연스럽게 무한 스크롤이 되도록 수정했습니다!

지금도 좋은 방법입니다! 만약에 step2 에서 다 하신 후에 시간이 남으면 도전해보시면 좋을 것 같아요!

Copy link

@yongholeeme yongholeeme left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

커밋링크도 함께 남겨주셔서 리뷰하게 무척이나 수월했습니다 👍
말씀드린 부분 이상으로 잘 수정해주셔서 다음 step 2 로 넘어가도 될 것 같습니다. 고생하셨습니다!!

@yongholeeme yongholeeme merged commit 2d212b3 into woowacourse:kamwoo Mar 15, 2022
@yongholeeme
Copy link

@kamwoo 머지되었지만 step 2 PR올리기 전에 궁금한 점 있으면 코멘트 주세요!

@kamwoo kamwoo deleted the step1-kamwoo branch March 15, 2022 14:08
@yongholeeme
Copy link

아 8ㅅ8 그리고 제가 제대로 확인을 못했는데, throttle 이나 말씀드린 인피니티스크롤 API 요청에 대해서 step2 에서 개선이 되면 좋겠습니다 :)
지금은 API 가 중복되어서 요청되고 그러긴 하네요 8ㅅ8
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants