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

[김창민] week20 #1076

Open
wants to merge 10 commits into
base: part3-김창민
Choose a base branch
from

Conversation

changmin6362
Copy link
Collaborator

@changmin6362 changmin6362 commented May 12, 2024

요구사항

기본

  • 변경된 api를 활용해 주세요.

  • 모달에 필요한 api 요청을 만들어 기능을 완성해 주세요.

  • api 요청에 TanStack React Query를 활용해 주세요.

  • Github에 위클리 미션 PR을 만들어 주세요.

  • React, Next.js를 사용해 진행합니다.

심화

  • [x]
  • []

주요 변경사항

  • 기존 api 요청을 리액트 쿼리를 사용하도록 변경하기
  • 커스텀 훅으로 만들어서 하나의 파일로 관리하기

스크린샷

image

멘토에게

  • 제가 템플릿 코드를 기반으로 코드를 변경해봤는데, 기존 코드에서 리액트 훅 폼을 사용한 부분을 이해하지 못해서 리액트 훅폼을 통해 사용되는 api 호출 코드는 변경하지 못 했습니다. 제 메시지를 보고 답변해주시면 감사할 것 같습니다.

커밋이 많지만 제일 마지막 커밋만 제가 작업한 파일입니다

Copy link

vercel bot commented May 12, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
4-weekly-mission ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 12, 2024 9:45am
4-weekly-mission-ursd ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 12, 2024 9:45am

@changmin6362 changmin6362 self-assigned this May 12, 2024
@changmin6362 changmin6362 changed the title [week20] 김창민 [김창민] week20 May 12, 2024
@changmin6362 changmin6362 added 매운맛🔥 뒤는 없습니다. 그냥 필터 없이 말해주세요. 책임은 제가 집니다. 미완성 죄송합니다.. labels May 12, 2024
@changmin6362 changmin6362 requested a review from domuk-k May 15, 2024 13:21
Copy link
Collaborator

@domuk-k domuk-k left a comment

Choose a reason for hiding this comment

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

고생하셨습니다.

  • api 동작이 약간 다를 수 있으니 확인해보시면 좋겠네요
  • react-hook-form 사용된 부분도 usePostSignup/usePostSignin으로 잘 대체해주신걸로 보여요
    • react-hook-form의 handleSubmit에 바로 전달하는건 타입에러가 있었을 텐데, 그 이유도 알고가시면 좋겠습니다

});
}

export function useGetUser() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

템플릿 코드에서 중요한차이가 userId를 인자로 받는지 여부군요.
use get user 기 때문에, 특정 user를 조회하는 기능을 가진 훅인데, userId를 받지 않는 다는 것만으로도 혼동을 줄만한 상황입니다. 템플릿코드를 참고해주세요. 아래라인에서 좀더 코멘트드릴게요

Comment on lines +75 to +85
const data = responseData?.[0] && {
id: responseData?.[0].id,
name: responseData?.[0].name,
email: responseData?.[0].email,
imageSource: responseData?.[0].image_source,
};
localStorage.setItem("userId", data.id);

return data;
},
enabled: !!token,
Copy link
Collaborator

Choose a reason for hiding this comment

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

기존 코드에서처럼, !!userId가 추가되어야 react-query로 안정적으로 마이그레이션했다고 할 수 있을거같아요. 지금은 동작이 달라질걸로 보입니다. userId 없이 유저목록을 조회하고 그 중에 언제나 첫번째 유저를 반환할거라서요

enabled: !!token && !!userId

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