Change 1#1516
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 there are a few critical behavioral issues that violate the explicit checklist and would likely break tests. Your Pagination correctly renders pages, highlights the active one, and wires up onPageChange, but it currently calls onPageChange even when the page does not actually change or when prev/next are visually disabled. Specifically, in src/App.tsx and src/components/Pagination/Pagination.tsx, the prev handler always calls onPageChange(currentPage - 1) (even when currentPage === 1), and the next handler always calls onPageChange(currentPage + 1) (even when currentPage === pages.length); both must early-return and not invoke the callback when at their respective boundaries to satisfy [CHECKLIST ITEM #5, #7, #9, #10]. Similarly, when clicking a page number, your code always calls onPageChange(page) even if page === currentPage; you need a guard like if (page !== currentPage) { onPageChange(page); } so the callback only fires when the page changes [CHECKLIST ITEM #5].
There is also a TypeScript correctness issue in src/components/Pagination/Pagination.tsx: the RenderingPages interface defines a property item, but renderPerPage destructures partOfPage, which will cause a type error; align the interface and function parameter to use the same property name. These issues are critical because they can cause invalid page indices (e.g., going to page 0 or beyond the last page) and violate the requirement that disabled controls must not trigger navigation. Once you add the appropriate guards around onPageChange and fix the interface/parameter name mismatch, the rest of your implementation looks solid and should meet the task requirements. Keep going—your overall structure is good, and these are focused behavioral fixes that will significantly improve robustness.
✨ 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
| setPages(+event.target.value); | ||
| setCount(1); | ||
| }; | ||
|
|
||
| const pageChange = (num: number) => setCount(num); | ||
|
|
||
| const renderPerPage = ({ partOfPage, before, to }: RenderingPages) => { | ||
| const slicedItems = partOfPage.slice(before, to); | ||
|
|
There was a problem hiding this comment.
onPageChange(currentPage - 1) is executed even when currentPage === 1. According to checklist items #5, #7 and #9, the prev link must be disabled on the first page and the callback should only fire when the page actually changes. Add a guard so that when currentPage === 1 the handler does nothing and does not call onPageChange.
| onPageChange: (page: number) => void; | ||
| } | ||
|
|
||
| export const Pagination = ({ |
There was a problem hiding this comment.
The RenderingPages interface defines a property item, but the renderPerPage function uses partOfPage in its parameter destructuring, which will cause a TypeScript error. Align the interface and destructuring (use the same property name).
| <li className={cn(`page-item ${currentPage === 1 ? `disabled` : ``}`)}> | ||
| <a | ||
| data-cy="prevLink" | ||
| className="page-link" | ||
| href="#prev" | ||
| aria-disabled={currentPage === 1} | ||
| onClick={e => { | ||
| e.preventDefault(); | ||
| onPageChange(currentPage - 1); |
There was a problem hiding this comment.
Because you always call onPageChange(currentPage - 1) on click, onPageChange will still be fired even when currentPage === 1. Requirement #5 says the callback should only trigger if the page was changed, and #9 says the prev link should be disabled on the first page; add a guard to avoid calling onPageChange when already on the first page.
| {pages.map(page => ( | ||
| <li | ||
| key={page} | ||
| className={cn(`page-item ${currentPage === page ? 'active' : ''}`)} | ||
| > | ||
| <a | ||
| data-cy="pageLink" | ||
| className="page-link" | ||
| href={`#${page}`} | ||
| onClick={e => { | ||
| e.preventDefault(); | ||
| onPageChange(page); |
There was a problem hiding this comment.
Here you always invoke onPageChange(page) even when page === currentPage. Requirement #5 requires that onPageChange only fires when the page actually changes; add a check to skip calling the callback if page equals currentPage.
| <li | ||
| className={cn( | ||
| `page-item ${currentPage === pages.length ? `disabled` : ``}`, | ||
| )} | ||
| > | ||
| <a | ||
| data-cy="nextLink" | ||
| className="page-link" | ||
| href="#next" | ||
| aria-disabled={currentPage === pages.length} | ||
| onClick={e => { | ||
| e.preventDefault(); | ||
| onPageChange(currentPage + 1); |
There was a problem hiding this comment.
Similar to the prev link, this will call onPageChange(currentPage + 1) even when currentPage is already the last page. You should prevent calling onPageChange when currentPage === pages.length to comply with requirement #5 and the disabled state behavior.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving your solution because all core, non-starred requirements of the pagination task are implemented correctly and there are no functional blockers. Your Pagination component is stateless, accepts the correct props, renders the correct number of pages, highlights the active page, and correctly disables prev/next with li.disabled and aria-disabled at the bounds. In App, you correctly track currentPage and perPage, slice the items to show only the relevant range, and render the info text in the required Page X (items A - B of N) format, which updates as the user navigates or changes the page size. You also handle onPageChange safely so it does nothing when clicking the current page or disabled prev/next, which matches the expected behavior.
The only missing piece is the optional starred requirement to sync page and perPage with the URL via React Router, but since this is explicitly optional, its absence does not block acceptance. If you want to extend this later, you could add React Router, read page and perPage from query params on load, and update the URL when the user changes pagination settings. Overall, this is a solid, clean implementation of the required pagination behavior—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.