Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: page-number-based pagination on homepage #242

Merged
merged 4 commits into from
May 30, 2023
Merged

feat: page-number-based pagination on homepage #242

merged 4 commits into from
May 30, 2023

Conversation

iuioiua
Copy link
Contributor

@iuioiua iuioiua commented May 29, 2023

This PR:

  1. Introduces page-number-based pagination to the homepage.
  2. Refactors much of the database-related code to be more straightforward.
  3. Changes the homepage to show only the items posted in the last week.
  4. Supercedes fix: pagination order #236
  5. Moves functionality closer to proposal: complete homepage functionality #211
  6. Closes proposal: pagination with numbers  #233
  7. Closes bug: pagination seems to break the order of Items #231

Note: once merged, these same changes can be made to paginate item and user pages.

Please feel free to review, @brunocorrea23 and @huai-jie.

@huai-jie
Copy link
Contributor

Does this PR gonna tackle New votes per day functionality that is mentioned in #202?

@iuioiua
Copy link
Contributor Author

iuioiua commented May 29, 2023

Not so much. We can do that in another PR.

@brunocorrea23
Copy link
Contributor

@iuioiua , thanks for the work here and for requesting the review.
Really liked the db refactor.

Couple of comments on the proposed solution.

  • It seems that the items older than a week are never accessible to the users, right? Is this the intended behavior?
  • 'Loading' all items for the last week provides no guarantee of the amount of items that will be 'loaded'. Although it is better than loading the whole db at once, I feel this kind of defeats the purpose of pagination.

Wdyt?

@iuioiua
Copy link
Contributor Author

iuioiua commented May 30, 2023

  • It seems that the items older than a week are never accessible to the users, right? Is this the intended behavior?

Yes, as it stands now. Later, we can add functionality to make the window customisable.

  • 'Loading' all items for the last week provides no guarantee of the amount of items that will be 'loaded'. Although it is better than loading the whole db at once, I feel this kind of defeats the purpose of pagination.

True, the amount of loading is sub-optimal. This will be fixed if/when Deno.Kv.count() is implemented.

Does the PR look good to you?

@brunocorrea23
Copy link
Contributor

ok, thanks for the clarification!

LGTM

@iuioiua iuioiua merged commit 6a3357a into main May 30, 2023
@iuioiua iuioiua deleted the pagination branch May 30, 2023 01:42
@iuioiua iuioiua mentioned this pull request May 30, 2023
@iuioiua iuioiua changed the title feat: page-number-based pagination feat: page-number-based pagination on homepage May 30, 2023
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.

proposal: pagination with numbers bug: pagination seems to break the order of Items
3 participants