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

[FE] Refactor/#642: 북마크 및 전체보기 페이지 API 로직 수정 및 React-Query 적용 #644

Merged
merged 21 commits into from
Jan 21, 2024

Conversation

semnil5202
Copy link
Collaborator

@semnil5202 semnil5202 commented Jan 13, 2024

작업 대상

  • 각 항목별 전체보기 페이지 (SeeAll ~)
  • 즐겨찾기 페이지

📄 작업 내용

  • 각 페이지 Axios 및 React Query 적용
  • 스켈레톤 UI 수정
  • 혼동 방지를 위해 TopicCardList 컴포넌트 제거
  • 리프레쉬 토큰 Axios 인터셉터로 적용

🙋🏻 주의 사항

  • Dynamic Import를 해제하면서 Suspense가 작동하지 않아 isLoading 상태를 사용하였습니다. 아이크가 제시해준 리액트 쿼리의 suspense 확인 후 적용해보겠습니다.
  • 리프레쉬 토큰 관련 로직 작업 수정중입니다.
  • SeeAll ~ 전체보기 페이지 동적 라우팅 적용과 리액트 라우터 돔의 loader 방식 고려중입니다.
  • TopicCardContainer랑 TopicCardList가 사실상 동일 기능이나 props가 달라서 구분된걸로 확인됩니다. 혼동 방지를 위해서 TopicCardList는 제거하겠습니다.

스크린샷

📎 관련 이슈

close #642

레퍼런스

@semnil5202 semnil5202 added FE 프론트엔드 관련 이슈 refactor 리팩토링 관련 이슈 labels Jan 13, 2024
@semnil5202 semnil5202 self-assigned this Jan 13, 2024
@semnil5202 semnil5202 marked this pull request as ready for review January 15, 2024 23:04
Copy link
Collaborator

@GC-Park GC-Park left a comment

Choose a reason for hiding this comment

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

'세인은 리팩토링을 좋아하고 참 잘한다'라고 항상 생각했었는데..

허허 이번에도 정말 좋군요! 많이 배웁니다..! 🙏

Comment on lines 4 to 12
export const getBookmarks = () =>
http.get<TopicCardProps[]>('/members/my/bookmarks');

export const getNewestTopics = () =>
http.get<TopicCardProps[]>('/topics/newest');

export const getAllTopics = () => http.get<TopicCardProps[]>('/topics');

export const getBestTopics = () => http.get<TopicCardProps[]>('/topics/bests');
Copy link
Collaborator

Choose a reason for hiding this comment

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

아하 어차피 각각 분리가 되어있으니 바로 url을 넣어주신 거군요! 너무 좋습니닷 👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

네네 각 요청 함수에 맞는 React Query 훅을 작성할 계획이니 url을 파라미터로 받아올 환경이 아닐테고 그에 따라서 url을 직접 지정해주었습니다.

Copy link
Collaborator

Choose a reason for hiding this comment

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

스켈레톤 변경 좋습네다~! 😄

<App />
</ErrorBoundary>
</ThemeProvider>,
<QueryClientProvider client={queryClient}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

헉?... Tanstack query를 쓰려면 'QueryClientProvider 컴포넌트로 감싸주고 QueryClient 값을 Props로 넣어줘야 합니다'를 발견했습니다... 저 잘못 사용했군요 얼른 고쳐야겠네여..~ 허허허헣

Comment on lines 27 to 30
const { isLoading, bookmarks: topics } = useGetBookmarks();

if (isLoading)
return (
Copy link
Collaborator

Choose a reason for hiding this comment

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

이 부분이 저번에 아이크가 말했던 suspense로 바꿀 수 있는 부분일까요?!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

그런 것 같습니다 공식문서에 의하면 suspense option을 true로 바꾸면 서스펜스 모드를 사용할 수 있다고 안내하네요.

status 값과 연관이 깊은것 같은데 확인후 Suspense 컴포넌트 사용해서 선언적으로 처리해보겠습니다.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

https://tanstack.com/query/v4/docs/react/reference/useQuery

위 링크에서 suspense 검색해보시면 나올거에용

</Flex>
</Flex>
<Box>
<SkeletonBox width="100%" $maxWidth={212} ratio="1.6 / 1" />
Copy link
Collaborator

Choose a reason for hiding this comment

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

styled component로 만들었던 SkeletonImg나 SkeletonTitle을 효율적으로 쓸 수 있도록 SkeletonBox를 만들었군요 아주 멋집니다 👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

스켈레톤 한 번 싹 정리해볼 필요가 있겠네용. 예전 디자인이랑 많이 달라져서..

Comment on lines 52 to 58
<TopicCardList
url="/members/my/bookmarks"
errorMessage="로그인 후 이용해주세요."
commentWhenEmpty="버튼으로 지도를 즐겨찾기에 담아보세요."
pageCommentWhenEmpty="메인페이지로 가기"
routePage={goToHome}
>
Copy link
Collaborator

Choose a reason for hiding this comment

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

오호.. 혹시 TopicCardList를 다른 걸로 대체한 이유가 있으실까요?!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

TopicCardContainer와 기능상 별 차이가 없는데 props는 또 달라서 코드를 분석하는데 헷갈릴 가능성이 있다고 판단하여 제거할 생각입니다.

이 컴포넌트는 실제로 데이터를 페칭하지도 않고 정말 단순히 TopicCard를 감싸는 용도라.. 과연 분리하는게 의미가 있을지 고민입니다. 패트릭의 생각은 어떠한가요?

완전히 삭제하지 않은 이유는 충돌 우려때문인데 확인 후 완전히 제거가 가능하면 제거하도록 하겠습니다.

운영서버에 머지해야 확인가능할 것 같습니다.
기존 로직을 사용하는 운영서버와 request 값은 동일한데 cookie와 도메인 설정 문제로 실패하는 것으로 확인됩니다.
* 마운트 시 리페칭 해제
* 윈도우 포커스 시 리페칭 해제
* 일정 주기로 리페칭 해제
* 받아온 데이터 stale 시 리페칭 해제
Copy link
Collaborator

@jiwonh423 jiwonh423 left a comment

Choose a reason for hiding this comment

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

확인했습니다~


const API_POSTFIX = 'api';
const BASE_URL = process.env.APP_URL || `https://mapbefine.com/${API_POSTFIX}`;

const axiosInstance = axios.create({
baseURL: BASE_URL,
headers: { Authorization: `Bearer ${localStorage.getItem('userToken')}` },
Copy link
Collaborator

Choose a reason for hiding this comment

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

const axiosInstance = axios.create({
  baseURL: BASE_URL,
  headers: token ? { Authorization: `Bearer ${token}` } : {},

헤더에 저렇게 안해주면 제가 맡았던 home페이지에서 "Bearer "이게 Authorization에 무조건 들어가서 에러나는것 같은데 잘 동작하나요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

예 어제 오류나서 바꿔주었습니다 ㅋㅋㅋㅋ


import { getAllTopics } from '../../apis/new';

const useGetAllTopics = () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

useQuery를 useSuspenseQuery로 바꾸면 SeeAllTopics.tsx에서 isloading 상태를 신경안쓰고 Suspense fallback으로 관리하면 될꺼같은데 카톡에서 말한 내용이 이부분인가요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

뎃츠롸잇~ 제가 푸쉬를 안했군요

semnil5202 and others added 3 commits January 18, 2024 01:33
* TopicCardList 컴포넌트를 이전처럼 활용하도록 한다. 전체보기 및 즐겨찾기는 거의 동일한 형태이며 중복코드가 다량 발생하여 위와 같이 수정한다.
* url을 넘겨받음에 따라서 리액트 쿼리 훅, API 요청 로직 또한 하나의 훅으로 재사용한다.
@jiwonh423 jiwonh423 merged commit f7d8897 into develop-FE Jan 21, 2024
1 check failed
Copy link
Collaborator

@GC-Park GC-Park left a comment

Choose a reason for hiding this comment

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

머지 굿~!

@semnil5202 semnil5202 deleted the refactor/#642 branch February 9, 2024 23:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FE 프론트엔드 관련 이슈 refactor 리팩토링 관련 이슈
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants