Solution1 by SerMamchur · Pull Request #1482 · mate-academy/react_pagination

SerMamchur · 2026-04-22T08:23:44Z

https://SerMamchur.github.io/react_pagination/

mateacademy-ai-mentor

Review Summary

I am requesting changes because the core Pagination requirements from the task are not yet met, which will block tests and grading. The Pagination component’s API is incorrect: it currently takes pages, page, updateParams, handlePgeChange, lastPage, currentPage, but the task explicitly requires a stateless <Pagination total perPage currentPage onPageChange /> API; this must be refactored so the props are exactly { total: number; perPage: number; currentPage?: number; onPageChange: (page: number) => void }. Related to that, Pagination currently updates URL/search params itself via updateParams, which violates the “stateless component” requirement; it should only call onPageChange(newPage) when the page actually changes, and App.tsx should handle URL updates and pass total, numeric perPage, numeric currentPage, and the onPageChange callback. Inside Pagination, compute the number of pages with Math.ceil(total / perPage), render page numbers 1..N, mark the active page with li.active, and ensure prev/next use li.disabled and aria-disabled={true} when at the bounds. Also remove the unnecessary value={page} on each li and rely on currentPage === pageNumber for comparisons, and fix the typo handlePgeChange → handlePageChange or directly use onPageChange to match the required API. Once you make these changes, your existing URL sync and info text logic in App.tsx should fit better with the spec, and your solution will align well with the checklist and tests—your current work on per-page selection, info display, and disabling behavior is already a solid base to build on.

✨ 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-22T08:27:14Z

 import { createRoot } from 'react-dom/client';
+import React from 'react';

 import { App } from './App';


The App component uses prop names and prop types for Pagination that do not match the task description. The task required a stateless API. Here the Pagination is used with props pages, page, updateParams, handlePgeChange, lastPage, currentPage. This means the implementation does not follow the required component API from the description and checklist. Please adapt to accept total, perPage, optional currentPage (default 1), and onPageChange callback instead of the current custom props.

mateacademy-ai-mentor · 2026-04-22T08:27:14Z

 import React from 'react';
 import './App.css';
 import { getNumbers } from './utils';
+import { Pagination } from './components/Pagination';


The App component uses search params to manage page and perPage, but the task requires a stateless Pagination component that accepts props: total, perPage, currentPage (optional), and onPageChange. Here Pagination's props are different (pages, page, updateParams, handlePgeChange, lastPage, currentPage). This does not match the required API from the description — update the Pagination to accept the specified props (total, perPage, currentPage, onPageChange) or adjust the App to pass exactly those props as described in the task. (See checklist Core Functional Requirements.)

mateacademy-ai-mentor · 2026-04-22T08:27:14Z

+    page: '1',
+  });
+
+  const pages: number[] = [];


The Pagination component must be stateless and only show items for the current page via the props described in the task. Currently Pagination receives a prebuilt pages array and also directly calls updateParams — this couples it to URL logic. The component should not mutate URL directly; instead it must call onPageChange(newPage) when page changes. Please change the component to call onPageChange and remove direct URL updates. This violates requirements: 'stateless component' and 'onPageChange must be called only if page was changed'.

mateacademy-ai-mentor · 2026-04-22T08:27:15Z

+  for (let i = 1; i <= totalPage; i++) {
+    pages.push(i);
+  }
+
+  const updateParams = (key: string, value: string) => {


The task expects the Pagination to have data-cy attributes and specific structure for prev/next disabling using li.disabled and a[aria-disabled="true"]. While your code sets li class and aria-disabled, the aria-disabled attribute is assigned string 'true'/'false' — React will render these correctly, but better to pass boolean for aria-disabled (e.g. aria-disabled={currentPage === 1}) to match semantic usage. Consider using boolean values for aria-disabled for clarity.

mateacademy-ai-mentor · 2026-04-22T08:27:15Z

    <div className="container">
      <h1>Items with Pagination</h1>


In Pagination mapping, each page
uses value={page} (line 47) and the component prop page is a string. The value attribute on li isn't required and it's incorrect to assign the whole current page string to each list item. Remove value={page} or replace with a meaningful attribute (not required). This is unnecessary and may confuse logic.

mateacademy-ai-mentor · 2026-04-22T08:27:15Z

+type Props = {
+  pages: number[];
+  page: string;
+  updateParams: (key: string, value: string) => void;
+  handlePgeChange: (value: number) => void;
+  lastPage: boolean;
+  currentPage: number;
+};
+
+export const Pagination: React.FC<Props> = ({


The Pagination component's props and behavior don't match the task description. The description requires a stateless Pagination that accepts props: total, perPage, currentPage (optional default 1), and onPageChange (callback). In this file the component expects: pages, page (string), updateParams, handlePgeChange, lastPage, and currentPage. This diverges from the required API and shifts responsibility for computing pages and updating URL into Pagination rather than App. Please rework the component signature to match the required props: total: number, perPage: number, currentPage?: number, onPageChange: (page:number) => void and remove URL/updateParams logic from Pagination.

mateacademy-ai-mentor · 2026-04-22T08:27:15Z

+          <li
+            value={page}
+            className={classNames('page-item', {
+              active: Number(page) === pageNumber,
+            })}
+            key={pageNumber}


li elements for individual pages set value={page} where page is a string prop for the current page. That is incorrect: the value attribute on each page li should not be the current page string. If you need to mark the list item you should not use value, and ensure li.active is set based on the currentPage prop (number). Also the comparison uses Number(page) === pageNumber which relies on a string prop; with the corrected API it should compare currentPage === pageNumber.

mateacademy-ai-mentor · 2026-04-22T08:27:15Z

+            <a
+              data-cy="pageLink"
+              className="page-link"
+              href={`#${pageNumber}`}
+              onClick={e => {
+                e.preventDefault();
+                updateParams('page', String(pageNumber));
+              }}


The component uses updateParams('page', String(pageNumber)) inside page link clicks and aria-disabled with string 'true'/'false'. The Pagination should be stateless and not manipulate URL params directly; instead it must call onPageChange(pageNumber) only when the page actually changes. Move URL/searchParams handling into App and ensure onPageChange is invoked only when page differs from current value.

mateacademy-ai-mentor · 2026-04-22T08:27:15Z

+        <a
+          data-cy="prevLink"
+          className="page-link"
+          href="#prev"
+          aria-disabled={currentPage === 1 ? 'true' : 'false'}
+          onClick={e => {
+            e.preventDefault();
+            if (currentPage > 1) {
+              handlePgeChange(currentPage - 1);
+            }
+          }}
+        >
+          «
+        </a>


The prev and next links currently call handlePgeChange (note typo) for navigation but App already provides handlePgeChange name. However the overall design still couples URL updates into Pagination via updateParams earlier. After aligning props to the required API, ensure prev/next call onPageChange(currentPage - 1) and onPageChange(currentPage + 1) only when changes occur, and ensure li.disabled and a[aria-disabled="true"] are applied when first/last page respectively.

mateacademy-ai-mentor · 2026-04-22T08:27:15Z

+export const Pagination: React.FC<Props> = ({
+  pages,
+  page,
+  updateParams,
+  handlePgeChange,
+  lastPage,
+  currentPage,
+}) => {


The component currently receives page as a string and currentPage as a number in props. This duplication is confusing and error prone. With the required API use a single numeric currentPage prop (default 1) and remove page string prop.

mateacademy-ai-mentor

Review Summary

This implementation requires changes to pass the tests. Two critical issues must be fixed:

Missing data-cy attributes — All 25 tests fail because the rendered elements lack required test identifiers. You must add data-cy="item" to page number <li> elements, data-cy="pageLink" to page number links, data-cy="prevLink" and data-cy="nextLink" to navigation buttons, and data-cy="perPageSelector" to the dropdown.
Incorrect Pagination props interface — The component still accepts 7 props (page, handlePgeChange, lastPage, currentPage) when only 4 are required: total, perPage, currentPage, and onPageChange. The typo handlePgeChange must be removed, and value={page} on <li> elements should be removed since active state is determined by currentPage === pageNumber.

Fix these issues and your tests should pass.

✨ 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-24T07:17:11Z

+import { Pagination } from './components/Pagination';
+import { useSearchParams } from 'react-router-dom';

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

 export const App: React.FC = () => {
+  const [searchParams, setSearchParams] = useSearchParams({


Props type includes unnecessary fields. Required API: { total: number; perPage: number; currentPage?: number; onPageChange: (page: number) => void }. Remove page, handlePgeChange, lastPage, and currentPage from props.

mateacademy-ai-mentor · 2026-04-24T07:17:11Z


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



Typo in function name. If keeping a separate handler, rename to handlePageChange, but ideally just use onPageChange directly to match the required API.

mateacademy-ai-mentor · 2026-04-24T07:17:11Z

+    page: '1',
+  });
+
+  const pages: number[] = [];
+
+  const page = searchParams.get('page') || '1';
+  const perPage = searchParams.get('perPage') || '5';
+
+  const totalPage = Math.ceil(items.length / Number(perPage));


Remove page prop - the component should only use currentPage for comparisons. Also remove handlePgeChange and lastPage props - compute these internally.

mateacademy-ai-mentor · 2026-04-24T07:17:11Z

      <p className="lead" data-cy="info">
-        Page 1 (items 1 - 5 of 42)
+        Page {page} (items {startIndex + 1} - {endIndex} of {items.length})
      </p>


The value={page} attribute on <li> is unnecessary. Rely on currentPage === pageNumber for the active state comparison on line 53.

mateacademy-ai-mentor · 2026-04-24T07:17:11Z

+  const handlePgeChange = (newPage: number) => {
+    if (newPage !== currentPage) {
+      updateParams('page', String(newPage));


Instead of calling handlePgeChange, use onPageChange(currentPage - 1) directly. The onPageChange prop is already the required callback.

mateacademy-ai-mentor · 2026-04-24T07:17:12Z

+              const params = new URLSearchParams();
+
+              params.set('perPage', e.target.value);


Use onPageChange(pageNumber) directly instead of a separate handlePgeChange function. Only call if pageNumber !== currentPage to avoid unnecessary callbacks.

mateacademy-ai-mentor · 2026-04-24T07:17:12Z

+        perPage={Number(perPage)}
+        onPageChange={newPAge => {
+          updateParams('page', String(newPAge));


Instead of lastPage prop, compute internally: const lastPage = currentPage === totalPage. Use onPageChange(currentPage + 1) directly.

mateacademy-ai-mentor · 2026-04-24T07:17:12Z

+
+  for (let i = 1; i <= totalPage; i++) {
+    pages.push(i);
+  }


These lines can be removed since Pagination should be stateless and compute pages internally. Keep only the variables needed for rendering items and info display.

mateacademy-ai-mentor · 2026-04-24T07:17:12Z

+
+  const handlePgeChange = (newPage: number) => {
+    if (newPage !== currentPage) {
+      updateParams('page', String(newPage));
+    }
+  };


Remove handlePgeChange function - App should only need onPageChange passed to Pagination. The check for newPage !== currentPage is also redundant since Pagination should only call onPageChange when page actually changes.

mateacademy-ai-mentor · 2026-04-24T07:17:12Z

+      <Pagination
+        total={items.length}
+        perPage={Number(perPage)}
+        onPageChange={newPAge => {
+          updateParams('page', String(newPAge));
+        }}
+        page={page}
+        handlePgeChange={handlePgeChange}
+        lastPage={lastPage}
+        currentPage={currentPage}


Pagination should only receive the 4 required props: total, perPage, currentPage, and onPageChange. Remove page, handlePgeChange, lastPage, and currentPage from the JSX.

mateacademy-ai-mentor · 2026-04-24T07:17:16Z

+
+  const totalPage = Math.ceil(items.length / Number(perPage));
+  const currentPage = Number(page);
+  const lastPage = currentPage === totalPage;
+
+  const startIndex = (currentPage - 1) * Number(perPage);
+  const endIndex = Math.min(Number(perPage) + startIndex, items.length);


Keep currentPage and startIndex/endIndex for slicing items and displaying info. The totalPage can be removed since Pagination computes it internally.

mateacademy-ai-mentor · 2026-04-24T07:17:16Z

 import { App } from './App';
+import { HashRouter } from 'react-router-dom';

-createRoot(document.getElementById('root') as HTMLElement).render(<App />);
+createRoot(document.getElementById('root') as HTMLElement).render(
+  <HashRouter>
+    <App />
+  </HashRouter>,
+);


Props type includes wrong props. Task requires only: total, perPage, currentPage (optional), and onPageChange. Remove page, handlePgeChange, lastPage, currentPage.

mateacademy-ai-mentor · 2026-04-24T07:17:16Z


 import { App } from './App';
+import { HashRouter } from 'react-router-dom';



Remove this prop - it's not part of the required API. The component should use currentPage prop instead.

mateacademy-ai-mentor · 2026-04-24T07:17:16Z


-createRoot(document.getElementById('root') as HTMLElement).render(<App />);
+createRoot(document.getElementById('root') as HTMLElement).render(
+  <HashRouter>


Remove this prop - it's not part of the required API. Use onPageChange instead.

mateacademy-ai-mentor · 2026-04-24T07:17:16Z

-createRoot(document.getElementById('root') as HTMLElement).render(<App />);
+createRoot(document.getElementById('root') as HTMLElement).render(
+  <HashRouter>
+    <App />


Remove this prop - it's not part of the required API. Compute disabled state internally as currentPage === totalPage.

mateacademy-ai-mentor · 2026-04-24T07:17:16Z

+type Props = {
+  total: number;
+  perPage: number;
+  page: string;
+  onPageChange: (page: number) => void;
+  handlePgeChange: (value: number) => void;
+  lastPage: boolean;
+  currentPage: number;


Props type still contains incorrect props not in the required API: page, handlePgeChange, lastPage. The required interface is only { total: number; perPage: number; currentPage?: number; onPageChange: (page: number) => void }. Remove all non-standard props.

mateacademy-ai-mentor · 2026-04-24T07:17:16Z

+export const Pagination: React.FC<Props> = ({
+  total,
+  perPage,
+  page,
+  onPageChange,
+  handlePgeChange,
+  lastPage,
+  currentPage,


Destructuring still includes non-standard props. Only destructure total, perPage, currentPage, and onPageChange. The component should be stateless and only call onPageChange when the page changes.

mateacademy-ai-mentor · 2026-04-24T07:17:16Z

+
+      {pages.map(pageNumber => {
+        return (
+          <li


Remove this value={page} attribute - <li> elements don't need a value prop. Use currentPage === pageNumber to determine active state instead.

mateacademy-ai-mentor · 2026-04-24T07:17:16Z

+          onClick={e => {
+            e.preventDefault();
+            if (currentPage > 1) {
+              handlePgeChange(currentPage - 1);


The typo handlePgeChange should be removed. Use only onPageChange which is the required callback prop.

mateacademy-ai-mentor · 2026-04-24T07:17:16Z

+            e.preventDefault();
+            if (currentPage > 1) {
+              handlePgeChange(currentPage - 1);


Pagination should only call onPageChange(currentPage - 1) here. Since currentPage is a prop (not derived internally), remove handlePgeChange usage entirely.

mateacademy-ai-mentor · 2026-04-24T07:17:20Z

+                e.preventDefault();
+                onPageChange(pageNumber);


Use only onPageChange(pageNumber) here. No other callback prop is needed.

mateacademy-ai-mentor · 2026-04-24T07:17:20Z

+            if (!lastPage) {
+              handlePgeChange(currentPage + 1);


The lastPage prop is not in the required API. Compute it internally as currentPage === totalPage or derive it from currentPage prop. Remove handlePgeChange and use only onPageChange.

mateacademy-ai-mentor · 2026-04-24T07:17:20Z

+          onClick={e => {
+            e.preventDefault();
+
+            if (!lastPage) {
+              handlePgeChange(currentPage + 1);
+            }
+          }}
+        >
+          »
+        </a>


Pass only the required props: total, perPage, currentPage, and onPageChange. Remove page, handlePgeChange, and lastPage.

mateacademy-ai-mentor · 2026-04-24T07:17:21Z

+          onClick={e => {
+            e.preventDefault();
+
+            if (!lastPage) {


Fix the callback parameter name typo newPAge to newPage for consistency.

mateacademy-ai-mentor

Review Summary

This is an educational override to help you progress while learning. You've done well implementing the 4-props interface correctly, URL persistence with React Router, and most data-cy attributes. However, there are two issues that need fixing before full test compliance: 1) The page number <li> elements in Pagination.tsx are missing data-cy="item" - add this attribute to each <li> wrapping page number links; 2) Remove value={page} from <li> elements in both App.tsx and Pagination.tsx - this prop is non-standard for <li> and unnecessary since active state is handled via className. Once you add the missing data-cy="item" attribute and remove the value props, your tests should pass. Keep up the good work on the overall architecture!

✨ 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-24T08:52:30Z

+            className="form-control"
+            value={perPage}
+            onChange={e => {
+              const params = new URLSearchParams();


Remove the value={page} attribute from the <li> element. The value prop is not valid for <li> elements in React, and the active state is already correctly handled via the className conditional on the next line.

mateacademy-ai-mentor · 2026-04-24T08:52:30Z

+        return (
+          <li


Missing data-cy="item" attribute on page number <li> elements. The tests expect data-cy="item" on the <li> tags that represent page numbers, not just on the <a> tags.

mateacademy-ai-mentor · 2026-04-24T08:52:30Z

+
+      {pages.map(pageNumber => {
+        return (
+          <li


Remove value={page} from <li> element. The value attribute is unnecessary here since active state is determined by Number(page) === pageNumber in className. This may cause unexpected behavior.

2pasha

good job! 👏

try to pass all tests

take a look on comments below ⬇️

2pasha · 2026-04-25T07:58:55Z

+          onClick={e => {
+            e.preventDefault();
+            if (currentPage > 1) {
+              onPageChange(currentPage - 1);
+            }
+          }}


move to separate fn

2pasha · 2026-04-25T07:59:04Z

+              onClick={e => {
+                e.preventDefault();
+                onPageChange(pageNumber);


move to separate fn

2pasha · 2026-04-25T07:59:12Z

+          onClick={e => {
+            e.preventDefault();
+
+            if (!lastPage) {
+              onPageChange(currentPage + 1);
+            }
+          }}


move to separate fn

2pasha · 2026-04-25T07:59:30Z

+            onChange={e => {
+              const params = new URLSearchParams();
+
+              params.set('perPage', e.target.value);
+              params.set('page', '1');
+
+              setSearchParams(params);
+            }}


move to separate fn

Anton-Kuchmasov

Your logic is almost correct, but please try to pass all tests before your solution will be approved

vadiimvooo

Well done

Solution1

a3621dd

mateacademy-ai-mentor suggested changes Apr 22, 2026

View reviewed changes

solution2

5072495

SerMamchur requested a review from mateacademy-ai-mentor April 24, 2026 07:13

mateacademy-ai-mentor suggested changes Apr 24, 2026

View reviewed changes

mateacademy-ai-mentor reviewed Apr 24, 2026

View reviewed changes

solution2

92ed648

SerMamchur requested a review from mateacademy-ai-mentor April 24, 2026 08:50

mateacademy-ai-mentor suggested changes Apr 24, 2026

View reviewed changes

last Solution

31ab370

SerMamchur requested a review from mateacademy-ai-mentor April 25, 2026 05:47

2pasha suggested changes Apr 25, 2026

View reviewed changes

last

5c68ee3

SerMamchur requested a review from 2pasha April 25, 2026 09:46

Anton-Kuchmasov suggested changes Apr 25, 2026

View reviewed changes

changed jsons

91c32f4

SerMamchur requested a review from Anton-Kuchmasov April 25, 2026 19:09

vadiimvooo approved these changes Apr 25, 2026

View reviewed changes


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

		const params = new URLSearchParams();

		params.set('perPage', e.target.value);


		import { App } from './App';
		import { HashRouter } from 'react-router-dom';

Conversation

SerMamchur commented Apr 22, 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!

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!

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!

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