-
Notifications
You must be signed in to change notification settings - Fork 5
Added grade distribution displaying to SectionSelect #275
Conversation
97a078b
to
350ec99
Compare
How are you deciding the width? I think it would look better if it ended at the same place as the meeting times |
Pretty weird way of doing it honestly, the parent div has
Honestly how we have it set up currently I'm not sure we can do that right now, although lmk if you have any suggestions. We might have to wait until #238 |
Also just pushed a temp commit that fixes the grade dists appearing on the next line. It's added by #251 I think, so I'll drop it once that gets merged & I rebase this |
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 to me. We can work on the alignment, but I think we'll probably have a big PR for all the course card alignment issues later
} | ||
|
||
.gpa-underline { | ||
border-bottom: 1px solid rgb(221, 221, 221); |
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.
Nice! I wouldn't have known to hover over the GPA unless it had been underlined
c4489d1
to
2449104
Compare
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 except for a tiny nitpick that bothers me way more than it should
{makeGradesRect(grades.I + grades.S + grades.U + grades.X, colors.Other, 'Other')} | ||
</div> | ||
<Tooltip | ||
title={`From ${grades.count} total sections`} |
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.
This should check if there's only one section so that it can say "from 1 total section" instead
As a reminder for me, here's what we decided from the meeting:
|
Rebase this with master whenever you have time now that show remaining seats has been merged, then I'll review it |
- Made prof name & honors icon its own div so the honors icon wouldn't split the name & grade dist space - Flipped the GPA & grade dist rect so GPA is on the left
d7ff254
to
afb0a21
Compare
afb0a21
to
0123ba3
Compare
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
Since functionally I don't have any issues with this I'm going to approve it, like you said we can take care of any small css/layout tweaks in a separate PR
Description
This adds grade distribution displaying to the
SectionSelect
for each instructor group. It also adds the counting of the total amount of sections toGradesManager.instructor_performance
, so we can display theFrom X total sections
text when the GPA is hovered over (thanks for the idea www.aggiegrades.com)How to test
Not sure if there are any edge cases that I can think of. I've just been testing it with
CSCE 121- 202031
, which is my go to typically.Screenshots
Related tasks
Closes #163