Conversation
…ntegrate it with `App`.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
This is a great start, and the core pagination logic is working correctly! However, a few changes are needed to meet all the task requirements.
The most important missing piece is synchronizing the pagination state with the URL. In src/App.tsx, please use query parameters (e.g., ?page=2&perPage=10) to manage the page and perPage state. This is a key requirement that allows users to bookmark and share links. The useSearchParams hook from react-router-dom is a great tool for this.
Additionally, in src/components/Pagination/Pagination.tsx, the currentPage prop should be optional with a default value of 1 as specified. You can update its type to currentPage?: number; and set the default in the component's props.
You're very close to a complete solution!
✨ 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 = ({ | ||
| 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. You should make it optional in the Props type (e.g., currentPage?: number;) and provide a default value in the component's signature.
| key={n} | ||
| className={cn('page-item', { | ||
| active: currentPage === n, | ||
| disabled: n === currentPage, |
There was a problem hiding this comment.
The requirements state that the current page's li element should have the active class, which you've correctly implemented. However, adding the disabled class here is not specified in the task and is redundant, as the click handler already prevents action on the current page.
There was a problem hiding this comment.
Це ж додано, щоб візуально виділити не клікабельний стан
| const [currentPage, setCurrentPage] = React.useState(1); | ||
| const [perPage, setPerPage] = React.useState(5); |
There was a problem hiding this comment.
The task requires using React Router to manage the page and perPage values in the URL. Currently, the state is initialized with static values. You should read these values from the URL's query parameters on page load to properly apply them.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Good progress on the components! However, a couple of key requirements are still missing, so changes are needed before this can be approved.
The most critical issue is that the application state is not synchronized with the URL. The page and perPage values must be managed using URL query parameters (e.g., ?page=2&perPage=5) so the state can be bookmarked and shared. Please use the useSearchParams hook from react-router-dom in src/App.tsx to implement this.
Additionally, in src/components/Pagination/Pagination.tsx, the optional currentPage prop needs a default value of 1. You can set this directly in the component's props destructuring, like ({ currentPage = 1, ...props }).
Addressing these two points will complete the requirements. You're very close!
✨ 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
| export const Pagination = ({ | ||
| total, | ||
| perPage, | ||
| currentPage, | ||
| onPageChange, | ||
| }: Props) => { |
There was a problem hiding this comment.
Excellent work on making the currentPage prop optional in the type definition. To fully meet the requirement (currentPage={1} /* optional with 1 by default */), you also need to provide a default value of 1. You can easily set this up during prop destructuring, like currentPage = 1.
| const [currentPage, setCurrentPage] = React.useState(1); | ||
| const [perPage, setPerPage] = React.useState(5); |
There was a problem hiding this comment.
The task requires synchronizing the page and perPage state with the URL using query parameters (e.g., ?page=2&perPage=5). Currently, you're using local component state with useState, which doesn't persist in the URL. Please use the useSearchParams hook from react-router-dom to read the initial state from the URL and to update it when the page or per-page value changes.
… to handle undefined cases.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job fixing the Pagination component and correctly setting the default prop value!
I am approving this submission as an educational override to help you move forward, but the most critical requirement from the previous review has not yet been addressed. The application's state for page and perPage must be managed through URL query parameters, not local useState.
In src/App.tsx, please replace the useState hooks for currentPage and perPage with the useSearchParams hook from react-router-dom. This will ensure the application's state can be bookmarked and shared, which is a key requirement of the task. Keep up the great work!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| const [currentPage, setCurrentPage] = React.useState(1); | ||
| const [perPage, setPerPage] = React.useState(5); |
There was a problem hiding this comment.
According to the task requirements, the currentPage and perPage values should be managed through URL query parameters, not local component state. Please use the useSearchParams hook from react-router-dom to read these values from the URL on load and to update the URL when they change.
3 participants
No description provided.