add task solution#1514
Conversation
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 pagination requirements are implemented correctly and there are no critical blockers that would prevent the task from functioning as intended. Your Pagination component is stateless with the correct props and default currentPage, onPageChange only fires when the page actually changes, and the prev/next controls, active page highlighting, and disabled states are all handled correctly. The per-page selector has the required options (3, 5, 10, 20), resets to page 1 on change, and the info text (Page X (items A - B of N)) is computed accurately.
The only notable mismatch is around the optional React Router requirement: you manage page and perPage via window.location.search and history.pushState in App.tsx and Pagination.tsx instead of using Router hooks like useSearchParams/useNavigate. Functionally this still satisfies the behavior of syncing URL query params, but it does not literally use React Router as described in the starred requirement; if your project already includes React Router, refactoring to useSearchParams for reading/writing ?page and perPage would make it fully aligned. As it stands, this is not a functional blocker, just an opportunity to better match the intended approach. Overall, your implementation is solid and meets the essential criteria; nice work on keeping the Pagination component clean and controlled via props.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| useEffect(() => { | ||
| const params = new URLSearchParams(window.location.search); | ||
| params.set('page', currentPage.toString()); | ||
| params.set('perPage', perPage.toString()); | ||
|
|
||
| const newUrl = `${window.location.pathname}?${params.toString()}`; | ||
| window.history.pushState({}, '', newUrl); | ||
| }, [currentPage, perPage]); |
There was a problem hiding this comment.
You update the URL query params manually with window.history.pushState. The optional requirement (#15/#18) mentions using React Router to persist page and perPage; if your project is already using React Router, consider using its hooks (e.g. useSearchParams) instead of direct window access to better match that requirement.
| const params = new URLSearchParams(window.location.search); | ||
| const page = parseInt(params.get('page') || '1', 10); | ||
| return isNaN(page) || page < 1 ? 1 : page; | ||
| }); |
There was a problem hiding this comment.
Defaulting currentPage to 1 in the props destructuring fulfills the requirement that this prop be optional with a default of 1, and it keeps the component stateless as requested.
| params.set('page', currentPage.toString()); | ||
| params.set('perPage', perPage.toString()); | ||
|
|
||
| const newUrl = `${window.location.pathname}?${params.toString()}`; | ||
| window.history.pushState({}, '', newUrl); | ||
| }, [currentPage, perPage]); | ||
|
|
||
| const totalItems = items.length; | ||
| const startItemIndex = (currentPage - 1) * perPage; | ||
| const endItemIndex = Math.min(startItemIndex + perPage, totalItems); |
There was a problem hiding this comment.
Your handlePageClick correctly prevents default anchor navigation and only calls onPageChange when the new page is different and within 1..totalPages, which satisfies the requirement that onPageChange only fire when the page actually changes.
| value={perPage} | ||
| onChange={handlePerPageChange} | ||
| > | ||
| <option value="3">3</option> | ||
| <option value="5">5</option> | ||
| <option value="10">10</option> |
There was a problem hiding this comment.
The aria-disabled attributes and disabled classes on prev/next links meet the requirement to disable navigation when on the first or last page; make sure not to change these attributes or tests may fail.
|
|
||
| export const Pagination: React.FC<Props> = ({ |
There was a problem hiding this comment.
The task’s optional requirement (*) explicitly mentions using React Router to save and read ?page and perPage from the URL; here you are manually reading from window.location.search instead of using router APIs like useSearchParams, which does not satisfy that specific requirement.
| const isLastPage = currentPage === totalPages; | ||
|
|
||
| const handlePageClick = ( | ||
| event: React.MouseEvent<HTMLAnchorElement>, | ||
| page: number, | ||
| ) => { | ||
| event.preventDefault(); |
There was a problem hiding this comment.
Similarly, URL updates are handled with window.history.pushState rather than React Router navigation (e.g., useNavigate or setSearchParams), so query param synchronization is not implemented via React Router as requested in the description.
2 participants
DEMO LINK