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조 과제 제출(노준영, 박정민, 조민정) #2

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

quokka-eating-carrots
Copy link

@quokka-eating-carrots quokka-eating-carrots commented Jan 20, 2023

YouTube Clone - 3조

프로젝트 팀 레포

DEMO

팀원


FE.
FE.

FE.

기술 스택

이슈

  • NavBar가 반대로 열리게 되는 현상이 있습니다.
  • API 할당량으로 인한 이슈가 있을 수 있습니다. (특히 Search Page...) 오후 5시 이후로 들어가면 제대로 확인 가능합니다!
  • 초반에 env 를 레포에 올리는 실수가 있었습니다.

어떤 피드백이든 환영합니다!

Copy link

@sangheon-kim sangheon-kim left a comment

Choose a reason for hiding this comment

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

고생하셨습니다

슬랙에 코멘트 드렸습니다 해당사항도 반영하시면 좋을 것 같습니다. 나머지 기타 팁도 코멘트 드렸습니다

</div>
<p className={styles.description}>{comment.textOriginal}</p>
<div className={styles.likeContainer}>
<svg viewBox="0 0 24 24">

Choose a reason for hiding this comment

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

svgr 활용해서 실제 사용부에서는 컴포넌트로 넣으면 좋지 않았나 생각합니다

<p className={styles.description}>{comment.textOriginal}</p>
<div className={styles.likeContainer}>
<svg viewBox="0 0 24 24">
<path d="M18.77,11h-4.23l1.52-4.94C16.38,5.03,15.54,4,14.38,4c-0.58,0-1.14,0.24-1.52,0.65L7,11H3v10h4h1h9.43 c1.06,0,1.98-0.67,2.19-1.61l1.34-6C21.23,12.15,20.18,11,18.77,11z M7,20H4v-8h3V20z M19.98,13.17l-1.34,6 C18.54,19.65,18.03,20,17.43,20H8v-8.61l5.6-6.06C13.79,5.12,14.08,5,14.38,5c0.26,0,0.5,0.11,0.63,0.3 c0.07,0.1,0.15,0.26,0.09,0.47l-1.52,4.94L13.18,12h1.35h4.23c0.41,0,0.8,0.17,1.03,0.46C19.92,12.61,20.05,12.86,19.98,13.17z"></path>

Choose a reason for hiding this comment

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

이렇게 넣으면 어떤 아이콘인지 알 수가 없어요

navDisplay(!display);
}}
>
<svg viewBox="0 0 24 24" className={styles.menu}>

Choose a reason for hiding this comment

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

svg는 꼭 아이콘 컴포넌트로 따로 따로 떼서 명명해주면 좋겠어요 ㅎ 코드보는 사람입장에서 어떤 아이콘인지 알 수도 없고, 코드만 길어지네요 ㅎ

</header>
);
};

Choose a reason for hiding this comment

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

svg는 제발 따로 컴포넌트로 빼서 명명해주세요 :) Icons라는 컴포넌트를 만들어서 export const CloseIcon = () => {
return ...
}

사용처에서 Import해서 쓰는게 좋을 것 같습니다 그래야 코드도 간결해지고, 해당 아이콘이 어떤건지 알 수 있을 것 같아요

</div>
<button
type="submit"
onClick={(event) => {

Choose a reason for hiding this comment

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

함수 별도로 분리해서 명시해서 넣어주시고, useCallback 활용해서 최적화해주세요

// relatedVideoSearch[index]?.data?.items[0].statistics.viewCount;
const date = calcDate(video.publishedAt);
const duration = calcDuration(videoDuration);
const views = calcNum(videoViews);

Choose a reason for hiding this comment

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

calc라는 util하나에 유틸을 몰았으면 더좋지않았을까요? 그리고 명시적으로 export하면 한파일에 넣을수도 있을텐데요

}
}
getInfo();
}, []);

Choose a reason for hiding this comment

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

try, catch 씌워주세요

console.log("통신오류: ", error.response);
}
}
getComments();

Choose a reason for hiding this comment

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

try, catch 씌워주세요

return answer;
};

export default calcDate;

Choose a reason for hiding this comment

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

dateUtil로 만드는게 더 파일 명명상 맞아보여요 :) 나중에 다른 데이트 유틸 역시 만드시고, 그리고 export default 보다 명시적으로 export하셔서 최대한 다른파일에서 import코스트도 줄이시고, 파일 파편화도 막으시길 바랍니다

}
};

export default calcNum;

Choose a reason for hiding this comment

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

명시적으로 export 해주세요 :) 파일명 역시 개선이 필요해보입니다 지금 만들어두신 유틸 모을 수 있을 것 같아요

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