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

[유호민] Week15 #471

Conversation

HMRyu
Copy link
Collaborator

@HMRyu HMRyu commented May 25, 2024

요구사항

기본

  • 링크 공유 페이지의 url path를 ‘/shared’에서 ‘/shared/{folderId}’로 변경해 주세요.
  • 폴더의 정보는 ‘/api/folders/{folderId}’, 폴더 소유자의 정보는 ‘/api/users/{userId}’를 활용해 주세요.
  • 링크 공유 페이지에서 폴더의 링크 데이터는 ‘/api/users/{userId}/links?folderId={folderId}’를 사용해 주세요.
  • 폴더 페이지에서 유저가 access token이 없는 경우 ‘/signin’페이지로 이동하게 해주세요.
  • 테스트 유저는 id: “[email protected]”, pw: “sprint101” 를 활용해 보세요.
  • 폴더 페이지의 url path가 ‘/folder’일 경우 폴더 목록에서 “전체” 가 선택되어 있고, ‘/folder/{folderId}’일 경우 폴더 목록에서 {folderId} 에 해당하는 폴더가 선택되어 있고 폴더에 있는 링크들을 볼 수 있게 해주세요.
  • 폴더 페이지에서 현재 유저의 폴더 목록 데이터를 받아올 때 ‘/api/folders’를 활용해 주세요.
  • 폴더 페이지에서 전체 링크 데이터를 받아올 때 ‘/api/links’, 특정 폴더의 링크를 받아올 때 ‘/api/links?folderId={folderId}’를 활용해 주세요.
  • 유효한 access token이 있는 경우 ‘/api/users’로 현재 로그인한 유저 정보를 받아 상단 네비게이션 유저 프로필을 보이게 해주세요.

심화

  • 리퀘스트 헤더에 인증 토큰을 첨부할 때 axios interceptors 또는 이와 유사한 기능을 활용해 주세요.

주요 변경사항

  • 멘토링 때 말씀해 주셨던 로그아웃 기능을 추가하였습니다.

스크린샷

image
folder
shared

멘토에게

  • 저번 주 PR 피드백 부분 중 모달 관련된 부분은 아직 수정하지 못했습니다.
  • 다이나믹 라우팅을 사용할 때 겹치는 코드가 많을 때 어떻게 해야할 지 궁금합니다.
  • 버튼 탭 클릭 기능 중 전체 버튼 클릭 후 다른 버튼 탭 클릭 시 깜빡이는 현상을 고치고 싶은데 어떤 식으로 고치면 될 지 궁금합니다.
  • 셀프 코드 리뷰를 통해 질문 이어가겠습니다.
  • 감사합니다 :)

@HMRyu HMRyu requested a review from o-seung-yeon May 25, 2024 15:42
@HMRyu HMRyu added the 순한맛🐑 마음이 많이 여립니다.. label May 25, 2024
Copy link
Collaborator

@o-seung-yeon o-seung-yeon left a 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]);
Copy link
Collaborator

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 / 있으면 유저 정보 리턴하도록 수정하면 어떨까요?

Comment on lines +61 to +64
if (!localStorage.getItem("accessToken")) {
alert("로그인이 필요합니다.");
router.push("/signin");
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

로직과 ui 가 섞여있는 것보다 ui 는 선언형으로 컴포넌트 함수 최하단에서 리턴하고, 요런 로직은 컴포넌트가 마운트 됐을 때 처리되도록 effect 에 두어 실행 시점을 명확히 해주는 것이 코드를 파악하는데 혼란이 덜 할 것 같습니다~

Copy link
Collaborator

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

Comment on lines +7 to +18
axiosInstance.interceptors.request.use(
(config) => {
const token = localStorage.getItem("accessToken");
if (token) {
config.headers.Authorization = token;
}
return config;
},
(error) => {
return Promise.reject(error);
}
);
Copy link
Collaborator

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;
Copy link
Collaborator

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

Comment on lines +130 to +131
setClickedButton(folderIdFromURL);
setFolderId(folderIdFromURL);
Copy link
Collaborator

Choose a reason for hiding this comment

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

동일한 값을 두개의 상태로 관리하시는 이유가 있을까요?

Comment on lines +30 to +31
handleButtonClick: (folderId: number) => void;
handleAllButtonClick: () => void;
Copy link
Collaborator

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;
Copy link
Collaborator

Choose a reason for hiding this comment

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

folders 타입으로 세가지를 가지고 있는데 각각 값에 따라서 다른 처리를 해야하나용?

그게 아니라면

  1. folders: Folder[] (외부에서 props 값 넘겨줄 때 undefined 나 null 이면 빈 배열 전달하도록 기본값 설정)
  2. folders?: Folder[] (없을 수 있다는 뜻)
  3. folders: Folder[] | null (없을 수 있다는 뜻2, undefined 와 Null 중에선 한번 고민해보세요)

요렇게 세가지 중에 folders 값에 따라서 처리하는게 어떻게 달라지는지를 생각하고 타입을 명시해주시면 더 명확한 인터페이스가 될 것 같아요!

@@ -29,15 +30,17 @@ export default function LinkCardByFolderId({
setShowPopover(!showPopover);
};

console.log(link);
Copy link
Collaborator

Choose a reason for hiding this comment

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

로그 지워주세용

Comment on lines +73 to +84
{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>
Copy link
Collaborator

Choose a reason for hiding this comment

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

팝업 형태로 구현해주셨네요! 👍

Copy link
Collaborator

Choose a reason for hiding this comment

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

관련 로직을 커스텀훅으로 분리해줘도 좋을 것 같아요!

@o-seung-yeon o-seung-yeon merged commit be1702b into codeit-bootcamp-frontend:part3-유호민 May 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
순한맛🐑 마음이 많이 여립니다..
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants