Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because there are still a few critical issues in the Pagination component that violate explicit requirements and can break expected behavior. Most importantly, currentPage is declared as a required prop (currentPage: number) while the spec requires it to be optional with a default of 1, and onPageChange is typed as returning a mouse handler instead of a simple (page: number) => void callback; this breaks the intended public API [CHECKLIST ITEM #2]. Also, the page-number links currently call onPageChange(item) even when item === currentPage, which violates the requirement that the callback only fires when the page actually changes [CHECKLIST ITEM #5]; you should add a guard like if (item !== currentPage) and still call event.preventDefault() to avoid unwanted navigation. Additionally, the prev/next anchors keep their href attributes even when disabled and use empty handlers, so clicking them can still change the URL hash; you should either prevent default in the click handler when disabled or omit the href in that case while keeping li.disabled and aria-disabled="true" [CHECKLIST ITEM #7 and #8].
To fix this, update the props interface to currentPage?: number and onPageChange: (page: number) => void, and provide currentPage = 1 when destructuring props in the component. Then, adjust all click handlers to call event.preventDefault() and only invoke onPageChange(newPage) if the new page is different from the current one and not out of bounds. These are all localized changes in src/components/Pagination/Pagination.tsx, and once addressed, the rest of your implementation (including App.tsx state handling and rendering) already looks solid and close to the required behavior. You're very close—after these fixes, your solution should align well with the checklist and expected functionality.
✨ 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
| interface PaginationType { | ||
| total: number; | ||
| perPage: number; | ||
| currentPage: number; |
There was a problem hiding this comment.
| onPageChange: ( | ||
| value: number, | ||
| ) => React.MouseEventHandler<HTMLAnchorElement> | undefined | void; |
There was a problem hiding this comment.
| export const Pagination = ({ | ||
| total, | ||
| perPage, | ||
| currentPage, | ||
| onPageChange, |
There was a problem hiding this comment.
Provide a default for currentPage when destructuring props. The component uses currentPage directly without a default; set currentPage = 1 in the parameter list (or handle default in the interface) so the component works standalone and follows the spec.
| <a | ||
| data-cy="pageLink" | ||
| className="page-link" | ||
| href="#1" | ||
| onClick={() => onPageChange(item)} |
There was a problem hiding this comment.
Do not call onPageChange when user clicks the already active page. The page link onClick={() => onPageChange(item)} will call the callback even if item === currentPage. Add a guard to avoid calling the callback for the current page (checklist #5).
| <a | ||
| data-cy="prevLink" | ||
| className="page-link" | ||
| href="#prev" | ||
| aria-disabled={currentPage === 1 ? 'true' : 'false'} | ||
| onClick={ | ||
| currentPage !== 1 ? () => onPageChange(currentPage - 1) : () => {} | ||
| } |
There was a problem hiding this comment.
Prevent navigation for disabled prev/next links. When prev/next are disabled you use () => {} but still have href attributes (e.g. href="#prev") so clicking may change the URL hash. Prevent default in the click handler for disabled links or remove the href when disabled to avoid unwanted navigation while keeping li.disabled and aria-disabled="true" (checklist #8).
| href="#next" | ||
| aria-disabled={currentPage === lastPageNumber ? 'true' : 'false'} | ||
| onClick={ | ||
| currentPage !== lastPageNumber | ||
| ? () => onPageChange(currentPage + 1) | ||
| : () => {} |
There was a problem hiding this comment.
Same prevention needed for the next link. The next anchor also uses an empty handler when disabled; ensure you prevent default or remove its href when disabled to avoid hash changes.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job on fixing the prop types and implementing the logic in App.tsx! Your progress is excellent. However, there are two critical issues in Pagination.tsx that need to be addressed before this can be approved.
onPageChangefires on active page click: TheonPageChangecallback is triggered even when clicking the currently active page. The requirement is to only trigger it when the page actually changes. Please add a condition to the page number'sonClickhandler to ensure it doesn't fire ifitem === currentPage.- Links change the URL: All the
<a>tags are missingevent.preventDefault()in theironClickhandlers. This causes the URL hash to change on every click (e.g., to#prevor#1), which is incorrect behavior, especially for disabled links. Please update all click handlers to prevent this default action.
Once these two issues are resolved, the component will be working perfectly. Keep up the great work!
✨ 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={ | ||
| currentPage !== 1 ? () => onPageChange(currentPage - 1) : () => {} | ||
| } |
There was a problem hiding this comment.
This onClick handler should prevent the default link behavior. Because the <a> tag has an href attribute, clicking it will change the URL hash to #prev even if the link is disabled. You should modify the handler to accept the event and call event.preventDefault(). This applies to the next link and all page number links as well.
| data-cy="pageLink" | ||
| className="page-link" | ||
| href="#1" | ||
| onClick={() => onPageChange(item)} |
There was a problem hiding this comment.
According to the requirements, the onPageChange callback should only be triggered if the page was changed. This handler calls onPageChange even when the user clicks on the currently active page (item === currentPage). You need to add a condition here to avoid calling the function unnecessarily.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent work on this task! You've successfully implemented all the core requirements, and the pagination component is functioning perfectly. I am approving your solution so you can move forward, but I wanted to point out one small area for improvement. In src/components/Pagination/Pagination.tsx, the page links have a static href="#1". While you correctly use e.preventDefault(), it's better for semantics and accessibility to make the href dynamic (e.g., href={#${item}}) or use a neutral placeholder like href="#". This is a minor point in an otherwise fantastic submission. Keep up the great work!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| <a | ||
| data-cy="pageLink" | ||
| className="page-link" | ||
| href="#1" |
There was a problem hiding this comment.
This href is static and points to #1 for all page links. While e.preventDefault() stops the browser from navigating, it's better practice to make this dynamic (e.g., href={#${item}}) or use a non-navigational placeholder like href="#". This improves semantics and accessibility.
No description provided.