feat: search results pagination #361
Conversation
|
Closes #369 |
|
Please, always add a screenshot when your PR is releated to UI stuff. |
|
new translation keys are required for next_page and last_page
|
|
Could you please add the translation for LGTM overall, nice work :) |
|
@mohannadzidan u can use it actually the only problem is that this file uses |
|
I have updated the code to use
|
|
@zamitto hey, this feature is ready to merge, can you please review the code? |
|
hey @mohannadzidan , need some help here? |
|
I've implemented the changes requested in the previous review:
Please take another look at the code and let me know if everything looks good or if you have any further suggestions. Thanks! |
| @@ -16,6 +20,8 @@ import { useNavigate, useSearchParams } from "react-router-dom"; | |||
| import * as styles from "./home.css"; | |||
| import { buildGameDetailsPath } from "@renderer/helpers"; | |||
|
|
|||
| const PAGE_SIZE = 12; | |||
There was a problem hiding this comment.
why set the pagesize here if you already set the default on the function itself to 12?
There was a problem hiding this comment.
I made it like this to increase the code readability and avoid hard coding magic numbers in the code while calculating whether this is the last results page or not
For example if a new contributor saw this line
setIsEnd(results.length < 12)He will be confused and wonder what does this number 12 mean and why we put it here, right? But by declaring this constant and use it properly in the query it is clearer and will make since to him
There was a problem hiding this comment.
but you can fetch this number from the search params the same way you does to get the page no?
|
|
||
| const DEFAULT_SEARCH_ARGS: PaginationArgs = { | ||
| take: 12, | ||
| skip: 0, |
There was a problem hiding this comment.
instead of skip, you could also just pass the page as a default for zero, and calculate the skip inside the function itself, then you dont need to know the default value for take when you just wannt to go to the page like 5 using the default "take" parameter
There was a problem hiding this comment.
Actually your suggestion was exactly the previous version of the function when i modified it initially for this pagination feature, but I thought that it is more reusable like this? Like if in the future we had to use this function to display different number of results we will able to do this without revisting the code again and modify it
There was a problem hiding this comment.
+/-, like, how it is now, you need to know the take parameter to send the skip correctly considering the number of the page you want, so you basically dont make use of the default value for take, as you need to consider it anyway to send the skip.
if you send only take and page, both optional to 12 an 0 respectively, you dont need to care about any of this logic, and can even just send the page arg for any request. and you also has access to the take (default or not) used on the query to see if you are on the last page (query could also return a parameter with the number of pages tho)
what do you think @zamitto?
There was a problem hiding this comment.
I kinda agree with @piradata on this one, but the default to 12 on take I think that should be a global default value, indeed of having one here, another one in src/renderer/src/pages/home/search-results.tsx and so on.
There was a problem hiding this comment.
I kinda agree with @piradata on this one, but the default to 12 on
takeI think that should be a global default value, indeed of having one here, another one insrc/renderer/src/pages/home/search-results.tsxand so on.
yea, i thought about it being defined on the own function internally, that way it would basically be a global default, without the need to import the default and the function separately
|
@zamitto what do you think on this one? |
This PR will add pagination buttons to search results to show more search results
closes #357, and closes #292