Skip to content

done#1519

Open
misulkin wants to merge 5 commits into
mate-academy:masterfrom
misulkin:develop
Open

done#1519
misulkin wants to merge 5 commits into
mate-academy:masterfrom
misulkin:develop

Conversation

@misulkin

@misulkin misulkin commented Jun 24, 2026

Copy link
Copy Markdown

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

I am requesting changes because one core requirement of the task is not implemented yet: synchronizing pagination state with the URL using React Router. Your App.tsx currently manages currentPage and perPage only via useState, and the URL never reflects these values (e.g. no ?page=2&perPage=7), nor are query parameters read on initial load. This is a functional requirement, not just a nice-to-have, so without it the task is considered incomplete.

To fix this, you should:

  • Wrap your application in a router (e.g. BrowserRouter in main.tsx or index.tsx).
  • In App.tsx, use React Router hooks such as useSearchParams (or useLocation + useNavigate) to:
    • Initialize currentPage and perPage from the page and perPage query parameters (with sensible numeric defaults when they are missing or invalid).
    • Update those query parameters whenever currentPage or perPage changes (and ensure changing perPage still resets the page to 1).
      Everything else about your pagination component and UI behavior looks solid, so once you add the URL syncing with React Router, your solution should meet all the requirements. Keep going—you’re very close to a complete and robust implementation.

✨ 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

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

I am requesting changes because, although your implementation meets the functional and routing requirements of the task, there are still CI failures that are considered blocking for this assignment. Functionally, your Pagination component and App correctly implement stateless pagination, URL synchronization via React Router, proper data-cy usage, and accurate page/item calculations, so the core checklist items are satisfied. However, ESLint reports two issues in src/App.tsx: an unused variable totalPages (around line 17), and a label without an associated control (around line 55), which violates the accessibility rule jsx-a11y/label-has-associated-control. These must be fixed by either using or removing totalPages, and by adding a matching id on the select and htmlFor on the label. Once you address these linter errors, your solution should be in good shape to pass all required checks; you’ve done solid work on the main behavior, and this is mainly about cleaning up the remaining tooling issues.


✨ 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

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

I am approving your solution as an educational override so you can keep progressing while learning. You’ve done a strong job making Pagination stateless, wiring it to React Router via useSearchParams, preserving the required HTML structure and data-cy attributes, and correctly implementing the info line and per-page selector according to the task requirements. One remaining issue is that in src/components/Pagination/Pagination.tsx your Props type still requires currentPage, while the checklist specifies it should be optional and default to 1 when omitted [CHECKLIST ITEM #20]; you can fix this by declaring currentPage?: number and using a default value in the component destructuring (e.g. const { total, perPage, currentPage = 1, onPageChange } = props). Aside from that, your use of li.active/li.disabled, aria-disabled="true", and proper onPageChange behavior shows good attention to both functionality and accessibility. Overall, you’ve addressed the previous review points well, and this last type/detail tweak will help make your solution fully aligned with the specification. Keep up the good work—this is very solid progress for a routing + pagination task.


✨ 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

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

Comment thread src/App.tsx
import { getNumbers } from './utils';
import { useSearchParams } from 'react-router-dom';
import Pagination from './components/Pagination/Pagination';

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

currentPage is required in Props, but the requirements specify it must be optional and default to 1 if not provided (checklist items #2 and #20). Consider making currentPage optional in the type (e.g. currentPage?: number) and providing a default value of 1 in the component destructuring.

@etojeDenys etojeDenys left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

demo link does not work. please fix it and request the review again

@misulkin misulkin requested a review from etojeDenys June 24, 2026 19:06

@brespect brespect left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good progress, but you DEMO LINK is not working. Also, don't forget to pass all tests before requesting the review

@misulkin misulkin requested a review from brespect June 25, 2026 14:47

@Denys-Kravchuk9988 Denys-Kravchuk9988 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job!

A few things to improve:

return (
<ul className="pagination">
{/* PREV */}
<li className={`page-item ${currentPage === 1 ? 'disabled' : ''}`}>

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better to use classNames here

{pages.map(page => (
<li
key={page}
className={`page-item ${currentPage === page ? 'active' : ''}`}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better to use classNames here


{/* NEXT */}
<li
className={`page-item ${currentPage === totalPages ? 'disabled' : ''}`}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better to use classNames here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants