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

like function implemented #68

Merged
merged 10 commits into from
Oct 5, 2022
Merged

Conversation

prateek565
Copy link
Contributor

fixes #41

@mathiasayivor
Copy link
Contributor

Sorry, I'm unavailable atm, but would review as soon as I'm back

@narayan954
Copy link
Owner

It's okay @mathiasayivor

Copy link
Contributor

@mathiasayivor mathiasayivor left a comment

Choose a reason for hiding this comment

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

What's the purpose of this though?

src/components/Post.jsx Outdated Show resolved Hide resolved
Copy link
Contributor

@mathiasayivor mathiasayivor left a comment

Choose a reason for hiding this comment

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

@prateek565 I've left some comments on the changes, but in summary:

  • Replace user.email with user.id since users can update their emails at any point in time
  • Avoid mutating component props directly; Clone/copy the prop first
  • Use updater functions to make updates reliable
  • Re-structure Post.jsx props

src/components/Post.jsx Outdated Show resolved Hide resolved
src/components/Post.jsx Outdated Show resolved Hide resolved
src/components/Post.jsx Outdated Show resolved Hide resolved
src/components/Post.jsx Outdated Show resolved Hide resolved
@prateek565
Copy link
Contributor Author

prateek565 commented Oct 5, 2022

Okay i will get it done by today. Thanks for feedback

@prateek565
Copy link
Contributor Author

Hi @mathiasayivor I have implemented all the mentioned changes except setLikesno(likesno+1); as this is just a counter and a use state hook which help to show likes fastly as soon as like button is clicked .

Using an updating function in this context is important IMO since likes can be updated unpredictably
@mathiasayivor
Copy link
Contributor

LGTM! Great job @prateek565!
One more thing though,
Based on Post.jsx line 64 if (user && likecount !== undefined) {, does it mean older posts cannot be liked?
was thinking we could add the likecount field if it doesn't exist else users would not be able to like older posts

@prateek565
Copy link
Contributor Author

prateek565 commented Oct 5, 2022

Yeah older posts cant be liked as they dont have likecount in their database . as i cant change database i have discussed this with @narayan954 he said he will remove older posts.

This is what he said

I have some cleanup to do i guess. Or you can have a condition for older post where post without likecount field wont be bothered while displaying likes

@mathiasayivor
Copy link
Contributor

Yeah older posts cant be liked as they dont have likecount in their database . as i cant change database i have discussed this with @narayan954 he said he will remove older posts.

Is it not possible to add fields to a firebase doc dynamically?

@prateek565
Copy link
Contributor Author

Yeah older posts cant be liked as they dont have likecount in their database . as i cant change database i have discussed this with @narayan954 he said he will remove older posts.

Is it not possible to add fields to a firebase doc dynamically?

I have no idea about that i know it can be done by admin who have acess to firestore

@mathiasayivor
Copy link
Contributor

Yeah older posts cant be liked as they dont have likecount in their database . as i cant change database i have discussed this with @narayan954 he said he will remove older posts.

Is it not possible to add fields to a firebase doc dynamically?

I have no idea about that i know it can be done by admin who have acess to firestore

Well, I think it's possible with setDoc. But @narayan954 can still delete the old posts as suggested to free up some space

Copy link
Contributor

@mathiasayivor mathiasayivor left a comment

Choose a reason for hiding this comment

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

LGTM

@mathiasayivor mathiasayivor merged commit 0a33e42 into narayan954:master Oct 5, 2022
@narayan954
Copy link
Owner

narayan954 commented Oct 6, 2022

Thanks a lot for your contribution :)
Sorry I was unavailable yesterday

@prateek565
Copy link
Contributor Author

Thanks I was able to learn a lot of new things with this project 😊.

@narayan954
Copy link
Owner

So good to hear that!!

@narayan954
Copy link
Owner

It was all due to @mathiasayivor , I am really thankful to him.

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.

[FEATURE] Post Like Feature
3 participants