Connect Leaderboard Routes #25
Conversation
willmullinthomas
requested review from
a team,
796RCP92VZ,
florisdobber,
jackblanc,
justinkonecny and
liammoyn
and removed request for
a team
willmullinthomas
requested review from
a team
and removed request for
justinkonecny
florisdobber
left a comment
florisdobber
left a comment
There was a problem hiding this comment.
Will review this in more detail when the back end is fixed. Should be soon
liammoyn
left a comment
liammoyn
left a comment
There was a problem hiding this comment.
So is our approach that we'll call the leaderboard route multiple times for each timeframe when the user loads the page? It would probably be better to only be making the call once the user actually selects a particular timeframe so that we load faster and have less data overhead
| tabs={[weeklyTab, monthlyTab, yearlyTab, allTimeTab]} | ||
| itemsPerPage={4} | ||
| /> | ||
| )} |
There was a problem hiding this comment.
We should have some way of displaying an error if one of the calls fails right?
src/store.ts
Outdated
| teamLeaderboardWeeklyState: TeamLeaderboardWeeklyReducerState; | ||
| teamLeaderboardMonthlyState: TeamLeaderboardMonthlyReducerState; | ||
| teamLeaderboardYearlyState: TeamLeaderboardYearlyReducerState; | ||
| teamLeaderboardAllTimeState: TeamLeaderboardAllTimeReducerState; |
There was a problem hiding this comment.
Seems like some nesting would be helpful, having a leaderboardState that then contains user and team states
jackblanc
left a comment
jackblanc
left a comment
There was a problem hiding this comment.
Overall this looks incredible, thank you for doing this, and I'm sorry I didn't get you a review any sooner.
This is mostly there, but I think there's a lot of extra code we won't end up needing.
The main idea I have in this review is that we want to simplify the reducers state and remove usages of React state. We don't need a separate reducer for each time frame of each leaderboard. If we want to store all four time frames in state, we can make four pieces of state in each reducer. Personally, I think we're better off simplifying this by simply having one reducer per leaderboard, which can be called to fetch T - X days for any X.
Let me know if any of this is confusing, happy to chat during office hours today.
|
Condensed a lot of the redux stuff so there is now only 1 item in the store for each of the leaderboards. Also moved the logic for converting LeaderboardItems to TabItems in order to avoid having a redundant React state |
| <LinkCard | ||
| text="Team Leaderboard" | ||
| path="/team-leaderboard" | ||
| background="img4" |
There was a problem hiding this comment.
what is img4? Where is it stored?
There was a problem hiding this comment.
In the linkcard, folder. I didn't make this component, I just added the path variable and prettier changed the indenting
| userLeaderboardState: userLeaderboardReducer, | ||
| teamLeaderboardState: teamLeaderboardReducer, |
jackblanc
left a comment
jackblanc
left a comment
There was a problem hiding this comment.
This looks much better! Left a few more questions, please update the API client to hit the API and verify that it can connect properly, then I'll give a final review.
|
|
src/auth/axios.ts
Outdated
| } | ||
| }; | ||
| store.subscribe(listener); |
There was a problem hiding this comment.
Is this used anywhere? I got an error when it wasn't commented out
There was a problem hiding this comment.
Hmm, it is, I'll look into this right now it came up yesterday for me too
jackblanc
left a comment
jackblanc
left a comment
There was a problem hiding this comment.
Looks great! Awesome work iterating on this
…-localstorage Implement Refresh and State Persistence
|
@willmt80 let me know if you want help resolving these merge conflicts |
Why
Monday.com Ticket
What benefit does this bring to the end user? Or, what benefit does this bring to developers working in the codebase?
Connecting the leaderboard routes populates the leaderboard objects on the volunteer and teams pages with our data, allowing the user to see how who has logged the most trees
This PR
Describe the changes required and any implementation choices you made to give context to reviewers.
Some of the features looked like they could be abstracted, such as the actions and a lot of the types that the leaderboards use. These are in the actions.ts file and types.ts file in the components/leaderboard/ducks directory. The other ducks files are in the respective container directories for the volunteer and teams leaderboards.
One of the features of the leaderboard pages is the ability to view the best volunteers/teams of the last week/month/year/all-time. Each of these possible time scales requires a different object in the store and each has its own reducer. I think it's possible that a ton of the code here can be abstracted, however I was having a hard time doing so.
Screenshots
Provide screenshots of any new components, styling changes, or pages.
Verification Steps
What steps did you take to verify your changes work? These should be clear enough for someone to be able to clone the branch and follow the steps themselves.
Unfortunately, it turns out that the axios library we are using cannot accept arguments in request bodies for get requests. This means we are unable to test that the routes pull in data properly.