Skip to content

add task solution#1476

Open
naviailpach wants to merge 3 commits into
mate-academy:masterfrom
naviailpach:develop
Open

add task solution#1476
naviailpach wants to merge 3 commits into
mate-academy:masterfrom
naviailpach:develop

Conversation

@naviailpach
Copy link
Copy Markdown

Copy link
Copy Markdown

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

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 pass so you can keep progressing while you learn, even though there is a small edge-case to address. You did a solid job implementing a stateless Pagination component with the required props, preserving all data-cy attributes, and wiring it up to React Router so that page and perPage are kept in the URL while correctly handling active and disabled states. The main issue to fix next is in src/components/Pagination/Pagination.tsx, where safePage is currently computed as Math.min(currentPage, totalPages); this prevents values above the last page but still allows invalid values below 1 from the URL (e.g. ?page=-1), which breaks the expectation that pages are within [1, totalPages] [CHECKLIST ITEM #13]. To fix this, clamp from both sides, for example by changing that line to something like const safePage = Math.max(1, Math.min(currentPage, totalPages));, ensuring the page index is always valid. As a minor improvement, consider adding aria-current="page" to the active page link for better accessibility, and optionally preserving unrelated query parameters when calling setSearchParams so you don’t overwrite other filters or state. Overall, your structure, typing, and behavior are on the right track, and with these small refinements you’ll have a very robust and production-ready pagination 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

Comment on lines +17 to +18
}) => {
const totalPages = Math.max(1, Math.ceil(total / perPage));
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Right now safePage is computed as Math.min(currentPage, totalPages), which prevents pages beyond totalPages but allows values less than 1 (e.g. ?page=-1). Clamp the page to at least 1 as well. Consider: const safePage = Math.max(1, Math.min(currentPage, totalPages)); This ensures safePage is always within [1, totalPages].

Copy link
Copy Markdown

@Anton-Kuchmasov Anton-Kuchmasov left a comment

Choose a reason for hiding this comment

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

Hey, to get this task approved, you should pass all tests

To proceed with tests on GitHub side, please add, commit & push both package.json & package-lock.json on your repo

git add .
git commit -m 'solution'
git push origin develop

Copy link
Copy Markdown

@etojeDenys etojeDenys left a comment

Choose a reason for hiding this comment

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

good job

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.

4 participants