-
Notifications
You must be signed in to change notification settings - Fork 0
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
Wrapped Frontend - Add TA slides & Update slide logic #879
Conversation
[diff-counting] Significant lines: 792. |
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.
Great start on this so far Annie! And nice work navigating the existing (long) component file and adding in React/CSS changes to more closely mimic the designs. I added a few code style comments, but a couple other things to note:
- usually for frontend PRs, it's good to attach screenshots of the changes you made (even if the reviewer will run it locally) so it's clearer what changes you made. Attaching the reference Figma designs is also a plus.
- if you reload and go to to the favorite TA slide for the first time, there's some flickering – probably some incorrect logic related to the total number of stages for users who are also TAs, but I can take another look and suggest fixes
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 couple misc. design todos for this PR (or the next):
- banner styling on intro slide, adding banners to total minutes, fav ta, and conclusion slide
- decrease transparency of the people on total visits slide
- mini ppl in places
- numbers
@@ -53,7 +129,24 @@ const Wrapped = (props: Props) => { | |||
personalityType: string; | |||
timeHelpingStudents: number; | |||
totalMinutes: number; | |||
favTaId: string; |
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.
Since this is type checking using the as
keyword on the document data retrieved from firebase, would it be cleaner to just write a typescript interface like
interface WrappedData { officeHourVisit: never[]; personalityType: string; timeHelpingStudents: number; ... }
. Then we can just use that in the state variable wrappedData
above. But this is just a minor optimization imo :)
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.
Good to merge once all comments are addressed!
Summary
Added three total slides to the Wrapped Frontend by building on this PR and Figma dev handoff. I added slides is to display a student's favorite TA, the total time a TA helped students, and a slide for how many students a TA has helped. Also updated slide logic so that if an user is only a student, they do not see TA slides, and if an user is both a student & TA they see all slides, and if an user is only a TA they only see TA specific slides.
Test Plan
To test the slide logic, I edited my test data in QMI Test to test:
Notes
Breaking Changes
None