This repository has been archived by the owner on Aug 14, 2020. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pramith/backend routes #61
Pramith/backend routes #61
Changes from 5 commits
01b5517
a4f3150
052f225
9ecb40c
35e51c4
f157c8f
5a607e6
a424b2f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
(nitpick) Delete the comments
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.
What was the purpose of this change?
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.
Rohith said that he and Pawan were going to initialize each list with a dummy string element to make sure Javascript knew that the list was a list of Strings. They wanted the list getters implemented this way to ignore that first dummy string
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.
Oh interesting okay I must not have caught that in a previous PR. Honestly that shouldn't be necessary but that could also be a refactor for later. Let's make a comment or something about that in the code to prevent future confusion.
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.
In a future PR let's write tests for these to make sure that they work
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.
await shouldn't be necessary UNLESS you want to wait for the "this.push()" to finish being executed in the incrementScore method. I don't think it'll be necessary though since it'll be faster if we let the two updates to Firebase happen at the same time (feel free to delete the comment as well)
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.
Unrelated to implementation but what do you think about having each post collect the users that like a post (I believe we could do this implementation wise by adding a user_list to post) but do you think we should do that or is it better to make likes anonymous (maybe we can ask the team about this too)
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 think collecting the users that like a post would be a good idea. It's easy to implement and offers us more options in the future so I think it's a good idea