add task solution#1473
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 pagination feature! The implementation is very solid and meets almost all the requirements.
However, one change is needed before this can be approved. The task requires the currentPage prop in the Pagination component to be optional with a default value of 1. Currently, it's defined as a required prop.
To fix this, please make the following changes in src/components/Pagination/Pagination.tsx:
- Update the prop's type definition to be optional:
currentPage?: number;. - Set a default value of
1for the prop within the component.
Once that's updated, your solution will be perfect. Keep up the excellent 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
| type Props = { | ||
| total: number; | ||
| perPage: number; | ||
| currentPage: number; |
There was a problem hiding this comment.
The task description states that currentPage should be an optional prop with a default value of 1. To implement this, you can change the type to currentPage?: number; and provide a default value for the prop in the component's signature.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job addressing the previous feedback and building out the pagination component! The core logic is working well, but I am requesting changes because a key requirement is missing.
The pagination state (page and perPage) needs to be synchronized with the URL query parameters. This allows the state to be preserved on page reloads and makes the URL shareable, which was part of the task's requirements.
- In
src/App.tsx, please use theuseSearchParamshook fromreact-router-domto read from and write to the URL. - Also, in
src/components/Pagination/Pagination.tsx, it's a good practice to callevent.preventDefault()in youronClickhandlers to prevent the browser's default navigation for links.
You're very close to a complete solution. Looking forward to your update!
✨ 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
| const [perPage, setPerPage] = useState(5); | ||
| const [currentPage, setCurrentPage] = useState(1); |
There was a problem hiding this comment.
The state for perPage and currentPage is currently managed locally. According to the requirements, this state should be synchronized with the URL query parameters (e.g., ?page=2&perPage=5) using React Router. On page load, the initial state should also be read from these URL parameters. You can use the useSearchParams hook from react-router-dom to manage this.
| onClick={() => { | ||
| if (currentPage !== 1) { | ||
| onPageChange(currentPage - 1); | ||
| } | ||
| }} |
There was a problem hiding this comment.
It's a good practice to prevent the default browser navigation for <a> tags when the click is handled by JavaScript. You can do this by accepting the event object in your onClick handler and calling event.preventDefault(). This should be applied to all clickable links in this component to avoid unintended changes to the URL's hash.
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 task! You've done a fantastic job implementing the pagination logic and synchronizing the state with the URL using useSearchParams. Your code is clean, well-structured, and meets the core functional requirements perfectly.
However, the automated tests are failing because the required data-cy attributes are missing from your JSX elements. For example, a page link should look like <a ... data-cy="pageLink">. Since the logic is solid, I am approving this solution as an educational override to help you progress. Please remember to include all data-cy attributes in future projects to ensure tests can pass. Great job on the hard parts!
✨ 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
brespect
left a comment
brespect
left a comment
There was a problem hiding this comment.
Good job, but you need to pass all test before requesting the review.
3 participants
DEMO LINK