-
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
[FE] feat: 리뷰 그룹 목록 조회 API 연동 및 MSW 핸들러 수정 #1090
base: develop
Are you sure you want to change the base?
Conversation
…ined-review-group-api
…ined-review-group-api
- apis/group.ts -> types
…up-api' into fe/feat/1080-reivew-link-page-api
…, BackButton 표시 방식 다르게 처리
gettingSectionList: `${serverUrl}/${VERSION2}/sections`, | ||
gettingGroupedReviews: (sectionId: number) => | ||
`${REVIEW_GROUP_API_URL}?${REVIEW_GROUP_API_PARAMS.queryString.sectionId}=${sectionId}`, | ||
postingHighlight: `${serverUrl}/${VERSION2}/highlight`, | ||
gettingReviewLinks: REVIEW_GROUP_DATA_API_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.
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이 같아서 저 상수를 사용했어요.
REVIEW_GROUP_DATA_API_URL
=== https://api.review-me.page/v2/groups
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.
쑤쑤 고생했어요.
굵직한 기능 위주로 코드를 봤어요. 스타일링 부분은 쑤쑤가 워낙 잘 하니 믿고 맡길게요 ㅎㅎㅎ
로그인 사용자 리뷰 링크 api url이 맞는 지 확인 부탁해요
refetch(); | ||
}; | ||
|
||
const handleReivewLinkItemClick = (reviewRequestCode: 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.
오타를 발견했어요 (Reivew -> Review)
const handleNewReviewLink = () => { | ||
refetch(); | ||
}; |
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.
리뷰 링크를 다시 불러오는 함수의 기능이 함수명에 담겼으면 좋겠어요. (예시: refetchReviewLinks)
리뷰 생성 폼에서 확인 시, 새로운 링크로 뭘한다는 거지?하고 의미 파악이 쉽지 않았어요.
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.
덧) refetch만 호출할 거라면 중괄호로 감싸지 않고 한 줄로 처리해도 좋을 것 같습니다~
}; | ||
|
||
// 새로 생성된 리뷰 링크를 목 데이터에 추가 | ||
REVIEW_LINKS.reviewGroups.push(newReviewLink); |
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.
대문자 스네이크는 코드 컨벤션에서 변하지 않는 상수를 의미하기 때문에, 배열을 변형하는 push 동작과 맞지 않아요.
리뷰 링크 생성에 따른, 리뷰 목록의 변경을 확인하고 싶어서 넣은 것 같아요.
그렇다면 ' REVIEW_LINKS'의 변수명을 바꿔야할 것 같아요.
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.
아....!!!! 맞네요... 꼼꼼하십니다!
수정하겠습니다!
|
||
// 비회원일 경우, revieweeId를 null로 변경 | ||
if (reviewRequestCode === nonMemberReviewRequestCode) { | ||
REVIEW_GROUP_DATA.revieweeId = null; |
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.
이것도 앞에서 언급한 이유로, 변수명을 바꿔야겠어요.
const url = `${window.origin}/${ROUTE.reviewZone}/${reviewRequestCode}`; | ||
|
||
return ( | ||
<S.Layout onClick={handleClick}> |
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.
Layout 태그가 div라서, 스크린 리더기를 사용하는 사용자 입장에서 클릭이 가능한지, 클릭하면 어떤 동작이 일어나는 지에 대한 정보가 없어요. 그리고 키보드 조작만으로는, 리뷰 카드 자체 클릭이 안돼요.
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.
상위 태그가 ul이라서 div -> li로 변경해야 할 것 같아요. 그리고 UndraggableWrapper
로 감싸보면 어떨까요?
observer.observe(element); | ||
|
||
return () => observer.disconnect(); | ||
}, []); |
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.
cardListRef.current 을 useEffect의 deps에 넣지 않아도 괜찮은가요?
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.
여러 가지 코멘트를 남겼지만ㅋㅋㅋ 당장 반영해야 할 것은 별로 없고 금방 할 수 있는 것들이라 가볍게 봐주시고, 확인 후에 재요청 주세요!
로그인 여부 처리가 까다로웠을 것 같은데 수고 많았어요~~~👍
export const REVIEW_EMPTY = { | ||
noReviewInTotal: '아직 받은 리뷰가 없어요!', | ||
noReviewInQuestion: '이 질문은 아직 받은 답변이 없어요!', | ||
noReviewInTotal: '아직 받은 리뷰가 없어요...', | ||
noReviewLink: '생성한 리뷰 링크가 없어요...', | ||
noReviewInQuestion: '이 질문은 아직 받은 답변이 없어요...', | ||
}; |
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.
(원래 있었던 상수긴 하지만) 이 값들을 상수로 관리할 필요가 있을까요?
각 페이지나 컴포넌트에 특화된 값들이고 재사용성도 딱히 고려할 필요 없을 것 같아서 공통 상수로 뺄 필요가 없다는 생각입니다. 또 당장 EmptyContent 컴포넌트도 안내 문구를 string으로만 받지 않고 children으로 받으니까요.
당장 수정하자는 건 아닙니다ㅎㅎ
import { REVIEW_QUERY_KEY } from '@/constants'; | ||
|
||
const useGetReviewLinks = () => { | ||
const result = useSuspenseQuery({ |
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.
오 리뷰 링크를 받아올 때 infiniteQuery를 쓸 줄 알았는데 일반 query를 쓰네요?!
서버에서 isLastPage나 lastReviewGroupId를 내려주길래 inf query를 쓸 줄 알았어요. 애초에 요청에 id를 보내지 않으니 무한 구현은 안 될 것 같긴 한데 왜 저 값들을 내려주는지는 궁금하네요...
export { default as useGetReviewList } from './useGetReviewList'; | ||
export { default as useGetReviewLinks } from './useGetReviewLinks'; |
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.
맥락을 까먹어서 그러는데, ReviewList 가져오는 훅(외에 한 페이지에서만 쓰는 쿼리 훅들)을 공통 훅으로 뺐던 이유가 뭐였는지 기억하시나요? depth 이슈였나...
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.
초반에 만든 훅이라 공통 훅 자리에 있는것 같아요
import useGetReviewLinks from '.'; | ||
|
||
describe('회원이 생성한 리뷰 그룹 목록 조회 테스트', () => { | ||
it('회원이 생성한 리뷰 그룹 목록을 성공적으로 불러온다.', async () => { |
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.
테스트까지 만들어주셨네요 👍
추후에 회원/비회원 쿠키를 나누고 그에 따라 테스트 유틸 함수도 나눠보면 좋겠네요!
// TODO: NavigationTab은 사용자가 홈 페이지와 연결 페이지가 아닌 다른 페이지에서 로그인 상태일 때만 보여준다. | ||
// 임시로 true 설정 (로그인 기능 추가하면서 여기도 수정해야 한다.) |
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.
주석 좋아요~~
<S.ReviewLink>{url}</S.ReviewLink> | ||
</S.ContentContainer> | ||
<S.Divider /> | ||
<RevieweeName revieweeTitle="🧑💻 리뷰이" revieweeName={revieweeName} /> |
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.
이모지는 임시용인가요?ㅎㅎ 리뷰어/리뷰이 이모지를 신중하게 골라서 모든 환경에서 동일해 보이도록 이미지 파일을 사용하자고 했던 것 같아서요. (제 PR에 png 파일이 있으니 두 페이지 모두 머지되면 수정해봅시다!)
useEffect(() => { | ||
const element = cardListRef.current; | ||
if (!element) return; | ||
|
||
const observer = new ResizeObserver(() => { | ||
requestAnimationFrame(() => { | ||
const newPadding = element.scrollHeight > element.clientHeight ? '1rem' : '0'; | ||
if (element.style.paddingRight !== newPadding) { | ||
element.style.paddingRight = newPadding; | ||
} | ||
}); | ||
}); | ||
|
||
observer.observe(element); | ||
|
||
return () => observer.disconnect(); | ||
}, []); |
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.
CSS의 scrollbar-gutter: stable
속성으로 대신할 수 있지 않을까요?
그리고 여기서는 requestAnimationFrame의 추가가 오히려 성능에 영향을 줄 수 있다고 해요.
ResizeObserver가 레이아웃 변경 직후 실행돼서 항상 최신 element 정보를 갖고올 수 있는데, 여기다requestAnimationFrame을 추가하면 이미 최신 값을 갖고 있음에도 다음 repaint가 일어날 때까지 1프레임을 더 기다린다고 하네요!
만약 이 로직을 사용한다면 ResizeObserver만으로 충분할 것 같습니다!
@@ -34,6 +29,6 @@ export const DetailedReviewContainer = styled.div` | |||
|
|||
export const ReviewContentContainer = styled.div` | |||
${media.xSmall} { | |||
padding: 0 2rem; | |||
padding: 0 1rem; | |||
} | |||
`; |
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.
DetailedReview 스타일을 수정한 이유가 있을까요?
작성한 리뷰 페이지에서 기존 DetailedReview를 재활용하면서 공통 컴포넌트로 뺐고, 그래서 제 PR에서는 DetailedReviewPage에서도 더 이상 기존 DetailedReviewPageContents 컴포넌트를 사용하지 않거든요. (다만 파일 삭제는 안 했습니다... ㅎㅎ 그래서 충돌은 안 날지도?) 그리고 반응형을 공통 DetailedReview 스타일을 보면서 작업해서 나중에 서로 생각하지 못한 결과물이 나올 수도 있을 것 같아요.
|
||
import NavItem from './NavItem'; | ||
import * as S from './styles'; | ||
export interface Tab { | ||
label: '리뷰 링크 관리' | '작성한 리뷰 확인'; |
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.
공통 컴포넌트라면 label에 올 수 있는 타입을 열어둬야 하지 않을까요?!
const isShowBreadCrumb = !isUserLoggedIn && isNeedBreadCrumb && breadcrumbPathList.length > 1; | ||
const isShowNavigationTab = isUserLoggedIn && pathname !== '/' && !pathname.includes(ROUTE.reviewZone); |
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.
렌더링 로직 빡세네요...ㅋㅋㅋㅋㅋ 개발자살려~!
🚀 어떤 기능을 구현했나요 ?
회원이 생성한 리뷰 그룹 목록
을 불러오는 API를 연동했습니다. 링크 생성 폼에서 새로운 링크를 생성하면 msw 환경에서도 이를 반영하도록 핸들러와 목 데이터를 수정했습니다.revieweeId
값이 달라지므로, 비회원일 경우revieweeId
가null
로 반환되는지 확인하는 테스트를 추가했습니다.NavigationTab
은 로그인 상태일 때만 표시되도록 처리했으며,Breadcrumb
은 로그인 상태에서는 보이지 않도록 수정했습니다.ReviewLinkItem
목록 페이지
와작성한 리뷰 확인 페이지
에서 사용하는 리뷰 아이템은 리뷰이가 포함되거나 키워드가 들어가는 등 차이점이 있어, 공통 컴포넌트로 재사용하기보다는리뷰 링크 페이지
에서 쓰이는ReviewLinkItem
컴포넌트를 별도로 구현했습니다.ResizeObserver
를 사용해ReviewLinkItem
을 감싸는 컴포넌트에 스크롤이 생기면padding-right: 2rem
추가했습니다.🔥 어떻게 해결했나요 ?
refetch()
를 실행해서 최신 데이터를 불러오도록 했으며 생성된 리뷰 링크가 오른쪽에ReviewLinkItem
으로 즉시 추가되도록 구현했습니다.2025-02-12.5.52.09.mov
리뷰가 없을 경우
isBorder
를 받게끔 수정했습니다.널~
이 아닌 돋보기 컴포넌트로 수정했습니다.📝 어떤 부분에 집중해서 리뷰해야 할까요?
📚 참고 자료, 할 말