-
Notifications
You must be signed in to change notification settings - Fork 0
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
[FEAT] Pagination 컴포넌트, 스토리북 구현 #22
base: main
Are you sure you want to change the base?
Conversation
Walkthrough이 풀 리퀘스트는 Pagination 컴포넌트를 구현하고 스토리북 스토리를 추가합니다. 컴포넌트는 현재 페이지와 총 페이지 수를 관리하며, 페이지 간 탐색을 위한 이전/다음 버튼과 페이지 번호 버튼을 포함합니다. 스토리북 스토리는 컴포넌트의 상호작용성을 시각화하고 테스트할 수 있도록 설계되었습니다. Changes
Assessment against linked issues
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Update: 2025년 01월 25일 03시 33분 27초 |
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.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/shared/ui/Pagination/Pagination.stories.tsx (1)
1-40
: 스토리북 구성에 대한 확장 제안현재
Basic
스토리만으로도 컴포넌트 동작을 확인하기에 충분합니다. 하지만 다양한 페이지 수나 큰 페이지 수 시나리오(예:totalPages=10
이상)도 함께 추가한다면, 구성 요소의 동작을 더 폭넓게 확인할 수 있습니다. 향후 스토리가 많아지면play
함수 등을 활용하여 시나리오 기반 테스트를 확장해 보시는 것도 좋겠습니다.src/shared/ui/Pagination/Pagination.tsx (2)
38-49
: PaginationItem 컴포넌트의 onClickCapture 사용
onClickCapture
를 사용해도 문제는 없지만, 일반적으로는onClick
을 사용하는 경우가 많습니다. 특별히 캡처 단계에서 처리해야 할 로직이 아니라면onClick
으로 교체하는 편이 더 직관적일 수 있습니다.
51-68
: Props 타입 설계 및 주석 처리
currentPage
,totalPages
,onPageChange
등 핵심 props가 잘 정의되어 있습니다.- 향후 페이지 외에
pageSize
나 기타 설정이 필요해질 수도 있으니, 구조가 확장 가능하도록 잘 설계된 것 같습니다.- 주석을 꼼꼼히 달아둬서 문서화에도 도움이 됩니다.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
src/shared/ui/assets/arrow-left.svg
is excluded by!**/*.svg
src/shared/ui/assets/arrow-right.svg
is excluded by!**/*.svg
📒 Files selected for processing (4)
src/shared/ui/Pagination/Pagination.stories.tsx
(1 hunks)src/shared/ui/Pagination/Pagination.tsx
(1 hunks)src/shared/ui/Pagination/index.ts
(1 hunks)src/shared/ui/assets/index.tsx
(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/shared/ui/Pagination/index.ts
🔇 Additional comments (4)
src/shared/ui/assets/index.tsx (2)
3-4
: 아이콘 SVG import에 대한 간단한 확인새로운
arrow-left.svg
,arrow-right.svg
아이콘을 잘 가져오셨습니다. 다른 아이콘들과 네이밍 패턴이 일관적이며 문제없이 사용 가능해 보입니다.
22-23
: Icons 객체에 화살표 아이콘 추가
arrowLeft
와arrowRight
속성이Icons
객체에 정상적으로 등록되었습니다. 다른 아이콘들과 함께 정상적으로 활용 가능할 것으로 보입니다.src/shared/ui/Pagination/Pagination.tsx (2)
5-21
: PaginationWrapper 컴포넌트 구조 확인
nav
요소에role="navigation"
과aria-label="pagination"
을 부여한 점이 접근성 측면에서 훌륭합니다.className
과 나머지 HTML 속성을 모두 전달해주는 구조로 확장성도 뛰어납니다.
전반적으로 구조가 명확해 보이므로 추가 보완 없이 괜찮습니다.
23-36
: PaginationItemProps 명세 확인
page
,isActive
등 필요한 속성들을 명확히 기술하여 가독성이 좋습니다.- 주석으로 필드의 용도를 설명한 점도 코드 유지보수에 큰 도움이 됩니다.
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.
수고하셨습니다 👍 혹시 컴포넌트 작성 시, 파일분리를 결정하시는 기준이 있으신가용?
name="arrowRight" | ||
size={35} | ||
className="size-9 rounded-md px-2 py-1 hover:bg-gray-100" | ||
onClickCapture={handleNext} |
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.
저번 pagination구현 시 남긴 리뷰와 동일하지만, 현재는 최대로 커버할 수 있는 page가 5개이네용. 피드에서도 무한스크롤을 적용하셔서 현재는 우선순위가 떨어지는 것 같고, 저희 sprint 일정이 가능해지면 챙기면 좋아보여요
로직이 들어가거나, 여러 태그를 사용해야하는 복잡한 컴포넌트의 경우 파일을 분리하고 있어요. |
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.
totalPages가 9개 이상이 되면 아이콘이 안 뜨거나 숫자가 밀리는 등의 현상이 있는데
아직 페이지는 피그마 상 확장된 형태로 사용되는 부분이 없기에 확장이 필요해진다면 이 부분도 수정하면 좋을 것 같습니다
수고하셨습니다 !
🔥 연관 이슈
🚀 작업 내용
🤔 고민했던 내용
페이지의 목록
이라는 점을 고려하면 현재의button
태그 보다는ul
,li
태그를 활용하여 설계하는게 더 적합하다고 생각했어요. 다만, 현재 페이지 네이션이 적용되는 곳이 많지 않아 간단한 구조를 유지하는 것이 중요하다고 판단했어요. 이후 페이지네이션이 다양한 화면에서 사용되거나 기능 확장이 필요해진다면 ul, li 기반으로 재구성해 시맨틱한 구조와 확장성을 확보할 예정입니다!💬 리뷰 중점사항
Summary by CodeRabbit
새로운 기능
개선 사항
스토리북