-
Notifications
You must be signed in to change notification settings - Fork 0
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
5조 과제(홍혜원, 김지영, 이은영, 조승후) #6
base: main
Are you sure you want to change the base?
Conversation
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.
고생많으셨습니다
슬랙으로 전체 코멘트 드렸고, 기타 나머지 사항들도 해당 PR에 코멘트 드렸습니다
src/App.js
Outdated
<QueryClientProvider client={queryClient}> | ||
<Header setMenuDrop={setMenuDrop} /> | ||
<div style={{ display: 'flex' }}> | ||
{location.pathname === '/watch/:videoId' ? null : <Sidebar menuDrop={menuDrop} />} |
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.
상세인 경우와 아닌 경우에 대해서 별도로 명명해서 레이아웃용 컴포넌트를 만들었더라면 어땠을까하는 아쉬움이 있네요 그렇게하면 추후에 App에서 코드 수정이 아닌 레이아웃 컴포넌트를 변경하면 되지않나 생각듭니다.
{ path: 'watch/:videoId', element: <VideoDetail /> }, | ||
{ path: 'results', element: <VideoSearch /> }, | ||
{ path: 'results/:keyword', element: <VideoSearch /> }, | ||
], |
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.
굳굳 너무 칭찬합니다 :) 리액트 라우터를 너무 깔끔하게 잘썼어요 ㅎㅎ
params: { key: process.env.REACT_APP_YOUTUBE_API_KEY }, | ||
}); | ||
|
||
export default instance; |
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.
인스턴스 만들어서 사용한것 개인적으로 칭찬합니다~ 근데 export default 보다 차라리 명시적으로 instance명을 붙여서 export해주면 나중에 서버 여러대와 api 송수신을 할때 authInstance, todoInstance 등 각 사용처에 맞게 처리해두는 것도 나중에 확장성에서 좋지 않을까 생각합니다 ㅎ
// } | ||
// }) | ||
// }) | ||
// } |
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.
Fake Api를 만드시는 시도는 좋았습니다 ㅎ
msw라는 목킹 라이브러리를 사용해보는 것도 좋을 것 같습니다.
return response.data.items[0].snippet.thumbnails.default.url; | ||
}; | ||
|
||
// 조회수 api |
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.
jsdoc 형식으로 주석 명시해주세요
/** 조회수 api */
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.
이유는 바꿔보시고 해당 함수 사용처나 현재 해당함수에서 마우스오버해보세요
// 이벤트 핸들러 해제 | ||
document.removeEventListener('mousedown', clickDocument); | ||
document.removeEventListener('touchstart', clickDocument); // 모바일 대응 | ||
}; |
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.
잘하셨습니다 :)
</button> | ||
<button> | ||
<MdOutlineRestore className={styles.sidebarIcon} size="24"></MdOutlineRestore>시청기록 | ||
</button> |
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.
하드코딩 자제해주세요
<p className={styles.time}>{timeFormat(publishedAt)}</p> | ||
</div> | ||
); | ||
} |
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.
잘하셨습니다 :)
if (viewCount >= 100000) { | ||
return (viewCount / 10000).toFixed() + '만회'; | ||
} | ||
}; |
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.
단위라고 명시해주는게 더 좋은 명명아닐까요?? viewCount 단위
return <VideoCards />; | ||
}; | ||
|
||
export default Videos; |
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숫자씩 되게 했습니다. 하지만, 10분 이하인 동영상인 경우, 분이 2글자일 필요가 없는데 더 좋은 방법이 있을지 궁급합니다. 피드백 받고 싶은 점 페이지 공통 레이아웃
[img src 대신 import로 이미지 불러오기]
창크기별로 반응형을 만들고, 창크기별로 버튼에 다른 모션을 주는 것이 어려웠습니다. 버튼을 누르면 확장형 사이드바가 없어지거나 모달 사이드바가 뜨는 것에 대해, state를 하나로 하여 작업하다가 두 개로 변경하여 작업했습니다. 이 과정에서 헤더와 사이드바, app.js 전체 레이아웃 등 고려해야할 것이 많아 어려웠습니다. useContext를 사용해야 했는지 궁금합니다. useRef - useRef 사용하는 것이 어려웠고, 수정하여 없어진 부분이나 자식 컴포넌트로 ref prop을 넘겨주는 것이 불가능 해 fowardRef 를 사용하려 했습니다. 하지만 에러가 났는데 해결하지 못해 사용하지 못했습니다. |
Front-End : React Youtube Clone
🔗 YouTube 페이지
🔗 Team5 배포 페이지
🔗 Team5 Git Repository
🔗 Team5 Notion
🦖 팀 소개
레이아웃
🦖 프로젝트 기간
1차 : 2023.01.23(월) ~ 2023.01.20(금)
2차 : 2023.01.25(수) ~ 2023.02.03(금)
🦖 기술 스택
🦖 작업 상세 내용
👩💻김지영
[메인페이지]
👩💻이은영
[레이아웃]
- 영상 디테일 페이지 : 사이드바 모달 형태 - 그외 페이지 - 사이드바 반응형에 따라, main 영역 사이즈 변경됨 - 스크롤 디자인[헤더]
- 검색바 반응형[사이드바]
- 데스크탑 사이즈에서는 버튼 클릭시, 토글로 열림 - 태블릿과 모바일 사이즈에서는 버튼 클릭시, 모달로 열림 - 모달일 때 - 사이드바 외 영역 클릭시, 모달 닫기 - 버튼으로 모달 닫기 - hover 시에만 스크롤 보임👩💻조승후
👩💻홍혜원
검색페이지
상세페이지
⭐️⭐️⭐️⭐️⭐️⭐️⭐️⭐️⭐️⭐️⭐️⭐️⭐️⭐️⭐️⭐️⭐️⭐️⭐️⭐️
🙏2차 피드백 받고 싶은 점, 궁금한 점
👩💻김지영
👩💻이은영
1차 피드백 받은대로 레이아웃용 컴포넌트를 추가했습니다. 잘 사용한 것인지 궁금합니다.
반응형은 최대한 css로 하는 것이 좋은 건가요? header와 sidebar 컴포넌트에서 반응형별 컴포넌트를 다르게 보여주는 부분이 있습니다. 이걸 css로 가져가는게 더 좋은 방법인걸까요?
중첩 삼항연산자를 쓰는 것에 대해 어떻게 생각하시나요?
👩💻조승후
👩💻홍혜원