-
Notifications
You must be signed in to change notification settings - Fork 289
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
Add related posts links to post page #119
Conversation
src/lib/posts.js
Outdated
* getRelatedPosts | ||
*/ | ||
|
||
export async function getRelatedPosts({ categories, postId }, count = 5) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since we're only ultimately getting the first instance of the category here, do you think it might make more sense to make this category
instead of categories
? it might make it more predictable, then when calling it, the callter would use categories[0]
looking below, looks like it was based off of just passing post
in, but i think that could make it a little more clear what's happening. thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, that makes totally sense.
When passing post
object (and its categories
) I was thinking that maybe in the future would be interesting to:
- Check if
categories[0]
have at least 5 posts - In case relatedPosts < 5 and
categories[1]
exists , fill in the rest withcategories[1]
- Update the title to have the two categories
But the way you suggest is better, because it keeps the funcionality very clear (get related posts for one category) and makes it very reusable. And in case of adding extra complexity, it can be placed outside (checking relatedPosts count, calling the function for another category...). Will make the changes, thanks @colbyfayock !!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, i think thats a cool idea though! i think we'd want to think about how that would work like title and such, maybe another Issue we can brainstorm on
src/pages/posts/[slug].js
Outdated
<span> | ||
More from{' '} | ||
<Link href={categoryPathBySlug(categories[0].slug)}> | ||
<a>{categories[0].name}</a> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
trying to think about how relatedPosts
is created, do you think it might make sense to have an object passed into the component that represents both an array of posts and the title? so that you're not inferring the association? thinking in terms of using the functionality elsewhere
for instance, this can be as simple as when passing in relatedPosts
in getStaticProps
, something like:
relatedPosts: {
posts: getRelatedPosts({ category: post.categories[0].slug }), // based on earlier thoughts
title: post.categories[0].name
}
thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is a great idea, making the changes!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@colbyfayock Just one doubt about this: the first version has a link to the category (in the title), so is required to pass at getStaticProps
at least the slug of the category or the link, something like this:
relatedPosts: {
posts: ... ,
title: {
name: post.categories[0].name
link: categoryPathBySlug(categories[0].slug)
}
}
Do you think is a good idea to include this link, or will be somehow redundant because the link is already at Metadata?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm thats a good question. id vote for the link option like the block you posted, simply because it provides someone the ability to change it to something else if they'd like 🤷 maybe someone decides to use a different pattern for linking or something, or wants to add tracking to those links, etc etc idk.
hey @GuilleAngulo thanks for grabbing this. i really like how it came together, just 2 quick thoughts on the code flow and sorry it looks like i merged some stuff in and caused you a conflict 😅 |
hahah no problem. Will update to solve the conflicts and add the changes 💪 |
713b16b
to
f086df0
Compare
Changes done! Just a couple of things I noticed to heads up:
|
hm good point, i think if we were to do that though i would want to create a new directory since /images is a common directory name, so something along the lines of i wonder if there's any good use case to keeping them around, but i can't think of anything. ill open a new ticket to think about it |
src/pages/posts/[slug].js
Outdated
relatedPosts: { | ||
posts: await getRelatedPosts(category, postId), | ||
title: { | ||
name: name || '', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we leave this as undefined
and only show More Posts
if it's not defined? rather than it showing More from
where it would include a link with no text in it, if im understanding this correctly
maybe if it's undefined More Posts
is the whole link
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see your point, I can change that and set the title with a conditional render evaluating if there is a name
. Just two considerations about that:
-
When rendering the
relatedPosts
, first check is if the array of post are not empty. And I was thinking if is possible to have a case in which there is an array of posts but the name isundefined
. This would mean that the category exists (because the relatedPosts are collected from it), but the name is missing ... is this a possibility? -
I just discovered while doing this that when returning the props at
getStaticProps
it can't have an undefined value, because it cannot be serialized. So one option is to pass it likename: name || null,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- I'm not sure it's possible to have an undefined category name, but i was thinking if someone hard coded it in there if they wanted to update the title but keep the same link 🤷
- interesting about undefined, TIL. i think the
name || null
makes sense
hey sorry one more thing i missed, if we dont have a title name defined, it would show weird |
f086df0
to
f8b86e5
Compare
src/pages/posts/[slug].js
Outdated
const { categories, postId } = post; | ||
const category = categories.length && categories[0]; | ||
let { name, slug } = category; | ||
name = undefined; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
won't this and slug override any value that comes in?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oopss sorry about that, testing undefined values 😅
Made the changes! About making |
i think we're cool with how you ahve it in there, just ignore the link if it doesnt have everything needed. if someone really wants to customize it they can hop in there |
f8b86e5
to
3f8c7db
Compare
Perfect! I think everything is working now 🙏 |
looks great! nice add 💪 |
Description
It adds at
Post
footer a list of related posts links (by post category).The default count of links is set to 5, but the function can receive a new count to show a custom number. The function filters the related posts to exclude the current post.
Image
Fixes #117