add task solution by Ivadriil · Pull Request #1485 · mate-academy/react_pagination

Ivadriil · 2026-04-25T17:28:31Z

mateacademy-ai-mentor

Review Summary

Your implementation has critical issues that violate task requirements. The onPageChange callback is triggered on every click, including when disabled or on the current page - but requirement #5 explicitly states it should only fire when the page actually changes. The click handlers on <li> elements bypass the disabled state entirely, causing unnecessary callbacks. Additionally, the pagination info display logic is incorrect - it caps the upper bound at 42 on all pages, but should only cap on the final page (e.g., with perPage=20 on page 2, it currently shows '21-42 of 42' but should show '21-40 of 42'). You need to move the onClick handlers to the <a> elements and add conditional guards like if (newPage !== currentPage && newPage >= 1 && newPage <= pageCount) onPageChange(newPage). Also remove the accidental sdsds.md file and replace hardcoded 42 values with the total prop for dynamic behavior.

✨ 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-04-25T17:30:11Z


      <p className="lead" data-cy="info">
-        Page 1 (items 1 - 5 of 42)
+        Page {currentPage} (items {firstItem + 1} -{' '}
+        {lastItems > 42 ? 42 : lastItems} of 42)


The onClick handler on <li> always calls onPageChange, even when disabled. According to requirement #5, the callback should only be triggered if the page was actually changed. Currently, clicking the disabled prev button still calls onPageChange(currentPage - 1) which evaluates to onPageChange(0) when on page 1. Move the page change logic to the <a> onClick handler or add guards to prevent calling onPageChange when disabled.

mateacademy-ai-mentor · 2026-04-25T17:30:11Z


-      {/* Move this markup to Pagination */}
-      <ul className="pagination">
-        <li className="page-item disabled">
-          <a
-            data-cy="prevLink"
-            className="page-link"
-            href="#prev"
-            aria-disabled="true">
-            «
-          </a>
-        </li>
-        <li className="page-item active">
-          <a data-cy="pageLink" className="page-link" href="#1">
-            1
-          </a>
-        </li>
-        <li className="page-item">
-          <a data-cy="pageLink" className="page-link" href="#2">
-            2
-          </a>
-        </li>
-        <li className="page-item">
-          <a data-cy="pageLink" className="page-link" href="#3">
-            3
-          </a>
-        </li>
-        <li className="page-item">
-          <a data-cy="pageLink" className="page-link" href="#4">
-            4
-          </a>
-        </li>
-        <li className="page-item">
-          <a data-cy="pageLink" className="page-link" href="#5">
-            5
-          </a>
-        </li>
-        <li className="page-item">
-          <a data-cy="pageLink" className="page-link" href="#6">
-            6
-          </a>
-        </li>
-        <li className="page-item">
-          <a data-cy="pageLink" className="page-link" href="#7">
-            7
-          </a>
-        </li>
-        <li className="page-item">
-          <a data-cy="pageLink" className="page-link" href="#8">
-            8
-          </a>
-        </li>
-        <li className="page-item">
-          <a data-cy="pageLink" className="page-link" href="#9">
-            9
-          </a>
-        </li>
-        <li className="page-item">
-          <a
-            data-cy="nextLink"
-            className="page-link"
-            href="#next"
-            aria-disabled="false">
-            »
-          </a>
-        </li>
-      </ul>
+      <Pagination
+        total={items.length}
+        perPage={perPage}
+        currentPage={currentPage}


Same issue as prev button - onPageChange is called on every click, including when disabled. Clicking the disabled next button on the last page still triggers the callback with the same page number.

mateacademy-ai-mentor · 2026-04-25T17:30:11Z

      <p className="lead" data-cy="info">
-        Page 1 (items 1 - 5 of 42)
+        Page {currentPage} (items {firstItem + 1} -{' '}


Hardcoded 42 should be replaced with total prop to properly display the actual total count when it changes.

mateacademy-ai-mentor · 2026-04-25T17:30:11Z

      <p className="lead" data-cy="info">
-        Page 1 (items 1 - 5 of 42)
+        Page {currentPage} (items {firstItem + 1} -{' '}
+        {lastItems > 42 ? 42 : lastItems} of 42)


Hardcoded 42 should use total prop for consistency with the actual data length.

mateacademy-ai-mentor · 2026-04-25T17:30:11Z

+
+  const newItems = items.slice(firstItem, lastItems);
+
  return (


Redundant .map(n => n) - this creates a new array with the same values. Use getNumbers(1, pageCount) directly, or use .map with a meaningful transformation if needed.

mateacademy-ai-mentor · 2026-04-25T17:30:11Z

+      <li
+        className={cn('page-item', { disabled: currentPage === 1 })}
+        onClick={() => onPageChange(currentPage <= 1 ? 1 : currentPage - 1)}
+      >
+        <a
+          data-cy="prevLink"
+          className="page-link"
+          href="#prev"
+          aria-disabled={currentPage === 1}
+        >
+          «
+        </a>


The onClick handler is on the <li> element instead of <a>. This means clicking on disabled items still triggers the callback. Move onClick to the <a> element and add a condition to prevent calling onPageChange when already on page 1.

mateacademy-ai-mentor · 2026-04-25T17:30:11Z

+        className={cn('page-item', { disabled: currentPage === 1 })}
+        onClick={() => onPageChange(currentPage <= 1 ? 1 : currentPage - 1)}


The onClick handler on <li> always calls onPageChange even when on the first page. Requirement #5 states callback should be triggered only if page was changed. When currentPage === 1, clicking previous should not call onPageChange at all.

mateacademy-ai-mentor · 2026-04-25T17:30:11Z

+      <li
+        className={cn('page-item', { disabled: currentPage === pageCount })}
+        onClick={() =>
+          onPageChange(currentPage >= pageCount ? pageCount : currentPage + 1)
+        }


The onClick handler is on the <li> element instead of <a>. This means clicking on disabled items still triggers the callback. Move onClick to the <a> element and add a condition to prevent calling onPageChange when already on the last page.

mateacademy-ai-mentor · 2026-04-25T17:30:11Z

+        onClick={() =>
+          onPageChange(currentPage >= pageCount ? pageCount : currentPage + 1)


The onClick handler always calls onPageChange even when already on the last page. Requirement #5 states callback should be triggered only if page was changed. When currentPage === pageCount, clicking next should not call onPageChange at all.

mateacademy-ai-mentor · 2026-04-25T17:30:11Z

+        className={cn('page-item', { disabled: currentPage === 1 })}
+        onClick={() => onPageChange(currentPage <= 1 ? 1 : currentPage - 1)}


The pagination info display logic is incorrect. Using lastItems > 42 ? 42 : lastItems caps the upper bound on ALL pages, but it should only cap on the LAST page. For example, with perPage=20 on page 2: current code shows 'items 21 - 42 of 42' but should show 'items 21 - 40 of 42'.

mateacademy-ai-mentor

Review Summary

The numbered page links in Pagination.tsx directly call onPageChange(count) instead of using the handlePageChange guard function, violating requirement #5 that onPageChange should only trigger when the page actually changes. This must be fixed by updating line 47 to use handlePageChange(count) for consistency.

✨ 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-04-25T18:27:05Z

+            className="page-link"
+            onClick={() => onPageChange(count)}


This calls onPageChange directly without checking if the page actually changed. For consistency with the prev/next buttons (which use handlePageChange), this should also use handlePageChange(count) to ensure the callback is only triggered when the page actually changes (requirement #5).

mateacademy-ai-mentor

Review Summary

Your solution is approved. All nine requirements have been successfully implemented, including proper pagination logic, data-cy attributes for testing, page change guards, navigation controls with disabled states, and the per-page selector. The fix to use handlePageChange(count) instead of directly calling onPageChange on numbered page links was done correctly - this ensures the callback only triggers when the page actually changes. Great job on the clean TypeScript implementation and proper component structure!

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

Rate AI review example ↩

add task solution

4f09918

mateacademy-ai-mentor suggested changes Apr 25, 2026

View reviewed changes

Fix pagination behavior and info display

5c22e2a

Ivadriil requested a review from mateacademy-ai-mentor April 25, 2026 18:25

mateacademy-ai-mentor suggested changes Apr 25, 2026

View reviewed changes

Ivadriil added 2 commits April 25, 2026 21:28

Fix pagination behavior and info display

1f589ea

Fix pagination behavior and info display

a4ff18c

Ivadriil requested a review from mateacademy-ai-mentor April 25, 2026 18:36

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

View reviewed changes

		className={cn('page-item', { disabled: currentPage === 1 })}
		onClick={() => onPageChange(currentPage <= 1 ? 1 : currentPage - 1)}

		onClick={() =>
		onPageChange(currentPage >= pageCount ? pageCount : currentPage + 1)

Conversation

Ivadriil commented Apr 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!

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!

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