Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job setting up the overall structure and state management for the pagination feature! The previous/next buttons and the perPage selector are working well. However, this requires changes because some core functionality is broken or missing.
The main issue is in src/components/Pagination/Pagination.tsx, where the page number links are not working correctly. They all display the number "1", and the active page isn't highlighted. This is due to a mismatch between the 0-indexed page variable in your loop and the 1-based currentPage prop. To fix this, ensure you are using the correct page number for both the display text and the active page comparison.
Additionally, in src/App.tsx, the pagination information text is hardcoded. This needs to be calculated dynamically using the currentPage, perPage, and total items props to reflect the current state accurately.
✨ 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
| onPageChange(currentPage - 1); | ||
| } | ||
| }} |
There was a problem hiding this comment.
This pagination information is hardcoded. According to the requirements, it should dynamically display the current page, the range of items shown, and the total number of items. You can use the currentPage, perPage, and items.length variables to construct this string.
| for (let i = 0; i < totalPages; i++) { | ||
| arrayPages.push(i); | ||
| } |
There was a problem hiding this comment.
This loop creates an array of 0-indexed page numbers ([0, 1, 2, ...]). However, currentPage is a 1-based number. This mismatch causes issues when determining the active page and handling page clicks. Consider creating an array of 1-based page numbers (from 1 to totalPages) to simplify the logic.
| </li> | ||
| {arrayPages.map(page => ( | ||
| <li | ||
| className={`page-item ${page === currentPage ? 'active' : ''}`} |
There was a problem hiding this comment.
The logic to determine the active page is incorrect because page is a 0-based index from arrayPages while currentPage is 1-based. For example, for the first page, this will compare 0 with 1.
| } | ||
| }} | ||
| > | ||
| 1 |
There was a problem hiding this comment.
The text for the page link is hardcoded to 1. It should display the correct page number for each link.
src/App.tsx
Outdated
| const [perPage, setPerPage] = useState(5); | ||
| const [currentPage, setCurrentPage] = useState(1); |
There was a problem hiding this comment.
The task requires using React Router to manage state in the URL. currentPage and perPage should be initialized from the URL's query parameters on page load, and the URL should be updated when they change. Using useState with static initial values doesn't meet this requirement.
src/App.tsx
Outdated
| const [perPage, setPerPage] = useState(5); | ||
| const [currentPage, setCurrentPage] = useState(1); | ||
|
|
||
| const onPageCange = (page: number) => { |
There was a problem hiding this comment.
There's a small typo in the function name. It should be onPageChange to match the prop name expected by the Pagination component for consistency and to follow the task description.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent work on the pagination logic and addressing the previous feedback! The components are well-structured and the core functionality is implemented correctly.
This requires changes because a key requirement from the task description is missing. The application must use React Router to store the currentPage and perPage values in the URL's query parameters (e.g., ?page=2&perPage=5). This is essential for making the application state shareable and persistent on page reloads.
To implement this, you can use the useSearchParams hook from react-router-dom in src/App.tsx to read from and write to the URL. Also, there's a minor typo in a function name: please rename onPageCange to onPageChange for consistency. You're very close to finishing!
✨ 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
src/App.tsx
Outdated
| const [perPage, setPerPage] = useState(5); | ||
| const [currentPage, setCurrentPage] = useState(1); |
There was a problem hiding this comment.
The core functionality is well-implemented. However, the task requires using React Router to store the currentPage and perPage values in the URL query parameters (e.g., ?page=2&perPage=5). This makes the application state shareable and reload-proof.
You can use the useSearchParams hook from react-router-dom to get and set the search parameters from the URL.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Sorry, the pull request review was unsuccessful for the following reasons:
Warning
No changes were made in the pull request since the last review. Please make sure that you have added files to the pull request and committed the changes.
[CODE: NCM]
If the issue is within your control, please address it.
The next review attempt will automatically launch in about 1 hour.
If you believe this is an error or need assistance, please contact the support team.
2 participants
No description provided.