-
Notifications
You must be signed in to change notification settings - Fork 5
feat: add a dashboard page to the front end to display aggregations #18
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 a dashboard page to the front end to display aggregations #18
Conversation
cbullinger
left a comment
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.
overall lgtm. a couple visual nits and we're missing accessibility options (e.g. table captions)
client/app/aggregations/page.tsx
Outdated
| const yearData = yearResult.status === 'fulfilled' ? yearResult.value : { success: false, error: 'Failed to fetch year data' }; | ||
| const directorsData = directorsResult.status === 'fulfilled' ? directorsResult.value : { success: false, error: 'Failed to fetch directors data' }; | ||
|
|
||
| console.log('Aggregations SSR: Data fetch completed', { |
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('Aggregations SSR: Data fetch completed', { | |
| if (process.env.NODE_ENV === 'development') { | |
| console.log('Aggregations SSR: Data fetch completed', { | |
| } |
client/app/aggregations/page.tsx
Outdated
| fetchMoviesWithComments(5), | ||
| fetchMoviesByYear(), | ||
| fetchDirectorStats(15) |
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.
should we move these to constants?
client/app/aggregations/page.tsx
Outdated
| {movie.recentComments?.slice(0, 2).map((comment, index) => ( | ||
| <div key={index} className={styles.comment}> |
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.
| {movie.recentComments?.slice(0, 2).map((comment, index) => ( | |
| <div key={index} className={styles.comment}> | |
| key={`${movie._id}-${comment.date}-${index}`} |
| @@ -0,0 +1,182 @@ | |||
| import React from 'react'; | |||
| import { fetchMoviesWithComments, fetchMoviesByYear, fetchDirectorStats } from '../lib/api'; | |||
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.
can we also import MovieWithComments, YearlyStats, and DirectorStats?
| max-width: 1200px; | ||
| margin: 0 auto; | ||
| padding: 2rem; | ||
| font-family: var(--font-system); |
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'm getting a "Cannot resolve '--font-system' custom property " error in my IDE here
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.
Updated to use actual fonts
|
|
||
| .year { | ||
| font-weight: 600; | ||
| color: #3498db; |
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.
the blue here indicates a clickable link to me. can we make it a header column instead (i.e. the same dark blue/white text that the header row has?)
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.
Took away the color and I think it looks good just bolded a bit so left it at that
|
|
||
| .rank { | ||
| font-weight: 700; | ||
| color: #e74c3c; |
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.
same with the rank -- is there a different styling we can do that doesn't solely rely on text color?
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.
Removed color
Adds an "Aggregations" page to display the information from the agg endpionts. Also updates the python server code to create a new index on the comments db to avoid timeouts.