-
Notifications
You must be signed in to change notification settings - Fork 2
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
[refactor/#193/Pagination] 페이지네이션 리팩토링 #196
base: dev
Are you sure you want to change the base?
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.
리펙토링 고생하셨습니다!
프로젝트 끝나고도 계속 열심히 하시는 모습 본받겠습니다!
저는 거의 풀어져서 깔짝깔짝하고 있는데
200줄 가까이 리펙토링은 ㄷㄷ
분발하겠습니다!
const searchParams = new URLSearchParams(location.search) | ||
const paramsValue = paramsName && searchParams.get(paramsName) |
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.
react-router-dom
의 useSearchParams
를 쓰는게 이후에 변경되는 값들을 제대로 반영할 수 있을 것 같은데 어떻게 생각하시나요?
승민님도 검색 페이지를 URLSearchParams
로 query string을 가져왔었는데 컴포넌트 외부에서 query string을 변경하면 그 컴포넌트는 해당 query string이 바뀌었는지 인식하지 못하는 버그가 발생하더라구요...
(헤더에서 검색 => 검색 페이지는 바뀐 query string 인식 못함)
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.
리뷰를 늦게 드렸네요..
querystring
기반으로 페이지네이션을 구현하는게 어려우셨을텐데 잘 구현해주신 거 같아요. 수고하셨습니다!
@@ -18,6 +18,7 @@ export const useUserProjects = ({ | |||
const { data } = useQuery({ | |||
queryKey: ["projects", userId, type, page, size], | |||
queryFn: () => getUserProjects({ userId, type, page, size }), | |||
staleTime: 1000 * 60 * 5, |
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.
저도 staletime을 적용하는 것을 까먹고있었는데 프로젝트 끝나고 나서야 알았네요.. default가 0이라서 되도록 설정해주면 좋다고 생각했는데 꼼꼼히 설정해주셨네요!!
totalProjectsCount: number | ||
setPage: React.Dispatch<React.SetStateAction<number>> | ||
} | ||
const PaginationRQ = ({ |
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.
ASK : 컴포넌트명의 RQ는 Request의 약자인가요?
@@ -0,0 +1,13 @@ | |||
import { useLocation, useNavigate } from "react-router-dom" | |||
|
|||
const useQueryString = (paramsName: string) => { |
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.
P2 : hook에 맞게 기본 내보내기가 아니라 export const
내보내기로 수정해주시면 감사하겠습니다!
@@ -16,16 +21,41 @@ import ProjectsGrid from "./ProjectsGrid" | |||
const ProjectsView = ({ userId, isMe }: UserIdProps) => { | |||
const [isLargerThan500] = useMediaQuery("(min-width: 500px)") | |||
|
|||
// eslint-disable-next-line react-hooks/exhaustive-deps | |||
const projectsType = isMe ? ["JOINED", "LIKED", "COMMENTED"] : ["JOINED"] |
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.
ASK : usememo
대신에 lint warning을 무시하는 이유가 따로 있으신가요?
const projectsType = isMe ? ["JOINED", "LIKED", "COMMENTED"] : ["JOINED"] | ||
|
||
const { location, navigate, paramsValue: tabInfo } = useQueryString("tab") | ||
const { paramsValue: tempPageInfo } = useQueryString("page") |
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.
P3 : pageInfo
가 Number
로 변환되어 사용하는 곳이 여기서는 한 군데이므로 받을때 pageInfo
로 받고 전달할때 Number
를 씌워주는건 어떤가요? temp
가 없으면 더 좋을 것 같아서요
|
||
const useQueryString = (paramsName: string) => { | ||
const location = useLocation() | ||
const navigate = useNavigate() |
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.
P3 : navigate
를 반환하지만 사용하지 않는 곳도 존재하고, 선언후 바로 반환하는 부분이라 view
에서 선언하는것이 더 자연스럽지 않나 생각했는데 여기서 선언하신 이유가 있나요?
(projectType) => projectType === tabInfo, | ||
) | ||
setTabIndex(newIndex >= 0 ? newIndex : 0) | ||
}, [location, projectsType, tabInfo]) |
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.
P3 : location
을 의존성배열에 넣으신 이유가 있나요? projectsType
과 tabInfo
만으로도 해결이 될 거 같아서요! 훅에서 location
을 내려주지 않아도 된다고 생각하는데 어떤가요?
}, [location, projectsType, tabInfo]) | ||
|
||
const handleChangeTab = (index: number) => { | ||
const storedPage = tabPageInfo[projectsType[index]] |
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.
P2 : 뒤로가기 했을때 페이지 번호는 변하는데 화면이 그대로인 오류가 있어서 확인 한번 해주시면 감사하겠습니다!
#️⃣연관된 이슈
close #193
💡 핵심적으로 구현된 사항
➕ 그 외에 추가적으로 구현된 사항
🤔 테스트,검증 && 고민 사항
📌 PR Comment 작성 시 Prefix for Reviewers