feat: add pagination#1523
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because an important functional requirement from the task is still missing: the app does not use React Router to sync page and perPage with the URL query parameters (e.g. ?page=2&perPage=7), nor does it initialize state from those query params on load. This means that pagination state is not preserved in the URL, which is explicitly required and can affect both navigation and test expectations. To fix this, update App.tsx to use React Router hooks (for example, useSearchParams) to read initial page and perPage from the URL, and then update the URL whenever page or perPage changes.
There are also a couple of smaller but still requirement-related issues. In both src/App.tsx and src/components/Pagination/Pagination.tsx, the Props interface marks currentPage as required, but the task states it should be optional with a default of 1; update the type to currentPage?: number so callers are not forced to pass it. Additionally, in Pagination, page link clicks currently call onPageChange even when clicking the already active page; add a guard so onPageChange is only fired when num !== currentPage, matching the specification. Once you address the React Router integration and these typing/behavior adjustments, your solution will fully meet the task requirements—your existing pagination logic and UI are otherwise well implemented.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
| import { Pagination } from './components/Pagination'; | ||
| import { getNumbers } from './utils'; | ||
|
|
||
| // eslint-disable-next-line @typescript-eslint/no-unused-vars | ||
| const items = getNumbers(1, 42).map(n => `Item ${n}`); | ||
| enum PerPage { |
There was a problem hiding this comment.
The Props interface marks currentPage as required, but the task states currentPage is optional with a default of 1. Make this prop optional (currentPage?: number) to match the requirements and your default parameter value.
| interface Props { | ||
| total: number; | ||
| perPage: number; | ||
| currentPage: number; |
There was a problem hiding this comment.
The Props interface marks currentPage as a required prop, but the requirements specify that currentPage should be optional with a default of 1. Consider updating the type to make currentPage optional (e.g. currentPage?: number) so callers are not forced to pass it.
| const totalPages: number = Math.ceil(total / perPage); | ||
| const pages: number[] = Array.from({ length: totalPages }, (_, i) => i + 1); | ||
|
|
||
| return ( | ||
| <ul className="pagination"> | ||
| <li | ||
| className={cn('page-item', { | ||
| disabled: currentPage <= 1, | ||
| })} | ||
| > | ||
| <a | ||
| data-cy="prevLink" | ||
| className={cn('page-link')} | ||
| href="#prev" | ||
| aria-disabled={currentPage === 1} | ||
| onClick={e => { | ||
| e.preventDefault(); | ||
| if (currentPage > 1) { | ||
| onPageChange(currentPage - 1); | ||
| } | ||
| }} | ||
| > | ||
| « | ||
| </a> | ||
| </li> | ||
| {pages.map(num => { | ||
| return ( | ||
| <li | ||
| key={num} | ||
| className={cn('page-item', { | ||
| active: currentPage === num, | ||
| })} | ||
| > | ||
| <a | ||
| data-cy="pageLink" | ||
| className={cn('page-link', { | ||
| active: currentPage === num, | ||
| })} | ||
| href={`#${num}`} | ||
| onClick={e => { | ||
| e.preventDefault(); | ||
| onPageChange(num); | ||
| }} | ||
| > | ||
| {num} | ||
| </a> | ||
| </li> | ||
| ); | ||
| })} |
There was a problem hiding this comment.
The App manages pagination state correctly, but there is no use of React Router to sync page and perPage with the URL query parameters as required (e.g. ?page=2&perPage=7). You need to integrate React Router hooks here to read initial values from the URL on load and update the URL when page or perPage change.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your solution is approved because it now implements all of the required pagination and URL-sync behavior without any critical functional issues. The Pagination component is correctly stateless, accepts total, perPage, optional currentPage, and onPageChange, and renders the full page list with the right data-cy attributes and li.active/li.disabled states. Prev/next navigation behaves correctly at the boundaries, with aria-disabled="true" applied when appropriate, and onPageChange is only fired when the target page actually differs from the current page. In App, you correctly compute visible items, wire onPageChange to useSearchParams, and keep page and perPage synchronized with the URL while initializing from the query params on load. The info text and per-page selector (3/5/10/20) match the specified format and behavior, including resetting the page to 1 when perPage changes. There are no remaining critical blockers or missing features relative to the task description, so you can move forward confidently with this implementation. Nice work tightening up the edge cases and keeping the pagination component clean and stateless.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
DEMO LINK