Conversation
There was a problem hiding this comment.
고생하셨습니다.
첫 리뷰인만큼 제 리뷰 스타일을 간단하게 말씀드릴게요
- 하나의 PR에서 동일한 사항에 대해서는 모든 코드에서 언급하진 않습니다. 만약 네이밍에 대한 리뷰가 있다면 해당하는 코드에 대해서는 자체적으로 검수후 리팩토링을 진행해주시면됩니다.
- CSS 구현에 대해서는 거의 보지 않습니다. 정말 이상하다 싶은것이 아니면 CSS에 대한 리뷰는 생략합니다. 만약 어떤 layout이나 ui를 css로 표현해야하는데 어렵다 싶은것이 있다면 별도의 질문으로 남겨주시면 중점적으로 봐드릴수는 있습니다.
- "~를 이렇게 한 이유가 있나요?" 라는 질문은 정말로 질문이지 "이 코드 바꿔주세요" 라는 메시지는 아닙니다. 물론 코드만으로 의도를 파악하기 어렵다는 사인이므로, 이유가 타당하지 않다면 "이거 나중에 바꿔주세요" 라는 결과가 될 확률이 높습니다.
전반적으로 아직 미완성이라 많이 드릴 말씀은 없구요, 몇가지 개선했으면 하는 사항들입니다.
1. 불필요한 Fragment 사용
여러곳에서 불필요하게 <></> 로 컴포넌트를 감싸고 계신데요, 굳이 필요없습니다.
2. any 타입 사용
거의 대부분의 타입스크립트 코드는 any없이 사용할 수 있습니다.
3. data fetch 라이브러리 사용
지금은 react state와 useEffect 를 이용해서 data fetch를 구현하고 계신데요, swr, reactquery 같은 라이브러리를 도입해보시는것도 추천드립니다.
|
|
||
| function AddLink(): JSX.Element { | ||
| return ( | ||
| <> |
There was a problem hiding this comment.
불필요한 Fragment입니다. 단일 Wrapper만 있는경우에는 제거해주세요
| @@ -0,0 +1,19 @@ | |||
| import styles from '@/styles/addLink.module.css'; | |||
|
|
|||
| function AddLink(): JSX.Element { | |||
There was a problem hiding this comment.
굳이 returntype을 명시하지 않으셔도됩니다. 컴포넌트는 알아서 타입이 추론되게 하는게 좋습니다.
| @@ -0,0 +1,42 @@ | |||
| import Moment from 'react-moment'; | |||
There was a problem hiding this comment.
Moment보다는 date-fns 같은 유틸리티 모듈을 사용하시는 것이 좋습니다. moment는 브라우저에서 사용하기에 너무 기능이 무겁습니다.
There was a problem hiding this comment.
이 말을 듣고 라이브러리를 비교해서 쓸 필요가 있다고 느껴서 라이브러리를 정리하는 시간을 가져봤어요!
| type OpenPopoverFunc = (e: any) => void; | ||
|
|
||
| function Card({ card }: { card: any }): JSX.Element { | ||
| const [isPopoverOpen, setIsPopoverOpen] = useState(false); |
| import '@/styles/cardList.module.css'; | ||
| import NoSavedLinks from './NoSavedLinks'; | ||
|
|
||
| function CardList({ cards }: { cards: any }): JSX.Element { |
There was a problem hiding this comment.
any보다도 파일에 빈 타입이라도 만들어두고 Card[] 라는 타입 을 기입해주시는것이 좋습니다.
There was a problem hiding this comment.
any를 계속 지우는 연습해보겠습니다
| </div> | ||
| <div className="sns"> | ||
| <a href="https://www.facebook.com/" target="_blank" rel="noopener noreferrer"> | ||
| <img src="../assets/image/facebook.svg" alt="facebook 홈페이지로 연결된 facebook 로고" /> |
| return ( | ||
| <div className="header"> | ||
| <div className="header-info"> | ||
| <img className="header-avatar" src={folder?.avatar} alt="avatar" /> |
There was a problem hiding this comment.
folder가 없다면 에러가 발생할텐데요. foder가 없을때 그려질 ui를 정의해주는것이 좋습니다.
| </a> | ||
| </div> | ||
| </main> | ||
| 하이 |
|
|
||
| function useAsync(asyncFunction: any) { | ||
| const [pending, setPending] = useState(false); | ||
| const [error, setError] = useState(null); |
There was a problem hiding this comment.
data까지 제공해주고 hook 호출 시점에 내부에서 호출하는게 더 낫지 않을까요?
| [asyncFunction], | ||
| ); | ||
|
|
||
| return [pending, error, wrappedFunction]; |
There was a problem hiding this comment.
2개가 넘어가버리면 tuple보다는 객체를 반환해주는것이 좋습니다.
주요 변경사항
마이그레이션 실패
템플릿 코드를 이용해서 마이그레이션 작업을 했다가 알 수 없는 곳에서 참조 에러와 타입 에러가 계속 떠서 제 코드를 사용해서 바꿔보자 생각 했는데 이것도 매한가지로 에러가 계속 나서 꼬리에 꼬리를 따라가서 에러만 해결만 하다가 결국 못 했습니다.
타입 에러 수정
타입 에러만이라도 계속 고쳐보자 해서 API Type 주는 법을 배웠습니다.
css.module
뒤늦게 css module 사용법을 알아서 급급하게 고치다가 제출했습니다.
멘토에게
잘하고 싶었는데 죄송합니다 면목이 없네요.
템플릿 코드 나오면 그거 보고 분석해서 다시 올리겠습니다.