add solution by Evg3n22 · Pull Request #1443 · mate-academy/react_pagination

Evg3n22 · 2026-02-25T15:01:18Z

Replace <your_account> with your Github username in the DEMO LINK and add it to the PR description.

mateacademy-ai-mentor

Review Summary

Excellent work on implementing the pagination functionality! The application works as expected, and the state management in App.tsx is solid. However, this submission requires a change to the component architecture to meet a core requirement.

The Pagination component should be a reusable UI component, responsible only for rendering the pagination controls. Currently, it also contains the logic for filtering and rendering the list of items. This responsibility should be moved to the parent App component.

To resolve this, please refactor your code so that:

The App component handles slicing the items array based on the current page.
The App component is responsible for rendering the <ul> list of items.
The Pagination component only receives props needed for navigation (like currentPage, total, perPage, onPageChange) and renders the page links and buttons.

This change will align the solution with the Single Responsibility Principle and make your Pagination component truly reusable. You're very close to a perfect solution!

✨ Please rate this review in the Platform's notification center¹. 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 button² 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 tag³.
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.

mateacademy-ai-mentor · 2026-02-25T15:03:29Z

src/App.tsx

+// type PerPage = '3' | '5' | '10' | '20';

 // eslint-disable-next-line @typescript-eslint/no-unused-vars
 const items = getNumbers(1, 42).map(n => `Item ${n}`);


The items array is defined here but is not used to render the list of items. The App component, as the container, should handle the logic for slicing the items based on the current page and items per page, and then render the resulting list. This would make the Pagination component more reusable, as it could then focus solely on rendering pagination controls.

mateacademy-ai-mentor · 2026-02-25T15:03:29Z

src/components/Pagination/Pagination.tsx

+  onPageChange: (page: number) => void;
+};
+
+const items: string[] = getNumbers(1, 42).map(n => `Item ${n}`);


The Pagination component should be a reusable UI component responsible only for rendering pagination controls. It shouldn't be aware of the specific data (items) it's paginating. This items array and the logic for filtering it should be managed by the parent App component.

mateacademy-ai-mentor · 2026-02-25T15:03:29Z

src/components/Pagination/Pagination.tsx

+        {arrSelectedNum.map((num, idx) => {
+          return (
+            <li
+              key={idx}


It's a best practice to use a unique and stable identifier for the key prop when rendering lists. Since each page number (num) is unique, it would be more appropriate to use key={num} instead of the array index (idx).

mateacademy-ai-mentor · 2026-02-25T15:03:29Z

src/components/Pagination/Pagination.tsx

+            className="page-link"
+            href="#next"
+            aria-disabled={currentPage === pages}
+            onClick={() => moveArrows('next')}


For consistency, the onClick handler should be placed on the <li> element, similar to how it's done for the 'previous' button on line 61. Currently, the handler for the 'next' button is on the <a> tag, while the 'previous' button's handler is on the <li>.

mateacademy-ai-mentor · 2026-02-25T15:03:29Z

src/components/Pagination/Pagination.tsx

+      <ul>
+        {currentItems.map((item, idx) => {
+          return (
+            <li data-cy="item" key={idx}>
+              {item}
+            </li>
+          );
+        })}
+      </ul>


The responsibility of rendering the items for the current page should belong to the parent component (App.tsx), not the Pagination component. This entire <ul> block, along with the associated filtering logic (lines 30-32), should be moved to the App component to keep the Pagination component focused on just navigation.

mateacademy-ai-mentor

Review Summary

Excellent work on refactoring the components! The separation of concerns is much clearer now. However, a couple of key requirements are not yet met, so I am requesting changes.

URL State Management: In src/App.tsx, the page and perPage values need to be managed in the URL's search parameters (e.g., ?page=2&perPage=5). The application should read these values on load and update the URL when they change. The useSearchParams hook from react-router-dom is perfect for this.
Incorrect Callback Trigger: In src/components/Pagination/Pagination.tsx, the onPageChange callback is fired even when clicking the currently active page. Please add a condition to ensure the callback is only invoked when the selected page is different from the currentPage.

Additionally, for better consistency, consider moving the onClick handler for the 'next' button from the <a> tag to its parent <li> element. You're very close to finishing this task!

✨ Please rate this review in the Platform's notification center¹. 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 button² 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 tag³.
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.

mateacademy-ai-mentor · 2026-02-25T15:13:47Z

src/components/Pagination/Pagination.tsx

+              className={cn('page-item', {
+                active: num === currentPage,
+              })}
+              onClick={() => onPageChange(num)}


This onClick handler calls onPageChange even when the user clicks on the currently active page. The requirements state that the callback should be triggered only if the page has changed. Please add a condition to ensure onPageChange is called only when num is different from currentPage.

mateacademy-ai-mentor · 2026-02-25T15:13:47Z

src/components/Pagination/Pagination.tsx

+            className="page-link"
+            href="#next"
+            aria-disabled={currentPage === pages}
+            onClick={() => moveArrows('next')}


The onClick handler for the 'next' button is placed on the <a> tag, while for the 'previous' button and page numbers, it's on the parent <li>. For better consistency and a larger clickable area, please move this handler to the parent <li> element (line 79).

mateacademy-ai-mentor · 2026-02-25T15:13:47Z

src/App.tsx

+  const [currentPage, setCurrentPage] = useState(1);
+  const [selectedNum, setSelectedNum] = useState('5');


According to the task requirements, the page and perPage values should be managed using URL search parameters, not just local component state. On page load, these values should be read from the URL, and when they change, the URL should be updated. Consider using the useSearchParams hook from react-router-dom to implement this.

mateacademy-ai-mentor

Review Summary

Fantastic work on this iteration! You've successfully addressed all the feedback and met every requirement. The implementation of URL state management using useSearchParams is excellent, handling the two-way synchronization perfectly. You also correctly fixed the onPageChange callback to prevent it from firing unnecessarily. The code is clean, functional, and ready to be merged. Great job!

✨ Please rate this review in the Platform's notification center¹. I will be grateful for your feedback! ✨

Rate AI review example ↩

add solution

2bf82ea

mateacademy-ai-mentor suggested changes Feb 25, 2026

View reviewed changes

fix: make paggination component more reusful

63ffda4

Evg3n22 requested a review from mateacademy-ai-mentor February 25, 2026 15:11

mateacademy-ai-mentor suggested changes Feb 25, 2026

View reviewed changes

fix: prevent incorrect callback trigger and url state managment

65fe9f6

Evg3n22 requested a review from mateacademy-ai-mentor February 25, 2026 17:08

mateacademy-ai-mentor approved these changes Feb 25, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add solution#1443

add solution#1443
Evg3n22 wants to merge 3 commits intomate-academy:masterfrom
Evg3n22:develop

Evg3n22 commented Feb 25, 2026

Uh oh!

mateacademy-ai-mentor left a comment

Uh oh!

mateacademy-ai-mentor Feb 25, 2026

Uh oh!

mateacademy-ai-mentor Feb 25, 2026

Uh oh!

mateacademy-ai-mentor Feb 25, 2026

Uh oh!

mateacademy-ai-mentor Feb 25, 2026

Uh oh!

mateacademy-ai-mentor Feb 25, 2026

Uh oh!

mateacademy-ai-mentor left a comment

Uh oh!

mateacademy-ai-mentor Feb 25, 2026

Uh oh!

mateacademy-ai-mentor Feb 25, 2026

Uh oh!

mateacademy-ai-mentor Feb 25, 2026

Uh oh!

mateacademy-ai-mentor left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		const [currentPage, setCurrentPage] = useState(1);
		const [selectedNum, setSelectedNum] = useState('5');

Conversation

Evg3n22 commented Feb 25, 2026

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants