fewsgarhghihfOG#1522
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 several core requirements are not yet met and they currently break the specified behavior and tests. The pagination state is managed only in React state and not synchronized with the URL, but the task explicitly requires using React Router to store and read ?page=...&perPage=... from the query string; this is a missing feature, not just a nice-to-have. In Pagination.tsx, onPageChange is still called when the prev/next buttons are disabled (isPrevDisabled/isNextDisabled) and when clicking the already active page; this violates the requirement that the callback should run only if the page actually changes and can lead to out-of-range pages (0 or totalPages + 1). You need to guard these handlers so they early-return when the target page equals currentPage or falls outside the valid range of 1 to totalPages. In App.tsx, changing perPage updates the items per page but does not reset currentPage to 1, which contradicts the requirement to always show the first page after a per-page change and is the source of some failing tests; add logic in the per-page onChange handler to set both perPage and currentPage = 1 and reflect this in the URL once Router is integrated. After you implement Router-based URL syncing, fix the onPageChange guards, and reset the current page on perPage change, your solution should align with the task description and pass the tests. You’ve already done a solid job on the stateless pagination, item slicing, and HTML structure, so focus on these behavioral and routing fixes to complete the task.
✨ 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
| onClick={e => { | ||
| e.preventDefault(); | ||
| onPageChange(currentPage - 1); |
There was a problem hiding this comment.
onPageChange is called even when isPrevDisabled is true (currentPage is 1). According to the requirements, the callback should only run if the page actually changes, so you should guard against calling onPageChange when the prev link is disabled.
| {pages.map(page => ( | ||
| <li | ||
| key={page} | ||
| className={`page-item ${page === currentPage ? 'active' : ''}`} | ||
| > | ||
| <a | ||
| data-cy="pageLink" | ||
| className="page-link" | ||
| href={`#${page}`} | ||
| onClick={e => { | ||
| e.preventDefault(); | ||
| onPageChange(page); |
There was a problem hiding this comment.
Similarly, clicking on the currently active page still calls onPageChange(page). The task requires that onPageChange is only triggered when the page changes, so add a condition to avoid calling it when page === currentPage.
| onClick={e => { | ||
| e.preventDefault(); | ||
| onPageChange(currentPage + 1); |
There was a problem hiding this comment.
onPageChange is also invoked when isNextDisabled is true (currentPage is the last page). You need to prevent this call when on the last page to satisfy the requirement that the callback fires only on actual page changes.
| export const Pagination: React.FC<Props> = ({ | ||
| total, | ||
| perPage, | ||
| currentPage = 1, | ||
| onPageChange, | ||
| }) => { | ||
| const totalPages = Math.ceil(total / perPage); | ||
| const pages = Array.from({ length: totalPages }, (_, i) => i + 1); | ||
|
|
||
| const isPrevDisabled = currentPage === 1; | ||
| const isNextDisabled = currentPage === totalPages; | ||
|
|
||
| return ( | ||
| <ul className="pagination"> | ||
| <li className={`page-item ${isPrevDisabled ? 'disabled' : ''}`}> | ||
| <a | ||
| data-cy="prevLink" | ||
| className="page-link" | ||
| href="#prev" | ||
| aria-disabled={isPrevDisabled} | ||
| onClick={e => { | ||
| e.preventDefault(); | ||
| onPageChange(currentPage - 1); | ||
| }} | ||
| > | ||
| « | ||
| </a> | ||
| </li> | ||
|
|
||
| {pages.map(page => ( | ||
| <li | ||
| key={page} | ||
| className={`page-item ${page === currentPage ? 'active' : ''}`} | ||
| > | ||
| <a | ||
| data-cy="pageLink" | ||
| className="page-link" | ||
| href={`#${page}`} | ||
| onClick={e => { | ||
| e.preventDefault(); | ||
| onPageChange(page); | ||
| }} | ||
| > |
There was a problem hiding this comment.
The app’s pagination state is managed entirely in React state and is not synchronized with the URL. The description requires using React Router to store ?page=...&perPage=... in the URL and initialize/apply these values on page load, which is currently missing.
| aria-disabled={isPrevDisabled} | ||
| onClick={e => { | ||
| e.preventDefault(); | ||
| onPageChange(currentPage - 1); | ||
| }} | ||
| > |
There was a problem hiding this comment.
Changing perPage updates the state but does not reset currentPage to 1, which the requirements specify should happen after changing perPage. You should also update currentPage to 1 in the onChange handler.
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 correctly implements all of the required pagination behavior and URL synchronization without any critical functional issues. The Pagination component is properly stateless, takes total, perPage, and an optional currentPage, and uses onPageChange only when the page actually changes and stays within valid bounds. You correctly render all pages, highlight the active one with li.active, and disable prev/next at the boundaries using li.disabled and aria-disabled, while still preserving the required data-cy attributes. In App.tsx, items are sliced according to the current page and perPage, the info string matches the exact required format (Page X (items A - B of 42)), and the per-page selector includes the specified options and resets the page to 1 on change. React Router is integrated properly with BrowserRouter and useSearchParams, keeping page and perPage in the query string and applying them on load, which meets the routing-related requirements. Any remaining points are purely stylistic and not blockers, so you’ve met the task’s functional and behavioral standards—well done.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
No description provided.