-
Notifications
You must be signed in to change notification settings - Fork 155
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: add upvote function #154
Conversation
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.
Wow, I like this. I've been wondering how we'd do this, but this provides a clearer picture and is a great foundation to build upon. I feel like this PR might take a bit of back-and-forth before it's ready, so we'll have to exercise some patience 😅
Either way, the common theme in my suggestions is that we can break up some of the logic of this functionality into smaller, simpler pieces.
This comment was marked as off-topic.
This comment was marked as off-topic.
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.
A step in the right direction! Please don't forget to resolve any comments that you've addressed.
The CI error appears to be unrelated to this change. Also, can you please change the name of |
Co-authored-by: Asher Gomez <ashersaupingomez@gmail.com>
Co-authored-by: Asher Gomez <ashersaupingomez@gmail.com>
Co-authored-by: Asher Gomez <ashersaupingomez@gmail.com>
Co-authored-by: Asher Gomez <ashersaupingomez@gmail.com>
Co-authored-by: Asher Gomez <ashersaupingomez@gmail.com>
@huai-jie, I've made a few improvements. PTAL and test. WDYT? |
LGTM! |
Thank you for all your work, @huai-jie! |
The code is really messy ATM
&
I encountered some issues while attempting to do this
Issues:
Any suggestions on how to improve this are really welcome and appreciated.