Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great work on this! The pagination component is well-structured and most of the functionality is in place. However, I'm requesting a couple of changes to meet all the requirements.
The most critical issue is that the application state doesn't sync with the URL when using the browser's back and forward buttons. In src/App.tsx, you're initializing state from the URL with useState, which only runs on the initial render. To fix this, the URL should be the single source of truth. You can achieve this by removing the useState for currentPage and perPage and instead reading their values directly from useSearchParams on every render.
Additionally, in src/components/Pagination/Pagination.tsx, the task requires the currentPage prop to be optional with a default value of 1. Please update the Props type to make it optional and provide the default value in the component signature.
Once these two points are addressed, your solution will be perfect. Keep up the excellent effort!
✨ 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
| currentPage: number; | ||
| onPageChange: (page: number) => void; | ||
| }; | ||
|
|
||
| export const Pagination: React.FC<Props> = ({ | ||
| total, | ||
| perPage, | ||
| currentPage, |
There was a problem hiding this comment.
The task description specifies that the currentPage prop should be optional with a default value of 1. Currently, it's defined as a required prop.
To meet the requirement, you should mark currentPage as optional in the Props type and provide a default value when destructuring props in the component signature.
src/App.tsx
Outdated
| const [currentPage, setCurrentPage] = React.useState(pageFromUrl); | ||
| const [perPage, setPerPage] = React.useState(perPageFromUrl); |
There was a problem hiding this comment.
Initializing state from the URL works for the initial page load, but it causes a problem with browser navigation. useState's initializer is only used during the first render. If the user clicks the back or forward button, the URL will change and the component will re-render, but currentPage and perPage will retain their old values from the state, leading to a mismatch between the URL and what's displayed.
To fix this, the URL should be the single source of truth. You can achieve this by removing useState for currentPage and perPage and instead deriving their values directly from searchParams on every render.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent work on this iteration! I am approving your solution as you've successfully addressed all the previous feedback. The application now correctly uses the URL as the single source of truth for pagination state, and the Pagination component's props meet all requirements. The code is clean, well-structured, and fully functional. Great job!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
https://butko9595.github.io/react_pagination/