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단계 - 나만의 유튜브 강의실] 콤피(류현승) 미션 제출합니다. #95

Merged
merged 71 commits into from
Mar 15, 2022

Conversation

compy-ryu
Copy link

@compy-ryu compy-ryu commented Mar 11, 2022

안녕하세요, 로이님! 🙇‍♂️
만나뵙게 되어 영광입니다!!

이번 미션에서는 패턴을 미리 정하지 않고
저희가 이전에 경험해본 패턴들을 통해 좋았던 부분, 단점으로 느꼈던 부분들에 대해 먼저 의견을 나누고
구조를 제안해보며 구현해보았습니다!


기능 목록

  • 유튜브 검색 기능
    • 검색어를 API에 전달하여, 검색 결과를 가져온다.
    • 검색 결과 없을 때의 오류 결과 값을 반환한다.
    • 기타 API 오류에 대해 처리 결과 값을 반환한다.
    • 사용자가 요청한 유튜브 영상을 로컬 스토리지 클래스를 활용하여 저장 혹은 취소할 수 있다.
  • 유튜브 동영상 저장 기능
    • 사용자가 요청한 동영상의 ID를 JSON 형식으로 로컬 스토리지에 100개까지 저장할 수 있다.
  • 검색 버튼을 누르면, 검색 모달이 출력되어야 한다.
  • 검색어를 입력하고 검색을 시도할 수 있다.
    • 예외 처리
    • 검색어는 빈 칸일 수 없다.
    • 현재 검색어와 중복된 검색어를 시도할 시 검색을 하지 않는다.
  • API를 통해 유튜브 검색 결과 컨텐츠 출력할 수 있어야한다.
    • 데이터를 불러오고 있을 땐 스켈레톤 UI를 보여준다.
    • 유튜브 컨텐츠는 10개씩 보여준다.
    • 검색 결과에서 스크롤이 마지막에 도착했을 때 새로운 컨텐츠를 불러온다.
    • 검색 결과가 없을 때는 요구사항의 이미지를 출력한다.
    • 검색 결과에서 동영상을 저장할 수 있어야 한다.
    • 이미 저장한 동영상은 저장 취소 버튼이 출력 되어야 한다.

🦖 콤피   🕊 호프 나만의 유튜브 강의실 설계

나만의 유튜브 강의실 설계

💁‍♂️ 구조

Core

  • Display

    DOM 요소를 직접적으로 조작하는 영역
    화면을 조작하는 영역과 UI 개발(마크업 개발) 영역 분리를 의도
    index.html : UI 개발 (마크업 개발) 영역
    Display 내부 파일 : 앱 접속 이후 화면을 직접적으로 조작하는 영역

  • Store

    비즈니스 로직을 담당하고, 그에 따라 업데이트 된 데이터를 저장하고 업데이트 되면 구독 중인 Display에게 알려주는 영역

Display

  • Share

    • Modal

      모달의 경우 단순히 1개만 존재하는 것이 아닌, 범용성이 있는 요소라 생각하여 분리

  • YoutubeClassRoom

    • Navigation

      메인 화면의 상단 메뉴를 조작하는 영역

    • SearchForm

      유튜브 검색 시 사용자의 검색어 입력을 받는 영역

    • SearchResult

      유튜브 검색 후 검색 결과를 출력하는 영역

Domain

  • YoutubeSearchStore

    현재 로딩 상태, 검색어와 유튜브에서 가져온 데이터를 저장하고 관리한다.

  • YoutubeSaveStorage

    사용자가 저장한 동영상의 아이디를 웹 로컬 스토리지에 저장 및 관리한다.

    데이터 저장 방식
    ​ [videoId, videoId, ...]

Api

API는 여러곳에서 사용될 것으로 예상하여 범용성을 위해 분리

Webpack 설정

  • 절대 경로 설정

  • 환경 변수 설정

  • 파일 로더 설정

  • Sass 설정

Step 2를 통해 추후 사용성을 고려할 시 가독성 좋은 스타일시트를 위해 설치

작동 방식

  1. Display 에서 구독 할 Store 저장
  2. Display 내 이벤트 핸들링 시 필요 시 Store 에 Dispatch 하여 상태 변경
  3. Store 에서 Dispatch 호출하여 상태 변경 및 비즈니스 로직 처리
  4. Store 에서 상태 변경시, 구독 중인 Display 내 함수 호출

🚀 후기

이번 미션에서는 패턴을 정하지 않고 구현하여, 정답이 정해져 있지 않다보니
서로 경험이나 학습했던 내용, 그리고 의견들을 많이 교환 해보고 새로운 시도도 많이하다보니
정말 재밌게, 배운 것도 많은 미션이었습니다!

누추한 코드지만, 잘못된 부분은 거침 없이 리뷰 해주시면 감사하겠습니다!!! 😀
잘 부탁드립니다!

- 변경된 Display 코어 구조에 맞춰 구조 변경
- 스켈레톤 UI 처음 1회만 렌더링
- 로딩 중 상태에서는 동영상 목록을 추가적으로 요청하지 않도록 구조 개선
- 분산된 조건문 정리
- 검색 결과 페이지에서 오류 출력 이후 다른 검색어로 대체한 후 시도할 시 로딩 인터페이스가 하단에 표기되는 문제 해결
- 실용성 없는 테스트 코드 제거
- 검색 결과 상태 처리 부분 테스트 코드 추가
- 사용되지 않는 유틸
@compy-ryu
Copy link
Author

안녕하세요, 로이님!
피드백 반영이 늦어져 죄송합니다 🥲

말씀 주신 피드백에 대해 다시 한번 고민해보고, 전체적으로 수정한 후
댓글 남겨주신 부분들에 대해 커밋 링크와 함께 내용을 정리하여 답글 작성하였습니다!

추가로 내용에 작성해주신 부분들은 아래에 작성하였습니다.

화면 레이아웃은 한 줄에 4개씩이니 한 번에 로드하는 컨텐츠 개수도 4의 배수로 맞추면 좋겠어요.

12개씩 불러오도록 변경 완료하였으며, 스켈레톤 인터페이스도 해당 갯수에 맞춰 출력되는 것이 사용자 경험에 더 좋을 것 같아
시작 시 동일한 갯수를 그리도록 개선하였습니다.


전체 리스트가 로딩 완료되고 난 뒤에 다시 스크롤을 내리면, 첫 페이지에 대해 다시 요청(pageToken 없이 요청)을 하게 됩니다. 특정 검색어에 대한 전체 페이지 개수는 정해져 있음에도, 무한히 같은 데이터가 반복해서 로드되고 있네요. 이는 심각한 문제이니 반드시 해결해주셨으면 합니다.

유튜브 API를 통해 총 결과 갯수를 저장한 후
불러온 갯수를 초과할 시 추가 로드를 멈추도록 버그 수정 완료하였습니다!


마지막에 테스트삼아 잠깐 스크롤을 빠르게 내렸다가 순식간에 API 횟수 초과가 되어버렸는데, 이부분에 대한 보완이 필요해요.

스크롤을 감지하는 엘리먼트를 로딩 중에는 출력되지 않도록 스타일시트로 처리하여
보완해보았습니다.


추상화에 많은 공을 들이신 것 같아요. 개발하면서 즐거우셨을 것 같습니다. 다만 이번과 같은 규모가 작은 프로젝트에서는 이런 추상화/패턴화가 오히려 가독성 / 빠른 개발을 해치는 것은 아닐지에 대해서도 한 번쯤 생각해 보시면 좋겠습니다.

이번에 페어와 설계하는 것이 재밌긴 했지만
초반에 설계 부분에서 굉장히 오랜 시간을 소모한만큼, 남겨주신 말씀에 매우 공감갑니다....!

아직 추상화 클래스에 대해 이해가 많이 부족하지만...
프로젝트를 어느정도 완성한 이후, 공통된 부분들에 대해 추상화를 시도하는 방향이
역시 맞는 것 같습니다...!

부족했던만큼 더 열심히 배워보겠습니다!


이외 개선사항

  • 검색어를 변경한 후 다시 검색할 시 스크롤이 최상단으로 이동 되도록 수정하여 간헐적으로 2페이지씩 로드되지 않도록 개선하였습니다. (27b8acc)
  • Display 영역에서 DOM 조작이 늘어날 시 render 부분이 비대해지는 것이 코드 가독성을 해칠 것으로 생각되어, 렌더링 요청 시 drawList에 등록/분리된 메서드들을 순회하며 실행하는 식으로 개선해보았습니다. (d036e09, 5b25f98)

여러 부분에서 다시 고민해볼 수 있었던, 세세한 피드백 정말 감사합니다 👍

이번 개선사항에서도 잘 부탁드립니다...!!!!
감사합니다!!

Copy link

@roy-jung roy-jung left a comment

Choose a reason for hiding this comment

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

리뷰와 함께 댓글도 같이 일괄해서 날리고 싶었는데 그러자니 outdated된 항목에는 코멘트를 남길 수가 없네요; 하는수 없이 일단 이렇게 남기고, 추가로 outdated된 항목들에 남긴 질문에 코멘트 남기도록 할게요.
그리고 한 번 리뷰를 진행하였고 동작 자체는 잘 되고 있으니 일단 승인은 해두겠습니다. 수정하고자 하는 부분 있으면 추가로 수정해 주세요. 머지는 내일 진행하도록 할게요. 고생하셨습니다. :)

};

dispatch(type, data) {
const stateByType = {

Choose a reason for hiding this comment

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

this때문에 그렇게 하신 것 같군요. 각 메소드 내에서 state를 인수로 처리하면 아예 외부로 빼낼 수 있어요. redux를 많이 참고하신 것 같은데, 리덕스도 그렇게 구성되어 있습니다. 다음과 같이요

const reducers = {
  [ACTION_TYPE.UPDATE_SEARCH_KEYWORD]: (state, keyword) => {
    return { ...state,  searchKeyword: keyword, ... }
  }, ...
}
class YoutubeSearchStore {
  dispatch(type, data) {
    const newState = reducers[type](this.state, data)
    if (newState) this.setState(newState)
  }
  ...

Comment on lines 8 to 11
const paramsResult = Object.entries(params).reduce((previous, [key, value]) => {
previous.set(key, value);
return previous;
}, new URLSearchParams());

Choose a reason for hiding this comment

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

new URLSearchParams(params)

로 충분합니다.

VIDEO_ITEM_SAVE_BUTTON: '.video-item__save-button',
SEARCH_RESULT_NOT_FOUND: '.no-result',
}),
});

Choose a reason for hiding this comment

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

"약속일 뿐 실제로 변경이 안되는 것은 아니다" -> 참입니다.
"실제로 변경한다면 그 사람이 약속을 안지킨 것이다" -> 역시 참이죠.
그렇다면 다음과 같이 정리가 될 것 같아요.

  • 누군가 약속을 지키지 않을 것이 염려되는 상황인 경우 -> freeze를 하여 강제성 부여.
  • 팀 내 컨벤션이 확고하여 약속을 지키지 않을 가능성이 현저히 적은 경우 -> 굳이 freeze를 할 필요까지는 없을 듯.

Comment on lines +30 to +33
export const DOM_NAME = Object.freeze({
ID: removeSelectorSymbol(SELECTOR.ID),
CLASS: removeSelectorSymbol(SELECTOR.CLASS),
});

Choose a reason for hiding this comment

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

  • "자바스크립트 내에서 사용 중인 선택자들을 다른 개발자에게 알려주어, 템플릿 수정 시 주의를 요하고 싶었습니다." 이 부분은 선택자를 사용하는 코드가 도처에 널려있을 경우에는 의미가 있습니다. 당연히 선택자가 수정될 상황이 생긴다면, html, css, js 모두에 수정이 필요할 것입니다. 그런 때에 js에서 해당 값을 찾아 변경하는 작업이 필요한 것도 맞죠.
  • 그런데 각 선택자가 오직 한군데씩만 쓰이고 있다면, 그 곳을 찾아서 변경하는 것과 상수를 찾아가서 수정하는 것 사이에 얼마나 큰 차이가 있을까요? IDE의 일괄검색/바꾸기 기능을 쓰면 constant를 변경하는 것과 아무런 차이가 없지 않나요?
  • 저는 일반론적인 말씀을 드린 것이 아닙니다. 지금 이 프로젝트에서의 효용성을 논하고 싶었던 거예요. 오버엔지니어링이 아닌지 고민해보자는 취지예요.

Comment on lines 7 to 14
export default class App {
constructor() {
new Modal();
new Navigation();
new SearchForm();
new SearchResult();
}
}

Choose a reason for hiding this comment

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

클래스를 사용해야 한다는 강박을 내려놓으셔도 됩니다..ㅎㅎ

Comment on lines 10 to 12
this.addEvent('click', '.dimmer', () => {
this.handleCloseModal();
});

Choose a reason for hiding this comment

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

handleCloseModal을 arrow function으로 작성하면 이렇게 표현이 가능해요

this.addEvent('click', '.dimmer', this.handlerCloseModal)

익명함수를 한 번 감싸지 않아도 되니 효율적이죠.

}

bindEvents() {
this.addEvent('click', SELECTOR.ID.SEARCH_MODAL_BUTTON, this.handleOpenModal.bind(this));

Choose a reason for hiding this comment

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

마찬가지로, handleOpenModal을 arrow function으로 작성하면 bind(this)가 없어도 됩니다.

Comment on lines 156 to 158
searchKeyword
? this.$scrollObserver.classList.add('enable')
: this.$scrollObserver.classList.remove('enable');

Choose a reason for hiding this comment

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

this.$scrollObserver.classList.toggle('enable', !!searchKeyword)

@roy-jung
Copy link

event handler를 arrow function으로 작성할 것을 추천드린 이유에 대해서는 링크로 설명 대신하겠습니다.
#111 (comment)

@roy-jung
Copy link

혹시 아직 작업 진행중이실까요? 아니시면 머지할까 하고요~

@compy-ryu
Copy link
Author

compy-ryu commented Mar 15, 2022

늦어져서 죄송합니다!!

지금까지 한 작업은 일단 모두 푸시하였습니다!!!
혹시 1단계 작업 중 생긴 궁금증들은 2차 PR에서 함께 여쭤보아도 될까요?


추가로 제가 몇몇 부분은 의도를 잘못 이해 했음에도 불구하고, 다시 알려주셔서 감사합니다..
덕분에 1단계 과정을 통해 정말 많이 찾아보게 되었고, 많이 얻어가는 것 같습니다!

부족했던만큼 2단계에서 더 열심히 하도록 하겠습니다!!
정말 감사합니다! 🙇‍♂️

Copy link

@roy-jung roy-jung left a comment

Choose a reason for hiding this comment

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

당연히 2단계에 이어서 계속 가야죠! 질문도 마찬가지구요 :)
고생하셨습니다. 머지할게요~

@roy-jung roy-jung merged commit e41aae3 into woowacourse:compy-ryu Mar 15, 2022
@compy-ryu
Copy link
Author

넵! 정말 고생하셨습니다!!

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.

3 participants