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

Logic to score activities #127

Merged
merged 21 commits into from
Dec 7, 2023
Merged

Logic to score activities #127

merged 21 commits into from
Dec 7, 2023

Conversation

RishikeshNK
Copy link
Contributor

@RishikeshNK RishikeshNK commented Nov 19, 2023

Closes #126

Description

Things you especially want reviewed

Screenshots if Applicable

Checklist

  • Ticket number in PR title
  • Add ticket number to ("Closes #")
  • Move status to Code Review in GH Board
  • People added to reviewers
  • Asked for Review in Slack

Copy link

vercel bot commented Nov 19, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
faculty-activity-tracker ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 6, 2023 9:39pm

@RishikeshNK RishikeshNK changed the title [WIP]: Logic to score activities Logic to score activities Dec 5, 2023
Copy link
Collaborator

@maxpinheiro maxpinheiro left a comment

Choose a reason for hiding this comment

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

looking good, just a few small suggestions.

also, I'm not sure if I missed this, but where is the redux store being initialized with the professor's score? will the ProfessorScoreCard render correctly on initially loading before we ever update the score?

src/services/professorScore.ts Outdated Show resolved Hide resolved
src/pages/merit/professors/[professorId].tsx Show resolved Hide resolved
@RishikeshNK
Copy link
Contributor Author

RishikeshNK commented Dec 6, 2023

looking good, just a few small suggestions.

also, I'm not sure if I missed this, but where is the redux store being initialized with the professor's score? will the ProfessorScoreCard render correctly on initially loading before we ever update the score?

@maxpinheiro

My idea was to initialize it as null initially. Then, when a user visits the /merit/professors/[professorId] route, the fetched ProfessorScore is dispatched to the Redux store (See updateComputedProfessorScoreForUser). The ProfessorScoreCard component gets the initial state from the store (using useSelector). If it is null, we don't render the component; otherwise, we use the global reference of the professor score for that user.

I tried it out quite a bit -- the dispatch from the page happens almost instantaneously so the variable in the Redux store is only null for the first render (you won't notice it!). You can also try it out for yourself in the Vercel preview environment. Do you think there is a cleaner way of doing this?

Copy link
Collaborator

@maxpinheiro maxpinheiro left a comment

Choose a reason for hiding this comment

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

👍

@RishikeshNK
Copy link
Contributor Author

Manually tested. All cases work as intended.

Screenshot from 2023-12-06 19-21-31

Copy link
Collaborator

@Suraj-Ram Suraj-Ram left a comment

Choose a reason for hiding this comment

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

Awesome!

@RishikeshNK RishikeshNK merged commit 6f99b5b into main Dec 7, 2023
3 checks passed
@RishikeshNK RishikeshNK deleted the add-scoring-functionality branch December 7, 2023 00:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Figure out logic to score activities
3 participants