-
Notifications
You must be signed in to change notification settings - Fork 14
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
Optimize upvote and downvote colors #364
Conversation
Deployed staging instance to https://staging-364.peterportal.org |
if (!cookies.hasOwnProperty('user')) { | ||
alert('You must be logged in to vote.'); | ||
return; | ||
} | ||
const votes = { | ||
id: ((e.target as HTMLElement).parentNode! as Element).getAttribute('id')!, | ||
// id: ((e.target as HTMLElement).parentNode! as Element).getAttribute('id')!, |
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.
// id: ((e.target as HTMLElement).parentNode! as Element).getAttribute('id')!, |
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.
Looks good. See comments regarding code. Also, I merged in my review sorting PR so it looks like there are merge conflicts now. Lmk if you have any questions about them or need help with them.
} | ||
let colors = await getColors(req); | ||
console.log(colors); |
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.
console.log(colors); |
@@ -61,14 +66,30 @@ const SubReview: FC<SubReviewProps> = ({ review, course, professor, colors, colo | |||
return; | |||
} | |||
const votes = { | |||
id: ((e.target as HTMLElement).parentNode! as Element).getAttribute('id')!, | |||
// id: ((e.target as HTMLElement).parentNode! as Element).getAttribute('id')!, |
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.
// id: ((e.target as HTMLElement).parentNode! as Element).getAttribute('id')!, |
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.
Tested on the instance, it runs better now. Besides Jacob's comments, I think the code looks good. Good job.
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.
Address the previous comments and you should be good to go!
Description
Screenshots
Screen.Recording.2023-10-13.at.5.17.28.PM.mov
Steps to verify/test this change:
Final Checks:
(optional)
Issues
Closes #