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

Show Blog Posts from API #48

Merged
merged 10 commits into from
Sep 30, 2024
Merged

Show Blog Posts from API #48

merged 10 commits into from
Sep 30, 2024

Conversation

clementinelove
Copy link
Collaborator

@clementinelove clementinelove commented Sep 4, 2024

UPDATED 10 Sep 2024

  • Now API client is updated to use the new response format.
  • Static post related function calls removed
  • Currently the blog page only shows posts from API server, notice that old markdown post are NOT added to server yet!

Last Week Update

Remote Blogs fetched from API are now presented along with static blogs, screenshots are attached below.

Important Changes:

  • Remote blog slug property is not very useful: because we need blog ID to fetch blog content, the URL slug part must contain the blog ID i.e. something like /blog/1 rather than /blog/post/blog-1.md. One drawback is that these articles won't have a descriptive url, but I guess this is pretty much unavoidable unless our authors always give their articles a unique url path name.
  • Since these remote blogs having integer ID, I use it to differentiate the type of blog the slug is referring to (i.e. integer means it's a dynamic blog loaded from API, NAN means it's a static blog)

Something worth noticing:

  • Remote authors currently don't have ID. This isn't a problem though since our system doesn't support searching blogs of a specific author.
  • Currently only first author will be listed in the post content page.
Blog Index page

Below is the correctly rendered remote blog post at /blog/1
Rendered Remote Blog Post

Other changes:

  • Commit 0188a72 update BlogTile UI so only the upper corners of the header image are rounded;
  • Commit 21bc201 fix author alignment with author bio inside blog post (AuthorTile), previously it was poorly styled, which made it look like capsule shape sometimes; author placeholder is also added
Screenshot 2024-09-05 at 00 25 19

- use `faUserCircle` as author profile image placeholder when profile image src is an empty string
- center author profile image to <title + description>: this design doesn't work for very long author bio/description, but works great for current authors.
- loads blog posts from both: 1. blog API; 2. static markdown pages.
@clementinelove clementinelove self-assigned this Sep 4, 2024
Copy link

vercel bot commented Sep 4, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
now-u-web ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 30, 2024 8:05pm

@clementinelove
Copy link
Collaborator Author

clementinelove commented Sep 4, 2024

Please have a look at this draft and branch blog-from-api when you are free! @JElgar

@JElgar
Copy link
Member

JElgar commented Sep 9, 2024

This is awesome thank you! 2 things though:

  1. I don't think we need to handle the static blogs. We can just move all of those blogs into the API and then use that directly
  2. For the blog id thing, Ive updated the API so the get API uses the slug. Blog writers are already required to provide a unique slug so that should be fine

- Remove static post related functions
- Update API Client to use new API format
@clementinelove
Copy link
Collaborator Author

clementinelove commented Sep 10, 2024

API client is now updated accordingly. Static Post related API calls are removed.
Notice that old markdown post files are not removed, and they are NOT added to server yet!

@clementinelove clementinelove marked this pull request as ready for review September 10, 2024 20:00
Comment on lines 22 to 34
export async function getStaticPostSlugs(): Promise<string[]> {
const fileNames = fs.readdirSync(POSTS_FILE_PATH);
return fileNames.map((fileName) => fileName.replace(".md", ""));
}

export async function getPostBySlug(slug: string): Promise<Post> {
/**
* Get a static post using slug.
* @param slug URL slug string. Should be a non-numerical string.
*/
export async function getStaticPostBySlug(slug: string): Promise<Post> {
const readFile = fs.readFileSync(`${POSTS_FILE_PATH}/${slug}.md`, "utf-8");
const {
data: {
Copy link
Member

Choose a reason for hiding this comment

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

Should we remove all of this static stuff now? (And update wording of "remote" stuff to just blog/post

* Get a remote blog post using slug.
* @param slug URL slug string that denotes a remote blog integer ID.
*/
export async function getDynamicPostBySlug(slug: string): Promise<Post | undefined> {
Copy link
Member

Choose a reason for hiding this comment

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

And then just expose this directly?

</div>
</div>
);
}

// TODO Type this properly
export async function generateStaticParams(): Promise<any> {
Copy link
Member

@JElgar JElgar Sep 17, 2024

Choose a reason for hiding this comment

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

Im still undecided whether removing this is the right plan.

The nice thing about it is we can render blogs as soon as they get released on the website. If we keep this then we have to do some sort of build everytime we release a blog which is a bit of a pain.

The annoying thing is

  1. We have to fetch the blog each time someone goes to the site (we can solve this with some caching below) but this is a very small issue
  2. If we are going to build a site map (which it sounds like we want) then for blogs I assume we will want to know all of them at build time (so we can add all the blogs in the sitemap?)

What do you think makes sense?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I suppose for now we can do it in two ways. One is to fetch everything dynamically (which is basically what this pull request does). I assume you worried about too much API calls right? We can definitely reduce API calls, by:

  1. having next.js cache accessed webpages (request results) for a fixed period (see https://nextjs.org/docs/app/api-reference/functions/fetch#optionsnextrevalidate), set it to cache blog posts for a day seems reasonable to me (which is not added to this pull request yet), so that the number of API calls = the number of times Next.js request new data for cache purposes.
  2. Same thing applies to sitemaps: we can have next.js dynamically generate a new site map after a fixed period passed.

I think this is the better way to do it. We can do some tests to see if the backend calls reduced after we added dynamic content cache to our project.

As of the second approach: we can also do it fully statically by only fetching data once at build time. I imagine you can add something (called webhooks?) to the backend so that any update to the blogs (or any other part of the backend data) to trigger a new frontend build, which should work just fine if we don't update blogs very frequently. The benefit of this is that we can render the whole website as a static website rather than server-side rendering, in theory because we can distribute static content via CDN the performance would be better. Then again, it will probably take more time to build considering this is an architecture change.

Let me know if there is anything need to be clarified.

@clementinelove clementinelove merged commit 97a4ccd into dev Sep 30, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants