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

[3단계 - 나만의 유튜브 강의실] 동동(김동희) 미션 제출합니다 #69

Merged
merged 60 commits into from
Apr 21, 2021

Conversation

bigsaigon333
Copy link

@bigsaigon333 bigsaigon333 commented Apr 3, 2021

@Vallista 님 안녕하세요 동동입니다. 아직 3단계 기능 구현 및 전에 주신 피드백 반영 전입니다.

일단 레벨1기간때 PR을 올려두고자 이와 같은 Draft PR을 작성하였습니다.

조만간 조속히 다시 정식으로 리뷰 요청드리도록 하겠습니다!

PR이 늦어져 정말 죄송합니다 ㅠㅠ 😭 😭 😭 😭 😭 😭


@Vallista 님!! 드디어 3단계 기능 다 구현하였으며, 비교적 만족할만큼(?) 리팩토링도 수행하였습니다.

2단계 PR에 리뷰해주신 부분에 대한 코멘트도 모두 기재하였습니다.

엊그제부터 작업시작해서 하루만에 끝내려고 했는데, 이것저것 건드리다보니 이제서야 PR 리뷰 요청드리게되었습니다 😭

개인적으로 이번 리팩토링하면서 느낀 점을 제가 나중에 볼 목적으로(?) 아래에 기술해봅니다..

  1. 코드가 있어야 할 곳에 코드가 있게끔 하자: 너무 어려운 구조, 실험적인 구조, 완성도가 떨어지는 구조라면 해당 구조에 대한 참고 자료를 작성해두자. -> 미래의 내가 편하다...

  2. 코드를 많이 본만큼 코드가 익숙하다: 남이 볼 때도 친숙한 코드를 짜려면, 보편적인 컨벤션으로 보편적인 코드를 짜자. (1번과 일맥상통)

  3. 렌더링부분은 CSS로 제어하는 것을 먼저 고려하자: JS로 복잡하게 로직을 짜지 않고 CSS로만 해결할 수 있는 부분이 꽤 많다.

  4. class를 쓸꺼면 제대로 객체지향으로 작성하자: 객체지향도 공부하자...


1. 🔥 데모사이트 🔥

localStorage에 저장하는 데이터의 형식을 변경하여, 기존의 자료와 충돌이 있을수도 있습니다.

데모사이트 확인시에 localStorage clear 부탁드립니다!

2. ✈️ VSCODE in web ✈️

3. 🎯 구현한 기능 목록 🎯🎯 (step3 유저 경험 증가 기능)

  • 반응형 웹: 유저가 사용하는 디바이스의 가로 길이에 따라 검색 결과의 row 당 column 개수를 변경한다.
    • 992px 이하: 3개
    • 768px 이하: 2개
    • 576px 이하: 1개
  • 좋아요 버튼을 누른 데이터만 필터링해서 보여준다.
  • 스크롤 페이징 방식을 이용해서 Lazy loading을 개선한다.

4. 메세지 목록

image
image

5. 디렉토리 구조

📦js
┣ 📂Main
┃ ┣ 📜MainContainer.js
┃ ┗ 📜MainVideoWrapper.js
┣ 📂Search
┃ ┣ 📜KeywordHistory.js
┃ ┣ 📜SavedVideosCount.js
┃ ┣ 📜SearchContainer.js
┃ ┣ 📜SearchForm.js
┃ ┗ 📜SearchVideoWrapper.js
┣ 📂Video
┃ ┣ 📜MainVideo.js
┃ ┣ 📜SearchVideo.js
┃ ┣ 📜Video.js
┃ ┣ 📜index.js
┃ ┗ 📜template.js
┣ 📂constants
┃ ┣ 📜classname.js
┃ ┣ 📜index.js
┃ ┣ 📜localStorageKey.js
┃ ┣ 📜number.js
┃ ┣ 📜regularExpression.js
┃ ┣ 📜snackbarText.js
┃ ┗ 📜videoType.js
┣ 📂messenger
┃ ┣ 📜index.js
┃ ┣ 📜message.js
┃ ┗ 📜messenger.js
┣ 📂utils
┃ ┣ 📜API.js
┃ ┣ 📜DOM.js
┃ ┣ 📜decodeHTML.js
┃ ┣ 📜index.js
┃ ┗ 📜snackbar.js
┣ 📜App.js
┗ 📜index.js

6. 3단계 진행시 유의한 점

  1. 각 비디오를 나타내는 Video Class를 정의하였습니다. Video 객체가 각자 데이터를 전달받아 렌더링하도록 하였습니다.
    또한, Video가 '볼 영상'의 비디오인지, '본 영상'의 비디오인지를 상태변수로 가지고 있습니다. 상태가 변할때마다 html tag의 class도 변경하도록 하여 '볼 영상'・'본 영상' 화면에서 표시되는 비디오를 CSS로 제어하였습니다.

  2. thumbnail을 빠르게 표시하여 UX를 증진시킨 점

동영상의 preview 로드 이미지가 끝나고 로드되는게 좀 더 좋지 않을까요?
로딩이 끝나고 이미지 로드가 이루어지니 더 딜레이가 되는 느낌이 있습니다!

기존에 주셨던 리뷰에 대하여 드디어 대응하였습니다. 엄청 시간이 오래 걸릴 줄 알고, 사실 좀 뒤로 미뤄뒀는데,
막상 해보니 어느 리팩토링보다 빠르게 구현할 수 있어 즐겁고 기뻤습니다 ㅎㅎ..

일단 youtube API server로부터 data를 받은 후에 thumbnail만 빠르게 표시하고, thumbnail을 클릭한 경우에 iframe을 로드하도록 하여, 검색 결과를 빠르게 표시하게끔 하였습니다!

@Vallista
Copy link

Vallista commented Apr 8, 2021

네엥~~ 천천히 주세용!

@bigsaigon333 bigsaigon333 changed the title [Draft] [3단계 - 나만의 유튜브 강의실] 동동(김동희) 미션 제출합니다 [3단계 - 나만의 유튜브 강의실] 동동(김동희) 미션 제출합니다 Apr 10, 2021
@bigsaigon333
Copy link
Author

@Vallista 님 시간되실때 리뷰 부탁드립니다 ~ 😄

우테코에서 저를 내쫓지만 않는다면, 늦게(?) 리뷰 주셔도 괜찮습니다 ㅎㅎ..

사랑해여 ❤️

Copy link

@Vallista Vallista left a comment

Choose a reason for hiding this comment

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

안녕하세요! 동동님!

길고 길었네요.. 10Days ago라니... 넘 늦게 피드백 드려서 죄송합니다!
2단계에 비해서 3단계 코드가 워낙 많이 바뀌어서, 확인하는데 오래걸렸고,
다른 리뷰와 병행하면서 엄청 늦어졌네요 ㅠㅠ 죄송합니다.

코드 단위의 피드백은 크게 할만한 부분은 없었구요~ 전반적인 구조나 코드에 대한 피드백을 진행하도록 하겠습니다!

전반적인 구조 피드백

  1. Form 그리고 API 연동 부분에 대한 예외처리가 조금 미흡한 것 같습니다. 사용자 입장에서 에러가 발생할 경우, 해당 로그에 대한 추적은 되어있지만, 다시 진행을 할 수 없는 코드로 짜여져 있는 것 같아요. 그래서 try-catch에 대한 부분을 작성하는게 필요해보입니다! 혹은, 에러가 나왔을 경우에 다른 값으로 유지를 한다던지..

  2. localstorage 부분이 각각 영역마다 파티션이 되어있는데, 이 부분은 매니저 같은걸 만들어서 한 곳에서 관리를 했으면 합니다. localstorage에 대한 에러가 발생했을때 각각 부분에서 해결을 해야하니까요!

  3. 그 외에는 전반적으로 잘 짠 코드로 보입니다 👍

  4. 나머지 피드백은 간단하게 코맨트로 달아놓았습니다


고생 많으셨습니다!
다음 프로젝트 (리액트)도 화이팅 하세용!

display: none;
a {
text-decoration: none;
color: black;

Choose a reason for hiding this comment

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

오.. color black... inherit이 낫지 않을까요?

}

.btn.absolute {
top: 2.5rem;
.like-tab {

Choose a reason for hiding this comment

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

.tab 으로 감싸고 .like를 넣는건 별로일까요?

.tab > .like

position: fixed;
z-index: 1;
bottom: 0px;

Choose a reason for hiding this comment

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

0과 0px은 같은 의미와 기능을 제공합니다 :)

조금이라도 텍스트가 줄으면 좋으니, px를 삭제해주는게 좋아보입니다!

@@ -109,3 +156,21 @@
background-position: 320px;
}
}

@media (max-width: 992px) {

Choose a reason for hiding this comment

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

992px이나 768px 등의 레이아웃은 css variable로 관리하면 더 좋을 것 같아요~

네이밍을 통해 어떤 디바이스를 기준잡는지 알기 쉬울 것 같아보임니다!

import SearchContainer from "./Search/SearchContainer.js";

export default class App {
#searchContainer = new SearchContainer();

Choose a reason for hiding this comment

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

변수를 위에 선언하는게 좋을까요. 아래 선언하는게 좋을까요?

https://stackoverflow.com/a/1411795

@Vallista Vallista merged commit c353611 into woowacourse:bigsaigon333 Apr 21, 2021
@bigsaigon333
Copy link
Author

학습로그

package.json versioning

private class field, public class field

Front-End 에서 API Key 숨기기

태그

  • private class field, public class field, semantic versioning spec, Client-side 에서 API Key 숨기기

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

2 participants