-
Notifications
You must be signed in to change notification settings - Fork 70
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단계 - 나만의 유튜브 강의실] 동동(김동희) 미션 제출합니다. #24
Conversation
cypress 환경 설정을 추가했다.
와... 안녕하세요!! 문서를 보고 너무 감동이었습니다 ㅠㅠ P.S. B마트는 언제나 동동님을 적극적으로 환영합니다 흑규규규규그극 ㅠㅠ 사랑해여 |
@Vallista 님 저두 사랑해여 ㅎㅎㅎ 다름이 아니라 페어의 리뷰어로부터 리뷰가 나왔는데 큰 틀은 바뀌지 않으나 세부적으로 조금 수정이 될것 같습니다( 함수 추출, 함수명 변경 등) |
@Vallista 형님, 페어 리뷰어의 리뷰를 모두 반영하였습니다! 시간 되실때 리뷰해주시면 감사하겠습니다 ❤️ |
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.
안녕하세요 동동님!
먼저 늦어서 죄송합니다 ㅠㅠ. 프로젝트가 막바지에 있어서, 마무리하느라... 정신이... ㅠㅠ
이번 미션 잘 부탁드립니다!
UX/UI 측면에서 사용성이 높은 프로젝트이다보니, 3가지 관점(UX/UI, 프로젝트 설계, 코드) 으로 피드백을 드리려 합니다!
첫 번째로 UX 측면 피드백입니다.
- 최근 검색어는 왼쪽부터 나와야 하지 않을까요?
최근 검색어가 오른쪽부터 추가되고 있는데, 리스트에 push를 하고 있어서 그런것 같습니다. unshift
로 앞에서부터 넣는게 어떨까요?
- 동영상의 preview 로드 이미지가 끝나고 로드되는게 좀 더 좋지 않을까요?
로딩이 끝나고 이미지 로드가 이루어지니 더 딜레이가 되는 느낌이 있습니다!
- 동영상 검색은 볼 영상, 본 영상 버튼과 레벨을 동일하게 하면 유저에게 혼동을 줄 것 같습니다.
동영상 검색은 눌러도 초록색으로 active 변경이 되는 요소가 아니므로, 해당하는 부분은 레이아웃을 다르게 가져가면 좋을 것 같습니다. (돋보기만 보인다던지..)
- 저장된 영상 갯수와 하단의 동영상 영역의 margin이 있었으면 좋겠습니다!
- 저장을 실수로 할 수 있어서, 다시 되돌리는 기능이 있었으면 좋겠습니다!
두 번째로 전반적인 코드 피드백입니다!
-
전반적으로 코드가 깔끔해서 흠 잡을 부분이 없었습니다... (열심히 봤는데..) 그래서 지금의 구조를 유지를 하고, 확장이 되어 갈 때 설계에 대해 보면 될 것 같습니다!
-
디테일한 코드에 대한 피드백은 코드 라인에 상세히 남겼습니다! 하지만, 큰 부분들은 아니여서 2단계를 진행하시면서 같이 해결하시면 될 것 같습니다.
-
설계상으로 데이터가 전달되는 파이프라인을 잘 구축하신 것 같아 좋았습니다 :) 이러한 설계에서 조금 더 나아가 레이어를 구축하고, 레이어 단위의 데이터가 전달되는 파이프라인을 좀 더 확장해서 제작하면 좋을 것 같습니다. 여기서 레이어라는 뜻은 비단 파일 단위로의 데이터 프로세스가 아닌, 파일들이 전체가 하나의 레이어를 구축하고, 이러한 다양한 레이어 모듈이 다른 레이어로의 접근을 하기 위해서 어떠한 파이프를 거쳐야 하는... 추후 확장을 어떻게 할 지 고민을 하시고 적용을 해보시면 좋을 것 같습니다 👍 N 계층 아키텍처 스타일을 참고해보세요!
감사합니다!
@@ -61,20 +77,266 @@ | |||
|
|||
## ⚙️ Before Started | |||
|
|||
#### <img alt="Tip" src="https://img.shields.io/static/v1.svg?label=&message=Tip&style=flat-square&color=673ab8"> 로컬에서 서버 띄워서 손쉽게 static resources 변경 및 확인하는 방법 | |||
1. yarn을 이용하여 package 초기화 |
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.
감사합니다! 이번에 환경설정을 하는 것에 대해서 정리를 하면서, README.md 에도 기재해두었습니다!
@@ -0,0 +1,4 @@ | |||
{ | |||
"baseUrl": "http://localhost:5500", |
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.
5500 포트로 지정한 이유가 있을까요?
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.
vscode 의 익스텐션인 liveserver의 기본 포트번호가 5500입니다. 이에 cypress 설정에도 liveserver 포트번호를 기재하여, 매 테스트파일마다 url과 포트번호를 입력하는 것을 지양하고자 하였습니다 😄
또한 .vscode/settings.json 에 liveserver의 포트번호를 5500 으로 강제한다는 내용을 추가하였습니다.
관련 커밋: 51aa7ed
.should("have.length", MAX_RESULTS_COUNT * 2); | ||
}); | ||
|
||
it("검색 직후 skeleton UI가 나타나고, 데이터 로드된 후 skeleton UI가 사라진다.", () => { |
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.
~사라진다 라는 표현보다 좀 더 명확한 표현을 쓰는게 어떨까요? 사라진다라는 의미는 사라지는 도중에 무언가를 캐치하는 상황도 연상할 수 있다고 생각합니다!
- UI가 제거된다. (UI가 없어진 것에 대한 테스트)
- UI가 없어진다. (UI가 없어진 것에 대한 테스트)
정도가 있을 것 같네요.
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.
'skeleton UI가 없어진다' 로 수정하였습니다! 🙏
관련 커밋입니다 bd058d5
|
||
cy.get(`.${CLASSNAME.MODAL_VIDEO_WRAPPER}`) | ||
.children() | ||
.should("have.length", MAX_RESULTS_COUNT * 2); |
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.
2를 하는 이유는 무엇일까요? 새로 불러왔기 때문에 최대 결과값 카운트의 2배를 하는걸까요?
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.
최대 값이 10 * 2가 되는 체크보다는 추가적으로 불러온 리스트가 10개가 맞는지를 확인하는게 좋을 것 같습니다.
그런데, 10개가 아니라 최대 수량을 넘어서 6개만 불러올 때도 있을텐데.. 이러한 경우에는 어떻게 될까요..?
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.
말씀하신 대로 최초 10개 불러온 후 스크롤으로 6개만 불러오는 경우도 충분히 있을 수 있는데 (또는 스크롤으로 불러올 동영상이 없는 경우 등), 그 부분에 대한 고려가 미흡했던 것 같습니다.
검색어를 입력하여 최초에 출력된 비디오들 중 마지막 비디오를 lastVideo
로 alias 한 후,
이후에 스크롤으로 불러온 비디오들의 개수가 0 ~ 10개가 되는 것을 확인하는 것으로 테스트를 수정하였습니다
cy.get(`.${CLASSNAME.SEARCH_VIDEO_WRAPPER}`)
.children("article.clip:last-child")
.as("lastVideo") // <-- 최초 검색으로 인한 비디오들 중 마지막 비디오를 lastVideo 로 alias
.scrollIntoView();
cy.wait("@searchFromScroll");
cy.get("@lastVideo")
.nextAll("article.clip") // <-- @lastVideo 이후에 추가된 비디오들
.its("length")
.should("be.within", 0, NUMBER.MAX_RESULTS_COUNT);
관련 커밋입니다: 2d12da2
cy.intercept({ | ||
url: REDIRECT_SERVER_HOST, | ||
query: { | ||
pageToken: /.+/, |
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.
요러한 RegExp는 constant로 분리해서 관리하는게 좋지 않을까요?
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.
아래와 같이 상수화하였습니다 😄
const REGULAR_EXPRESSION = Object.freeze({
EMPTY_STRING: /^$/,
NOT_EMPTY_STRING: /.+/,
});
export default REGULAR_EXPRESSION;
@@ -21,7 +21,6 @@ | |||
transition: top 0.25s ease; | |||
width: 960px; | |||
margin: auto; | |||
overflow: auto; | |||
background: #fff; |
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.
background로 전부를 가져가기보다, background-color만 사용한다면 background-color만 사용하는게 좋지 않을까요?
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.
말씀해주신 바와 같이 background-color: #fff
으로 수정하였습니다 😄
관련 커밋입니다. 7e755d3
width: 50%; | ||
} | ||
|
||
.skeleton .line { |
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.
.skeleton .line 이 두 개 네요? 한 개로 합치는 게 좋지 않을까요?
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.
앗 깜빡하고 놓친 부분이었는데 말씀해주셔서 감사합니다! 😃
관련 커밋입니다 ac2e5c8
} | ||
|
||
addMessageListener(message, messageHandler) { | ||
if (!Object.keys(MESSAGE).includes(message)) { |
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.
키에대한 예외를 설정하니까 좋네요! deliverMessage
에도 message 데이터에 대한 예외 로직을 적용하는 건 어떨까요? Messenger 내부에 private method로 만들어서 공통으로 사용하면 될 것 갍아유
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.
아 리뷰를 보고 정말 머리를 한대 얻어맞은 것 같았습니다 .. 간단한건데 여기까지 생각이 이르지 못했습니다... 역시 @Vallista 님 👍 👍 👍
message 에 대한 validation 메서드를 정의하여, addMessageListener 와 deliverMessage 양쪽에서 validation 을 수행하였습니다!
그리고 if문의 조건을 Object.keys(MESSAGE).includes(message)
가 아닌 Object.values(MESSAGE).includes(message)
로 수정하였습니다 😆
관련 커밋입니다! a6cc11a
|
||
const render = ($video, item) => { | ||
const { | ||
id: { videoId }, |
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.
요렇게 펼쳐놓으니 param을 넣는 입장에서 보고 쉽게 적용할 수 있겠네요!
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.
감사합니다!
함수의 인자로는 전체를 넘겨주고, 사용하는 곳에서는 필요한 것만 destructuring 해서 쓰는 것이 추후 확장성에 대비할 수 있을 것 같다고 판단하여 위와 같이 하였습니다 😉
`.${CLASSNAME.SAVE_VIDEO_BUTTON_WRAPPER}` | ||
); | ||
|
||
$iframe.src = `https://www.youtube.com/embed/${videoId}`; |
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.
https://www.youtube.com/
의 도메인이 수정될 수 있으니, render 선언 위에 전역 변수로 domain = https://www.youtube.com
으로 핸들링할 수 있게 선언하면 어떨까요?
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.
말씀해주신 사항 반영하여, const DOMAIN = "https://www.youtube.com";
을 전역변수로 정의하였습니다 🎉
관련 커밋입니다. a032cda
리뷰 확인해주시면, 머지하도록 하겠습니다! |
@Vallista 꼼꼼한 리뷰에 감동 먹어버렸습니다.. package.json 까지 리뷰를 해주시다니.. @Vallista 님은 사랑입니다 ❤️
리뷰는 해주시자마자 메일이 와서 바로 확인했었는데, 이 말 뜻을 이제 이해하였습니다! 그럼 일단 머지부탁드리겠습니다~! |
최근 검색어가 왼쪽부터 나오도록 수정하였습니다 😉
동영상 검색이 볼 영상, 본 영상과는 다른 수준의 버튼임을 명시적으로 보여주게끔 아래와 같이 수정해보았습니다!
마진을 추가하였습니다!! 😜
최초에는 저장 버튼이었다가, 저장을 누르면 취소 버튼으로 바뀌게끔 하였습니다. 또한 취소 버튼을 누르면 본 영상・볼 영상에서 사라지게끔도 구현하였습니다.
이 부분은 매번 해야지 해야지 하다가... 다른 일에 치여서 아직까지도 구현을 못하였습니다.. iframe 태그의 srcdoc 속성을 이용하여 말씀하신 것을 구현한 크루가 있던데 해당 부분 참고하여 다시 반영해보도록 하겠습니다..
2단계 진행하다가 말씀해주셨던 N계층 아키텍쳐를 다 잊어버리고 말았습니다.. 지금은 레이어 고려 없이 단순히 messenger 를 통해서 주고받는 형태만으로 되어 있는데, message의 개수가 늘어나다보니 전체적인 구조가 잘 안보이게 되는 것 같습니다. 특히 메세지명만을 보았을때 어떤 컴포넌트가 어디로 메세지를 전달하는지가 전혀 느껴지지 않아 리팩토링할 때 애를 먹었습니다. |
[1단계 - 나만의 유튜브 강의실] 미션 제출합니다!
@Vallista 안녕하세요 동동입니다! 바쁜 일정중에도 시간 내주셔서 제 코드를 봐주셔서 감사합니다! 🙇♂️
이 은혜는 절대 잊지 않고 나중에 꼭 갚을 수 있었으면 좋겠습니다. (저를 나중에 B마트 팀원으로 뽑아주세요...?! 😄)
1단계 기능 구현이 늦어져 PR상신이 일정보다 늦어진 점 사과드립니다...
부족한 점들 많이 말씀해주시면 적극 반영하도록 하겠습니다!
미션은 주모(@jum0)와 함께 진행했습니다!
1. 🔥 데모사이트 🔥
2.✈️ VSCODE in web ✈️
3. 🎯 구현한 기능 목록 🎯 (step1 검색 기능)
저장
할 수 있다. (실제 저장이 아닌 영상 id를 Web Storage에 저장).4. 설계에 대한 간략한 소개
각 컴포넌트 간의 의존성을 끊고 싶었습니다. 각 컴포넌트가 서로를 모른채, messenger 이라는 메세지 전달원을 통하여 데이터를 주고받도록 하였습니다.
HTML Element에 어떤 이벤트가 발생하면 실행될 콜백함수를 addEventLister에 등록하는 것과 유사하게 해보았습니다. (message-driven 방식이라고 할까요...?)
messenger은 각 컴포넌트들의 상태를 저장하지는 않으며, 단순히 메세지와 해당 메세지에 대한 데이터를 전달해주는 역할만 합니다.
각 컴포넌트들의 상태는 각 컴포넌트들이 각자 관리하며, 상태가 갱신될때마다 각 컴포넌트들이 localStorage에 저장하고 있습니다.
5. 프로그램 구조도
6. 메세지 목록
7. 디렉토리 구조
8. 부족한 점(추후 구현하고 싶은 기능 목록)
9. 질문