-
Notifications
You must be signed in to change notification settings - Fork 44
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
[유호민] Week15 #471
The head ref may contain hidden characters: "part3-\uC720\uD638\uBBFC-week15"
[유호민] Week15 #471
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.
저번에도 말씀드린거 같은데.. 전반적으로 코드가 깔끔합니다!
인터페이스 타입이나 관심사 분리 등 좀 더 신경써서 가독성을 개선할 수 있을 것 같아요.
버튼 탭 클릭 기능 중 전체 버튼 클릭 후 다른 버튼 탭 클릭 시 깜빡이는 현상을 고치고 싶은데 어떤 식으로 고치면 될 지 궁금합니다.
상태 유지 관련해서 확인을 해보면 좋을 것 같은데 코드상으로만 확인하긴 어려워서 배포 링크나 저 상주 시간에 별도로 질문해주시면 답변드릴게요
고생하셨습니다 👏
const getUsers = async () => { | ||
try { | ||
const { data } = await fetchUser(); | ||
setUser(data[0]); |
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.
users api 응답 형태를 보니 배열이고, 1개만 넘어오는 형태인 것 같아요.
토큰 값으로 현재 유저 정보를 가져오는데 왜 배열로 넘겨주는지는 모르겠지만..?!
이런 부분까지 fetchUser 에서 처리해서 명확히 유저 정보에 해당하는 값만 반환해주면 좋을 것 같아요.
(나중에 api 응답 형태가 변경됐을 때 fetchUser 만 수정하게끔요!)
fetchUser 내부에서 data 가 배열인지, 길이는 1 이상인지 체크하고, 없을 땐 undefined 나 null / 있으면 유저 정보 리턴하도록 수정하면 어떨까요?
if (!localStorage.getItem("accessToken")) { | ||
alert("로그인이 필요합니다."); | ||
router.push("/signin"); | ||
} |
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.
로직과 ui 가 섞여있는 것보다 ui 는 선언형으로 컴포넌트 함수 최하단에서 리턴하고, 요런 로직은 컴포넌트가 마운트 됐을 때 처리되도록 effect 에 두어 실행 시점을 명확히 해주는 것이 코드를 파악하는데 혼란이 덜 할 것 같습니다~
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.
겹치는 파일이 pages/folder 폴더에 있는 두 페이지 컴포넌트에 대해서 말씀하신 것 같아요~
아마 url 로 .../folder 과 .../folder/{folderId} 로 접근했을 때 표시할 페이지를 두 개의 파일로 작업해주신 것 같습니당
파일명 컨벤션 중에 optional 로 segment 값을 받을 수 있는게 있습니다. 요거 보시고 하나의 페이지로 작업해보세요!
https://nextjs.org/docs/pages/building-your-application/routing/dynamic-routes#optional-catch-all-segments
axiosInstance.interceptors.request.use( | ||
(config) => { | ||
const token = localStorage.getItem("accessToken"); | ||
if (token) { | ||
config.headers.Authorization = token; | ||
} | ||
return config; | ||
}, | ||
(error) => { | ||
return Promise.reject(error); | ||
} | ||
); |
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.
인터셉터로 헤더에 토큰 값 넣어주신 부분도 좋습니다~
스토리지 키값은 상수로 관리해서 일괄적으로 사용하면 더 좋을 것 같아요
@@ -119,9 +125,37 @@ export default function FolderMain({ | |||
handleFilter(); | |||
}, [inputValue]); | |||
|
|||
useEffect(() => { | |||
const folderIdFromURL = parseInt(router.query.folderId as string, 10) || 0; |
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.
쿼리 값이 유니언 타입이라 as 로 타입 단언해주신 것 같아요.
as 는 값 형태를 바꿔 디버깅이 어렵기 때문에 지양하는 편인데요.
타입 처리하는 건 타입 가드를 이용해 처리하면 좋을 것 같습니다.
// util
const isStringQuery = (value: unknown): value is string => {
return typeof value === 'string';
}
// hook
const useRouterQuery = (segmentName: string) => {
const { query } = useRouter();
const segment = query[segmentName];
if (!isStringQuery(segment)) {
return;
}
return segment;
}
이런식으로 타입 가드 함수와 커스텀 훅 작성해서 세그먼트 값과 타입 관련된 관심사를 분리할 수 있을 것 같습니다.
그냥 코멘트로 남기는 코드라 동작이 되는지는 모르겠네요.. ㅎㅎ 한번 확인해보시고 수정해서 적용해보세요~
타입 단언 관련 글도 참고해보세요!
https://velog.io/@kmh060020/%EC%93%B0%EC%A7%80%EB%A7%90%EB%9D%BC%EB%8A%94-Type-Assertions%ED%83%80%EC%9E%85-%EB%8B%A8%EC%96%B8-%EC%9D%80-%EC%99%9C-%EC%9E%88%EB%82%98
setClickedButton(folderIdFromURL); | ||
setFolderId(folderIdFromURL); |
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.
동일한 값을 두개의 상태로 관리하시는 이유가 있을까요?
handleButtonClick: (folderId: number) => void; | ||
handleAllButtonClick: () => void; |
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.
함수 props 네이밍 관련된 자료입니당 참고해보시면 좋을 것 같아요
https://jaketrent.com/post/naming-event-handlers-react/
clickedButton: number | null; | ||
handleButtonClick: (folderId: number) => void; | ||
handleAllButtonClick: () => void; | ||
folders: Folder[] | undefined | null; |
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.
folders 타입으로 세가지를 가지고 있는데 각각 값에 따라서 다른 처리를 해야하나용?
그게 아니라면
- folders: Folder[] (외부에서 props 값 넘겨줄 때 undefined 나 null 이면 빈 배열 전달하도록 기본값 설정)
- folders?: Folder[] (없을 수 있다는 뜻)
- folders: Folder[] | null (없을 수 있다는 뜻2, undefined 와 Null 중에선 한번 고민해보세요)
요렇게 세가지 중에 folders 값에 따라서 처리하는게 어떻게 달라지는지를 생각하고 타입을 명시해주시면 더 명확한 인터페이스가 될 것 같아요!
@@ -29,15 +30,17 @@ export default function LinkCardByFolderId({ | |||
setShowPopover(!showPopover); | |||
}; | |||
|
|||
console.log(link); |
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.
로그 지워주세용
{isPopoverOpen && ( | ||
<div | ||
ref={popoverRef} | ||
className="absolute right-0 mt-2 w-48 bg-white border rounded-md shadow-lg" | ||
> | ||
<button | ||
onClick={handleLogout} | ||
className="w-full text-left px-4 py-2 text-sm text-gray-700 hover:bg-gray-100" | ||
> | ||
로그아웃 | ||
</button> | ||
</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.
팝업 형태로 구현해주셨네요! 👍
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.
관련 로직을 커스텀훅으로 분리해줘도 좋을 것 같아요!
요구사항
기본
심화
주요 변경사항
스크린샷
멘토에게