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

[2단계 - 나만의 유튜브 강의실] 하루(김하루) 미션 제출합니다 #46

Merged
merged 27 commits into from
Mar 16, 2021

Conversation

365kim
Copy link

@365kim 365kim commented Mar 14, 2021

자프님, 안녕하세요!

2단계로 돌아온 🙋🏻‍♀️ 하루(@365kim) 입니다.
2단계까지 디토(@dudtjr913)와 함께 페어프로그래밍을 진행했습니다!


요약

  • 데모  : 유튜브미션 2단계 - 하루
  • 구현 순서: 테스트 전체 작성 후, 각 테스트를 통과하기 위한 단위 기능 구현
  • 구현 결과: 작성한 cypress 테스트 모두 통과 ✔

유튜브 강의실 미션

final_604dc9549d534d005c619b12_871674

2단계 - 구현기능 목록

  • 가장 처음에는 저장된 영상이 없으므로, 비어있다는 것을 사용자에게 알려주는 상태를 보여준다.
  • 이후 페이지를 방문했을 때 기본 메인 화면은 내가 시청 중인 영상 들의 리스트를 보여준다.
  • 영상 카드의 이모지 버튼을 클릭하여 아래와 같은 상태 변경이 가능해야 한다.
    • ✅ 시청 완료 영상으로 체크
    • 🗑️ 버튼으로 저장된 리스트에서 삭제할 수 있습니다. (삭제 시 사용자에게 정말 삭제할 것인지 물어봅니다.)
  • 사용자가 버튼을 클릭했을 때 해당 행위가 정상적으로 동작하거나, 실패하였음을 snackbar를 통해 보여준다.
    • 시청 중인 영상에서 시청 완료 영상으로 체크하면 '영상이 시청 완료 영상으로 이동되었습니다.'
    • 시청 완료 영상에서 시청 중인 영상으로 체크하면 '영상이 시청 중인 영상으로 이동되었습니다.'
    • 삭제버튼을 클릭하면 '영상 삭제가 완료되었습니다.'
    • 요청 작업 실패시 '요청하신 작업을 수행할 수 없습니다. 문제가 지속적으로 발생되면 관리자에게 문의해주세요.'
  • 시청 중인 영상, 시청 완료 영상 메뉴버튼을 눌러 필터링할 수 있다.


제가 놓치고 있는 부분을 여러 방면으로 조언해주신 덕분에 많이 배우고 있습니다!!! 🙏
또, 낯선 개념이더라도 쉽게 이해할 수 있도록 실생활의 비유로 설명해주셔서 제가 더 잘 소화할 수 있었던 점도 정말 감사드립니다!! 🙏
이번에도 크고 작은 개선사항 코멘트로 전해주시면 열심히 개선해보겠습니다!! 🙇🏻‍♀️
감사합니다 😆

365kim added 25 commits March 14, 2021 14:50
Comment on lines 12 to +30
export default class SearchModel {
constructor(keyword = '') {
this.init(keyword);
constructor() {
this.initSearchResult();
this.recentKeywords = getListByKey(DB_KEY.RECENT_KEYWORDS);
}

init(keyword) {
initSearchResult() {
this.nextPageToken = '';
this.totalSearchResult = [];
this.keyword = keyword;
this.saveKeyword();
}

setNextPageToken(token) {
this.nextPageToken = token;
}

saveKeyword() {
if (this.keyword === '') {
return;
}

const recentKeywords = getListByKey(KEY_RECENT_KEYWORDS);
updateForNewSearch(keyword) {
this.initSearchResult();
this.setKeyword(keyword);
}
Copy link
Author

Choose a reason for hiding this comment

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

질문드립니다! 🙋🏻‍♀️ - SearchModel 로직 수정

SearchModel이 생성되면 keyword를 저장한다.
라고 저는 읽힙니다.
SearModel의 사용 부분을 가보니 SearchModel은 컨트롤러가 생성될 때 생성되고 있고요.
다시 말하면 어플리케이션의 구동 단계에서 유저가 아무 검색도 하지 않았는데 키워드를 세이브 하는 로직이 도는 셈이됩니다.
saveKeyword 내부 구현 로직도 save 를 한다고 보기 어렵습니다.
어떤 조건일 때는 delete 하는 로직이 숨어있거든요.

프로세스, 로직, 네이밍 전반적으로 다시 검토가 필요한 부분 같습니다.

1단계 merge 해주시면서 같이 남겨주신 코멘트 참고해서 메서드를 기능별로 재 분리해보았습니다.

우선 constructor 실행 단계에서 필요하지 않은 saveKeyword 메서드가 호출하지 않도록 했습니다. 또, Controller가 Model에게 새로운 검색을 위해 모델을 update하도록 명령(this.model.updateForNewSearch)하면, 모델은 지금까지의 검색결과를 초기화하고, 검색어를 설정하는 흐름으로 수정했습니다...!

이 부분에 대해서 검토해주시면 감사하겠습니다...! 🙏

Copy link

Choose a reason for hiding this comment

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

이 정도면 괜찮은 것 같습니다 :)

Comment on lines +1 to +13
export const unescape = (str) => {
return str
.replaceAll('&#60;', '<')
.replaceAll('&lt;', '<')
.replaceAll('&#62;', '>')
.replaceAll('&gt;', '>')
.replaceAll('&#38;', '&')
.replaceAll('&amp;', '&')
.replaceAll('&#34;', '"')
.replaceAll('&quot;', '"')
.replaceAll('&#39;', "'")
.replaceAll('&apos;', "'");
};
Copy link
Author

Choose a reason for hiding this comment

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

질문드립니다! 🙋🏻‍♀️ - JS, HTML 호환(?) 문제

Group 91 (2)

위 그림과 같이 HTML에서 일부 특수문자가 깨지는 현상이 있었습니다. fetch로 받아온 데이터를 JS 변수에 저장할 때, 연산자로 사용하는 일부 특수문자가 자동으로 변환되었기 때문이라고 파악했습니다. escape 유틸함수를 만들어서 일단락은 지었는데 문제의 원인을 정확히 파악하지는 못해서 질문드리게 되었습니다.

현재 저희가 만든 escape함수는 다섯가지 연산자(<, >, &, ', ") 만 처리하고 있습니다. 스택오버플로우 등 질답을 보면 이 다섯 문자에 대한 처리가 주로 나와서 우선은 그렇게 작성했습니다. 그런데 딱 이 다섯 문자만 변환되는 것인지 아닌지, 혹시 맞다면 왜 많은 JS의 연산자 중 이 다섯 문자만인지는 검색능력 부족으로 결국 알아내지 못했습니다...😥

또, 위 그림을 보면 앤드연산자(&)의 경우 영문명인 amp로, 작은따옴표(')의 경우 아스키코드값인 39로 변환되는 있었습니다. 검색 중 영문명은 IE용이라는 얘기도 보았는데, 크롬환경에서도 amp로 떠서 결국 어떤 경우에 영문명으로 뜨는지, 어떤 경우에 아스키코드값으로 뜨는지도 알아내지 못했습니다... 그래서 escape 유틸함수에서는 다섯가지 문자에 대해 '숫자'일 경우 처리와 '영문명'일 경우를 복수로 처리해주고 있는 상황입니다...

혹시 이 내용 전반에 대해 저희가 더 알아볼 수 있도록 추천해주실 만한 ✨키워드✨가 있다면 추천 부탁드리겠습니다..! 🙇🏻‍♀️
(기 검색 키워드: js html special character)

덧붙여 비슷한 문제로 cypress에서 스낵바 문구가 일치하는지 검사하기 위해 JS의 상수와 HTML 노드의 값을 비교할 때, JS의 '\n'을, HTML은 <br>를 갖고 있어 서로 다른 텍스트를 가지고 있다고 판단 되어버리는 문제도 있었습니다. 이러한 JS와 HTML간의 호환(?) 문제는 현업에서도 종종 발생하지 않을까 조심스레 예상해봅니다...! 그래서 혹시 어떻게 해결하고 계신지(e.g. 프론트에서 데이터 처리를 따로 한다. 관련 라이브러리를 사용한다. 등등)도 공유해주시면 감사하겠습니다...!! 🙏

Copy link

Choose a reason for hiding this comment

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

Copy link

@wow9144 wow9144 left a comment

Choose a reason for hiding this comment

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

1단계에서 드린 피드백이 반영되어 노력하신 모습이 보이는것 같습니다. :)
세부 코멘트 확인 하시고 다음 단계에서 또 잘 반영되길 기대 할게요

import { isWatchingMenu, isWatchingVideo, isConfirmCancelButton, isConfirmApproveButton } from './elementValidator.js';
import { MESSAGE } from '../constants.js';

export default class ClassroomController {
Copy link

Choose a reason for hiding this comment

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

Classroom 이 뭘까.... 왜 이름을 이렇게 지으셨을까..하고 한참 고민을 했습니다. :)
아마 나만의 유투브 '강의실' 이라서 Classroom 으로 지으신것 같습니다.

실무에서도 이런 네이밍 실수(?)가 종종 발생할 수 있는데
예를 들자면....처음 기획에서는 나만의 유튜브 강의실로 시작했지만, 유튜브를 검색/저장하는 좋은 기능이 있는데, 이름에 붙은 강의실이 고객층을 한정하는거 같아서 고객층을 더 넓히기 위해 강의실을 빼고 '나만의 유튜브'로 하기로 하는..... 이런 일이 비일비재 합니다.
그럼 Classroom 이라는 이름이 달린 모든 코드를 다 수정해야 합니다.
이것은 처음부터 Classroom 이라는 이름이 적절하지 않았던 것입니다.

'나만의 유투브 강의실'은 그저 텍스트(값)일뿐이므로 언제든 바뀔 수 있는 이런 텍스트로 이름(변수, 클래스, 함수 등의 식별자)을 지어버리면 변경에 굉장히 취약해지게 됩니다.
이런 경우에는 텍스트가 주는 의미에 매몰되어 네이밍을 하지 않아야 하고, 만약 이 텍스트를 변수화 한다면 어떤 이름이 적절할까 하고 추상화를 해보는 것이 도움이 됩니다.
'나만의 유투브 강의실'이 어플리케이션의 이름이었다면 AppController 로 정도로 하는게 나았을것이고 만만약 App 안에서도 어떤 특정한 역할을 한다면 그 역할에 맞는 이름을 지어주면 더 좋습니다.

우는 아기를 안고 달래고 있는 아저씨를 떠올리며 이름을 붙인다고 하면 '아빠'라는이름이 적절하지, '자프' 라는 구체적인 이름이 적절하지 않은것과 마찬가지입니다. 이 아빠는 '자프' 일수도 있고 '홍길동' 일수도 있고 '철수' 일 수도 있습니다.

}

moveVideo() {
const targetId = this.videoToManage.id;
Copy link

Choose a reason for hiding this comment

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

이 모델 내에서 videoToManage 를 찾을 수 없습니다.
아마 모델 외부에서 특정 시점에 videoToManage를 세팅하는것 같은데, 그런 세팅이 제대로 이뤄지지 않은채 moveVideo 를 실행하게 되면 undefined 에서 id 를 찾으려 하여 런타임 에러가 날 것입니다.
videoToManage의 초기화는 이 클래스에서 이뤄져야 합니다.
또는 그렇지 않을거라면 this.videoToManage 가 잘 있는지 확인하고 id를 접근하시거나 하셔야 합니다.

videoToManage 에 warning 이 잡히지 않는 에디터(또는 IDE)를 쓰신다면 설정을 바꿔 적용하시거나 에디터를 바꾸시는게 좋아보입니다. (저는 인텔리제이를 사용하고 있습니다)

@@ -2,3 +2,11 @@ export const $ = (selector) => document.querySelector(selector);
export const $$ = (selector) => document.querySelectorAll(selector);

export const isEndOfScroll = ($element) => $element.scrollHeight - $element.scrollTop === $element.clientHeight;

export const showSnackbar = ({ messenger, message, showtime }) => {
Copy link

Choose a reason for hiding this comment

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

showSnackbar({ messenger: this.$snackbar, message, showtime: SNACKBAR_SHOW_TIME });
이런식으로 사용하고 계시는데,
messenger 를 파라미터로 받는것보다는
messenger가 되는 $snackbar 를 가진 객체를 통해 snackbar.show(message, showtime) 형태로 호출 하는게 더 좋지 않을까 싶습니다.

messenger가 되는 엘리먼트에 css 로 show를 만들어놔야만 동작하는 함수입니다.
messenger로 $snackbar 가 아닌 다른걸 과연 받을 수 있겠느냐, 혹은 받을 일이 있겠느냐를 생각해보시면 좀 더 쉽게 이해하실 수 있으실것 같습니다.

Comment on lines 12 to +30
export default class SearchModel {
constructor(keyword = '') {
this.init(keyword);
constructor() {
this.initSearchResult();
this.recentKeywords = getListByKey(DB_KEY.RECENT_KEYWORDS);
}

init(keyword) {
initSearchResult() {
this.nextPageToken = '';
this.totalSearchResult = [];
this.keyword = keyword;
this.saveKeyword();
}

setNextPageToken(token) {
this.nextPageToken = token;
}

saveKeyword() {
if (this.keyword === '') {
return;
}

const recentKeywords = getListByKey(KEY_RECENT_KEYWORDS);
updateForNewSearch(keyword) {
this.initSearchResult();
this.setKeyword(keyword);
}
Copy link

Choose a reason for hiding this comment

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

이 정도면 괜찮은 것 같습니다 :)

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.

2 participants