code review hawnbinyoo#2
Conversation
|
신청해주셔서 감사합니다! |
ericjypark
left a comment
ericjypark
left a comment
There was a problem hiding this comment.
안녕하세요 환빈님! 코드리뷰 신청해주셔서 감사합니다!
전체적인 코드에 대해서 리뷰를 해드렸습니다!
다만 말씀하신 문제에 대해서는 제 측에서는 재현이 되지 않기도 하고, 코드리뷰의 목적과는 조금 동떨어진 느낌이 없지 않아 있어서 추가적인 언급은 하지 않았습니다.
궁금하신 부분이 있으시거나, 이해가 되지 않는 부분, 또는 같이 논의 해보고 싶으신 부분이 있으시다면 편하게 커멘트 남겨주세요!
좋은 서비스 만드시느라 고생 많으셨습니다!
| const list = data.results; | ||
|
|
||
| return list; |
There was a problem hiding this comment.
바로 return data.results를 해도 무방할 것 같습니다!
| }; | ||
|
|
||
| export const getFilterList = async (names: string) => { | ||
| const ArrayNames = names.split(" "); |
There was a problem hiding this comment.
변수명은 PascalCase가 아닌 camelCase가 좋을 것 같습니다!
| }; | ||
|
|
||
| export const getFilterList = async (names: string) => { | ||
| const ArrayNames = names.split(" "); |
There was a problem hiding this comment.
역시 사소한거긴 하지만, 개인적으로 변수명을 지을때는 타입/이름이 아닌, 이름/타입의 구조를 더 선호하긴 합니다.
이 경우에선 ArrayNames가 아닌, nameArray 또는 names가 될 수 있겠네요
| export const getFilterList = async (names: string) => { | ||
| const ArrayNames = names.split(" "); | ||
| let objectArray: object[] = []; | ||
| ArrayNames.map((i) => { |
There was a problem hiding this comment.
사소한거긴 하지만, i 보단 더 의미있는 이름이 좋을 것 같아요! 지금같은 경우에선 arrayNames을 순회하는거니까
arrayNames.map((name) => {})
가 될 수 있겠네요!
| console.log(e); | ||
| return {}; |
There was a problem hiding this comment.
에러가 발생할 시, return문까지 넘어가면 안될 것 같아요!
이런 경우에선 저는 Sentry를 이용하여 에러 트래킹을 해주고 있고, 그 이후에는 바로 throw를 줍니다.
| const handleTitleClick = () => { | ||
| setField(""); | ||
| setFilterCategoryArray([]); | ||
| return router.push("/"); | ||
| }; |
There was a problem hiding this comment.
함수명이 직관적이라 좋은 것 같습니다! 👍
다만 이 함수가 매번 새로 생성될 필요는 없어보이네요! useCallback을 이용해보시는건 어떨까요?
| onKeyDown={(e: any) => { | ||
| if (e.key === "Enter") handleClick(); | ||
| }} |
There was a problem hiding this comment.
event의 타입을 any로 하면 간편할 수는 있어도, 정석적으로는 타입을 작성해주시는게 좋습니다!
| <S.TagBtns> | ||
| <input | ||
| defaultChecked | ||
| type="radio" | ||
| value={field} | ||
| id="전체" | ||
| name="분야" | ||
| onClick={() => { | ||
| handleSubmitBtnClick(""); | ||
| setField(""); | ||
| }} | ||
| /> | ||
| <label htmlFor="전체">전체</label> | ||
| <input | ||
| type="radio" | ||
| value={field} | ||
| id="영화" | ||
| name="분야" | ||
| onClick={() => { | ||
| handleSubmitBtnClick("영화"); | ||
| setField("영화"); | ||
| }} | ||
| /> | ||
| <label htmlFor="영화">영화</label> | ||
| <input | ||
| type="radio" | ||
| value={field} | ||
| id="드라마" | ||
| name="분야" | ||
| onClick={() => { | ||
| handleSubmitBtnClick("드라마"); | ||
| setField("드라마"); | ||
| }} | ||
| /> | ||
| <label htmlFor="드라마">드라마</label> | ||
| </S.TagBtns> |
There was a problem hiding this comment.
지금 당장은 세개의 버튼밖에 존재하지 않고, 토이프로젝트이기에 늘어날 가능성도 높지 않겠지만, 현업에서는 이러한 버튼의 개수가 추가되거나 삭제되거나 하는 경우가 적지 않습니다.
따라서 저라면 이 값들을 하나의 배열로 빼고, map을 이용하여 컴포넌트를 그려줄 것 같네요!
그쪽이 가독성 측면에서도 더 좋지 않을까 싶습니다!
| const router = useRouter(); | ||
| const [filterToggleBtn, setFilterToggleBtn] = useState(false); | ||
| const [filterCategoryArray, setFilterCategoryArray] = useState<string[]>([]); | ||
| const [field, setField] = useState(""); | ||
| const [searchValue, SetSearchValue] = useState<string>(""); | ||
| const imgUrl = useRecoilValue(imgAtom); | ||
| const [isScroll540, setIsScroll540] = useState(false); | ||
| const [isDetailPage, setIsDetailPage] = useState(false); | ||
| const pathname = usePathname(); |
There was a problem hiding this comment.
저라면 아마 이 Header 컴포넌트를 여러개의 컴포넌트로 또 나눌 것 같습니다!
그렇게 하면 Header 컴포넌트 하나에서 모든 상태를 다 관리하지 않게 되기 때문에 가독성 측면에서도 좋고, 불필요한 리렌더링 역시 방지가 가능합니다.
| }) { | ||
| return ( | ||
| <html lang="ko"> | ||
| <body className={inter.className} suppressHydrationWarning={true}> |
There was a problem hiding this comment.
suppressHydrationWarning가 필요했던 이유가 있으실까요? 일반적이지 않은 해결법으로 보이는데, 근본적인 원인을 해결하는게 더 중요하지 않을까 싶습니다!
Next에서 Hydration 에러를 던지는데는 이유가 있으니까요!
There was a problem hiding this comment.
이부분도 skeleton ui 사용할려다가 추가한 코드인거같습니다! 지금은 사용안해서 지우겠습니다!
There was a problem hiding this comment.
시간 내줘서 코맨트 달아줘서 감사합니다!
혹시 개발환경에서만 테스트하셔서 이슈를 발견 못하신거 아니였을까요?
배포한 사이트를 보면
위사진과같이 개발환경과 달리 배포환경은 홈페이지 이미지가 꺠진것을 볼 수있습니다
혹시 이현상에 대해서 제가 간과한 로직이나 짐작가는게 있으시다면 코맨트 부탁드립니다!
|
배포 사이트를 지금 접속해보아도 동일하게 이미지는 문제 없이 보이고 있습니다. 다른 페이지에서는 잘 되는데, 홈페이지에서만 안되는거면 Notion DB의 문제는 아니지 않을까 싶네요. 더 자세한 설명 없이는 문제에 대한 도움을 드리기가 힘들 것 같습니다ㅠ |
2 participants
Pull Request 템플릿
배포
로컬 환경
리뷰 요청 ✍️
- 신청하게 된 계기, 간단한 사연
안녕하세요 깃허브에서 우연히 보게되어서 신청하게되었습니다.
취준생은 아니고 2월에 입사예정인 학생인데 입사전에 더 공부하고 싶고 해결하지 못하고있는 이슈도있고 좋은기회일거같아서 신청하게되었습니다.
제 웹사이트는 notion db 를써서 데이터베이스를 만들고 api 로 불러오는 웹사이트입니다.
그런데 홈페이지의 img 가 계속 베포후 시간이지나면 에러가 떠서 확인해보니까
notion db 를 사용할때 notion db 에있는 img 가 3시간 유효기간이있어서 생기는 에러였습니다.
그래서 SSG도 안쓰고, 캐시도 없애고 매요청마다 데이터를 불러오는 로직을 짯는데
다른페이지는 잘되는데 홈페이지만 이미지오류가 뜨는 이유를 알고싶습니다.
- 중점적으로 리뷰받고 싶은 부분
이슈의 근본적인 이유를 알지 못해서 그 부분에 대해 중점적으로 리뷰받고싶습니다.
- 나누고 싶은 고민