-
Notifications
You must be signed in to change notification settings - Fork 6
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
[FE] Refactor/#643: Profile 페이지 API 로직 수정 및 React-Query 적용 #647
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.
리팩토링 하시느라 고생많으셨습니다 패트릭 👍
궁금한 부분이 몇 군데 있어서 리뷰 남겼습니다. 바로 어프로브를 드릴려고 했는데 서버 상태를 useState로 재할당 하는 부분이 있어서 한 번 의견을 듣고 싶어서 코멘트로 드리겠습니다.
TopicCardList가 좀 머리 아픈 구조로 되어있죠? 리뷰에도 적었지만 저는 이 컴포넌트를 제거할까.. 생각중입니다. 편하게 의견 주시면 감사하겠습니다.
아무튼 고생많으셨습니다. 👍
|
||
export const http: HttpClient = axiosInstance; | ||
|
||
// axios.interceptors.request.use() |
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.
하핳..! 전 아이크만 바라보고 있습니다 그저 빛.. 🔦
frontend/src/apis/Patrick/index.ts
Outdated
import { TopicCardProps } from '../../types/Topic'; | ||
|
||
export const getProfile = (url: string) => { | ||
return http.get<TopicCardProps[] | null>(url); |
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.
url을 매개변수로 받아오는 이유가 있을까요?? getProfile 이라고 한다면 프로파일에 관련된 정보를 받아올 것 같고 반환 타입을 TopicCardProps[] 라고 명시해주었으니 재사용이 어려울 듯한데, url을 바로 적지 않고 외부에서 받아오도록 한 이유가 있나용?
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.
오 이 부분에 대해서 세인 리뷰에 달았는데 저도 동의합니다! 어차피 분리가 되어 다른 곳에서 사용하지 않을 것이니 바로 url을 적으면 될 것 같습니다! 구우웃!
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.
제가 했던 getTopics는 홈화면에서 비슷하게 3번사용되서 이렇게 url을 넘겨받는 방법으로 했는데 이 부분은 한번만 쓰이는건가보내요
@@ -29,11 +30,12 @@ function TopicCardList({ | |||
}: TopicCardListProps) { | |||
const [topics, setTopics] = useState<TopicCardProps[] | null>(null); | |||
const { fetchGet } = useGet(); | |||
const { data } = useProfileList(url); |
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.
TopicCardList 내부에서 사용되서 url을 밖에서 받아오신거군요. 이 컴포넌트가 제가 맡은 페이지에서만 사용되는 줄 알았는데 아니였군요. 개인적으로 이 컴포넌트는 TopicCardContainer랑 모습이 동일해서 제거할 생각인데.. 패트릭은 어떻게 생각하시나요?
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.
오호 확인해보니 비슷한데 스와이프가 적용된 것이군요!
이 부분 좀 있다가 회의 때 한번 더 말해보면 좋을 것 같습니다! 이게 한번에 보는거랑 스와이프로 보는 것 중 선택해서 수정만 하면 될 것 같아요! 굿굿
setTopics(response); | ||
}); | ||
if (data !== undefined) { | ||
setTopics(data); |
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.
서버 상태의 data를 직접 사용하지 않고 setTopics를 통해 useState 상태로 다시 할당한 이유가 있나용?
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.
아하! 역시 이래서 리뷰가 필요한가 봅니다 바로 컨펌 후 수정하도록 하겠습니다!! 👍
queryFn: async () => { | ||
const data = await getProfile(url); | ||
return data; |
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.
getProfile
로 넘기지 않고 async 함수를 한 번 감싼 이유가 url 인자 때문일까요?
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.
쿼리쓰면 요기 async await없어도 되는거같던데???
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.
저랑 거의 똑같네엽
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.
수정해주신거 모두 확인하였습니다. 굳굳 고생많으셨습니다 👍
작업 대상
📄 작업 내용
🙋🏻 주의 사항
스크린샷
📎 관련 이슈
#643
레퍼런스