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: homepage pagination and local db seeding #221

Merged
merged 16 commits into from
May 24, 2023

Conversation

zzeleznick
Copy link
Contributor

@zzeleznick zzeleznick commented May 21, 2023

Description

This PR intends to support #211 by adding simple pagination with a page URL parameters and a helper script to seed the db with some dummy data.

Adds

  • tools/seed_submissions.ts script to pull in dummy data from HN for testing
  • page option to specify the cursor (noting that there isn't yet a feed design that takes into account the date and score of the submissions)
  • More button to navigate to the next page

Example

Screen Shot 2023-05-23 at 9 30 40 PM

Notes

  • Did not add numbering as this would require passing the start index along with the cursor which seems like a good indicator for a design discussion
  • Thinking there should be an iterator or indexer by "hotness" / "recency" as part of future work

Copy link
Contributor

@iuioiua iuioiua left a comment

Choose a reason for hiding this comment

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

Nice approach! Please be sure to run deno task ok before pushing.

components/ItemSummary.tsx Outdated Show resolved Hide resolved
deno.json Outdated Show resolved Hide resolved
routes/index.tsx Outdated Show resolved Hide resolved
routes/index.tsx Outdated Show resolved Hide resolved
tools/seed_submissions.ts Show resolved Hide resolved
utils/db.ts Outdated Show resolved Hide resolved
@iuioiua
Copy link
Contributor

iuioiua commented May 22, 2023

Mostly LGTM! However, I'm now getting the following error after running deno task db:seed:

An error occurred during route handling or page rendering. TypeError: Cannot read properties of null (reading 'login')
    at Object.ItemSummary (file:///Users/asher/GitHub/saaskit/components/ItemSummary.tsx:42:23)
    at https://esm.sh/v119/preact-render-to-string@5.2.6/X-ZS8q/deno/preact-render-to-string.mjs:12:1830
    at T (https://esm.sh/v119/preact-render-to-string@5.2.6/X-ZS8q/deno/preact-render-to-string.mjs:12:1857)
    at T (https://esm.sh/v119/preact-render-to-string@5.2.6/X-ZS8q/deno/preact-render-to-string.mjs:12:1074)
    at T (https://esm.sh/v119/preact-render-to-string@5.2.6/X-ZS8q/deno/preact-render-to-string.mjs:12:2844)
    at T (https://esm.sh/v119/preact-render-to-string@5.2.6/X-ZS8q/deno/preact-render-to-string.mjs:12:2844)
    at T (https://esm.sh/v119/preact-render-to-string@5.2.6/X-ZS8q/deno/preact-render-to-string.mjs:12:1933)
    at T (https://esm.sh/v119/preact-render-to-string@5.2.6/X-ZS8q/deno/preact-render-to-string.mjs:12:1074)
    at T (https://esm.sh/v119/preact-render-to-string@5.2.6/X-ZS8q/deno/preact-render-to-string.mjs:12:1933)
    at T (https://esm.sh/v119/preact-render-to-string@5.2.6/X-ZS8q/deno/preact-render-to-string.mjs:12:1933)

I suppose these are what your previous modifications to <ItemSummary /> were for. I think we just need to fill in some gaps when populating the KV database.

@zzeleznick
Copy link
Contributor Author

@iuioiua – Yep. To minimize the diffs, I elected to make User optional as there is a flow in which a post can exist without a user in prod (e.g. user created, user posts, and then user is deleted) independently of the KV database population. LMK if this is reasonable.

@iuioiua
Copy link
Contributor

iuioiua commented May 23, 2023

The user deletion flow is not yet defined or implemented. Even if it were, it'd likely delete all objects relating to the user, including posts.

@zzeleznick
Copy link
Contributor Author

@iuioiua – for context on my end, I saw a function and a corresponding test for user deletion -> https://github.com/denoland/saaskit/blob/main/utils/db_test.ts#L46

@iuioiua
Copy link
Contributor

iuioiua commented May 23, 2023

Ah, yes. deleteUser() is currently a test function. I'll create a PR to move it to the test file. Thanks for pointing that out!

tools/seed_submissions.ts Outdated Show resolved Hide resolved
@zzeleznick
Copy link
Contributor Author

@iuioiua – Could you clarify what changes you need for this PR?

For demo or local testing purposes, I feel like emulating real users via the item.userId as a fallback for the user ID works well.

@iuioiua
Copy link
Contributor

iuioiua commented May 24, 2023

The changes to <UserPostedAt /> and <ItemSummary /> should be reverted. These components shouldn't be affected by this PR. Instead, the new seed submissions script should populate the ["users", id] index. Again, the properties in the User object when creating the user are arbitrary. So these values don't have to be based on real data if it's too annoying to do so 😄

@iuioiua
Copy link
Contributor

iuioiua commented May 24, 2023

Also, could you add a short explainer in the README (Getting Started Locally) and seed submissions script about what it does?

@zzeleznick zzeleznick force-pushed the zz/seed-submissions branch from 612776f to 7d58382 Compare May 24, 2023 04:23
@zzeleznick
Copy link
Contributor Author

@iuioiua – PTAL when you have the chance

  • reverted changes to <UserPostedAt /> and <ItemSummary />
  • created a dummy users with avatar URLs
  • updated the Getting Started Locally in README.md
  • updated the screenshot in the PR body

@iuioiua iuioiua changed the title Begin Pagination Support for Submissions feat: homepage pagination and local db seeding May 24, 2023
Copy link
Contributor

@iuioiua iuioiua left a comment

Choose a reason for hiding this comment

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

LGTM! Great work, @zzeleznick 💪🏾

@iuioiua iuioiua merged commit 26d5dff into denoland:main May 24, 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.

2 participants