open-sauced / hot

πŸ•The site that recommends the hottest projects on GitHub.
https://hot.opensauced.pizza
MIT License
418 stars 145 forks source link

fix: hide load more button when no more data is available #448

Closed imvedanshmehra closed 1 year ago

imvedanshmehra commented 1 year ago

What type of PR is this? (check all applicable)

Description

This PR fixes issue #446

Related Tickets & Documents

Mobile & Desktop Screenshots/Recordings

Added tests?

Added to documentation?

[optional] Are there any post-deployment tasks we need to perform?

[optional] What gif best describes this PR or how it makes you feel?

netlify[bot] commented 1 year ago

Deploy Preview for hot-sauced-ui ready!

Name Link
Latest commit 7a4537bb3748adae7fddb1ad5c2481aee12fd61d
Latest deploy log https://app.netlify.com/sites/hot-sauced-ui/deploys/63ebeb8d086e230008a92094
Deploy Preview https://deploy-preview-448--hot-sauced-ui.netlify.app/
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

takanome-dev commented 1 year ago

Thanks for your contribution @imvedanshmehra :pizza:

Seems like the button is still visible :thinking: I noticed that there is no more data after limit === 50. Updating the condition should fix this issue.

{fetchedData.length > 0 && activeLink !== "myVotes" && limit < 50 && (//render btn)}

Another thing I have noticed, maybe not related to this issue, the limit in the search params is different from the one sent to the API even after removing if (value > 25) return 50 :thinking: (see screenshot below)

Screenshot from 2023-02-14 22-35-35

Any idea of what's causing this @0-vortex?

0-vortex commented 1 year ago

Yes, this is not how limit works in general ... API has to provide a totalPages type of variable or an itemCount like ours. Fetched data length will always be limit πŸ•

imvedanshmehra commented 1 year ago

Hi @TAKANOME-DEV, the Load More button is now visible only when we have some data.

We can set this condition limit <= 50 only if we are sure that the API will not return more than 50 data. Otherwise, we might not have any way to load past 50th data.

Regarding the second issue that the limit in search params is different than what we are sending to the API. I checked that locally it seems fine πŸ€” (Please check the screenshot attached below).

Screenshot 2023-02-18 at 11 58 22 AM
takanome-dev commented 1 year ago

We can set this condition limit <= 50 only if we are sure that the API will not return more than 50 data. Otherwise, we might not have any way to load past 50th data.

Yeah, also the API returns hasNextPage in the meta object. So the button should be hidden if that property is false. I checked that locally it seems fine.

Weird :thinking: Try it with the deploy preview https://deploy-preview-448--hot-sauced-ui.netlify.app/

imvedanshmehra commented 1 year ago

I noticed that 50 is the maximum limit we can pass to the limit query params, which means that once we have fetched all the 50 items hasNextPage in the meta should be set to false but was still true somehow. Therefore, if we relying on hasNextPage to show/hide the Load More button, it will always be shown even though we have no more data to fetch.

Screenshot 2023-02-19 at 12 32 26 PM Screenshot 2023-02-19 at 12 32 52 PM
takanome-dev commented 1 year ago

I noticed that 50 is the maximum limit we can pass to the limit query params, which means that once we have fetched all the 50 items hasNextPage in the meta should be set to false but was still true somehow.

True, there should be a reason why the limit is fixed. This pr should be good to go if you do the changes mentionned by o-vortex above and hide the button if limit <= 50 :pizza:

0-vortex commented 1 year ago

I noticed that 50 is the maximum limit we can pass to the limit query params, which means that once we have fetched all the 50 items hasNextPage in the meta should be set to false but was still true somehow.

True, there should be a reason why the limit is fixed.

This pr should be good to go if you do the changes mentionned by o-vortex above and hide the button if limit <= 50 :pizza:

I'm sorry but no, this is not the solution. Unless you agree to correctly implement meta, there is no incremental improvement we can do.

The load more button should disappear when the page is equal to the last page or hasNextPage is false. Has nothing to do with limit, it's just the number of items we display per page.

takanome-dev commented 1 year ago

I'm sorry but no, this is not the solution.

Oh my bad, thanks for clarifying this :+1: Unless you agree to correctly implement meta, there is no incremental improvement we can do.

I'll look into it, but can't promise anything

OgDev-01 commented 1 year ago

Hey @imvedanshmehra How is this going? Have you been able to pull through?

bdougie commented 1 year ago

We will need to approach this differently. Closing for now.