chore: Add pagination #103
Conversation
Eghizio
left a comment
Eghizio
left a comment
There was a problem hiding this comment.
Try to decouple it and separate some components. It is hard to read and understand what is going on in this component.
Also try to name those magic numbers as it is not explicitly obvious what are those and what are they responsible for. It's hard for me to understand the logic behind this pagination by looking at this code. Splitting it into smaller and named parts will help for sure 😉
Maybe create a couple pagination components that are displayed based on the pages amount etc. This may result in some code duplication but will increase code readability a lot. But first I'd give a try the things I mentioned above. And if it still remains complex enough, then it can be thought about such distinct components 👍
| ? Number(router.query.page) | ||
| : 1; | ||
| const perPage = | ||
| router.query.page && !isNaN(Number(router.query.limit)) |
There was a problem hiding this comment.
shouldn't that be check for router.query.limit? Just asking coz I don' really know 😅
Also I think it could be somehow extracted to some function as it is repeated two times here and kinda looks confusing even tho it just is some validation/parsing/default functionality.
| <div className="flex items-center justify-between border-t border-gray-200 bg-white px-4 py-3 sm:px-6"> | ||
| <div className="flex flex-1 justify-between sm:hidden"> | ||
| <a | ||
| href={`?page=${currentPage - 1}`} |
| Previous | ||
| </a> | ||
| <a | ||
| href={`?page=${currentPage + 1}`} |
| </span> | ||
| - | ||
| <span className="font-medium"> | ||
| {currentPage !== totalPages ? currentPage * perPage : quantity} |
There was a problem hiding this comment.
What is this comment? Explain your thoughts please, give others a chance to learn
| </a> | ||
| )} | ||
| {Array.from({ length: totalPages }, (_, i) => ( | ||
| <a |
| </a> | ||
| ))} | ||
| {totalPages > 5 && ( | ||
| <span className="relative inline-flex items-center px-4 py-2 text-sm font-semibold text-gray-700 ring-1 ring-inset ring-gray-300 focus:outline-offset-0"> |
| ... | ||
| </span> | ||
| )} | ||
| {totalPages > 1 && ( |
There was a problem hiding this comment.
This seems as repeated. Maybe it would be better to make a couple different components for different pagination scenarios instead of making so many cases here. It is kinda very hard to read.
It is okay to duplicate code sometimes or to make 2 or 3 similar components that are simpler, if they do differ than trying to make them one generic with higher complexity and less readability.
Don't be afraid to create smaller components 😉
| const perPage = | ||
| router.query.page && !isNaN(Number(router.query.limit)) | ||
| ? Number(router.query.limit) | ||
| : 11; |
There was a problem hiding this comment.
Could be moved to constant SOMETHING_NUMBER
There was a problem hiding this comment.
What is this number "11" ?
It's default quantity of items per page
3 participants
No description provided.