Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Select by instructor #473

Merged
merged 37 commits into from Apr 6, 2021
Merged

Select by instructor #473

merged 37 commits into from Apr 6, 2021

Conversation

firejake308
Copy link
Collaborator

@firejake308 firejake308 commented Nov 22, 2020

Description

A common request that we have received is the ability to select all sections taught by a particular professor, as this is a common way for students to pick classes. This PR adds such a checkbox next to the instructor name. From a design perspective, this makes the instructor header a little cramped on small screen sizes, but it can't really be helped imo 🤷. Maybe consider showing professor's last name only?

Anyway, in order to provide a sense of hierarchy, the SectionInfos for the individual sections had to be indented to the right of the professor checkbox. Because of this indentation, on smaller screens, the meeting info may be wrapped onto 2 lines. If you don't like the current wrapping, we might need to look into CSS Grid rather than Flexbox (the current approach), which honestly scares me just a little bit.

Also, I elected to use alternating colors (light maroon & white) to distinguish between mulitple meetings in the same section. Because I didn't like how this looked when there was only one meeting, I added a CSS rule to override the alternating colors for single-meeting sections. Someone remind me to add Added a comment to that line, because I'm 90% sure we're going to forget what it does in the future.

We had previously discussed adding icons, but after trying them out, they felt like they cluttered the space to me. Since alternating colors was pretty effective at distinguishing between one meeting and the next, I honestly don't feel that icons are necessary.

Small design bug: If a section has an odd number of meetings (e.g. ENGR 102), the mini-divider will be covered by the background of the last meeting. First fix that I thought of was throwing in a 3px margin between the last section and the divider, and I think I can use CSS selectors to limit this to the cases when the last meeting is colored. Lmk if y'all have better ideas.

How to test

Automated tests added for the selecting functionality. For visuals, check out KINE 199 for 1-meeting sections, BIOL 351 for 2-meeting sections, ENGR 102 for 3-meeting sections, PHYS 202 for 4-meeting sections, and any other classes you like.

Screenshots

1 meeting 2 meetings 3 meetings
image image image

Related tasks

Closes #259

@firejake308 firejake308 self-assigned this Nov 22, 2020
@firejake308 firejake308 added change Changes previously existing functionality frontend Anything related to the frontend - React, etc labels Nov 22, 2020
Copy link
Collaborator

@eelyort eelyort left a 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, its especially nice in that it seems like it selects only one group regardless of sorting order.

For addressing the instructor label being cramped, what do you think about dispensing with the checkbox and instead changing the color of the label or something?

Copy link
Collaborator

@rachelconn rachelconn left a comment

Choose a reason for hiding this comment

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

While we said to make this use grid in a prior meeting, looking at this again I think it looks fine with 2 columns since they're aligned. It looks like this hasn't been changed since then so we might want to discuss what exactly we want to do in the next meeting

While the 2 column layout likes fine now imo, it looks a bit odd with one especially as the width goes up, here's 1440p:
image
The two things I noticed are

  • Widths aren't uniform since they depend on content widths
  • Building and meeting days are really close together which looks odd imo. Off the top of my head I can't think of a simple solution to this since that's the behavior we want when there are two columns, but I still wanted to point it out

@firejake308
Copy link
Collaborator Author

Here are the options for spacing out each of the cells if we stick with flexbox:

Space between (current) Space around Space evenly
image image image

Personally, I think space evenly is best when there is no wrapping, but I want to double-check with y'all since it changes the ones where it wraps too

@firejake308
Copy link
Collaborator Author

Another screenshot that shows a class with a few more meetings (MATH 151):
image

@gannonprudhomme
Copy link
Member

Just btw you need to merge master into this as you have some conflicts

@gannonprudhomme
Copy link
Member

Reminder to update this with master!

@firejake308
Copy link
Collaborator Author

firejake308 commented Mar 1, 2021

Applied changes discussed at 2/28 meeting, namely:

  • Right-align meeting times
  • Move 1fr from meeting days to meeting time
  • Increase cutoff to 1500px 1600px

Here are 2 screenshots of the 2 possible layouts

2 rows 1 row
image image

@gannonprudhomme
Copy link
Member

This is failing frontend tests / build just btw

@firejake308
Copy link
Collaborator Author

firejake308 commented Mar 15, 2021

Merged with master

Copy link
Member

@gannonprudhomme gannonprudhomme left a comment

Choose a reason for hiding this comment

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

Super close to being ready! Just two minor things.

Getting an error in the console upon render: validateDOMNesting(...): <li> cannot appear as a descendant of <li>

Also the meeting days are barely not aligned to how they were previously - I might be fine leaving it like this, but really wouldn't prefer it tbh

Before(1080p) After(1080p)
chrome_h8Bt9jEYAl chrome_X0jGdRP8hy

Even worse on 1440p

chrome_BcONIg2nQE

@firejake308
Copy link
Collaborator Author

Centered text and fixed the DOM nesting by changing the GPA text in GradeDist by telling ListSubheader to render as a span rather than an li. This shouldn't have affected any of the styles from what I can tell.

Left aligned Centered
image image

Copy link
Member

@gannonprudhomme gannonprudhomme left a comment

Choose a reason for hiding this comment

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

From the meeting:

  • Meeting days spacing is off, isn't evenly spaced (see previous screenshots)

We need to revert the meeting days centering, as it not being left-aligned makes the low-width setting hard to parse. We could make it conditional (center aligned when high-width, left-aligned with low-width), but honestly I think left-aligned in both cases is fine personally
chrome_h6vvukKsYf

Also, I think the instructor text should be either center aligned or bottom aligned with the checkbox. It appears like its top-aligned currently.
chrome_3gYt8KQOVe

@firejake308
Copy link
Collaborator Author

To discuss at meeting:
image

@firejake308
Copy link
Collaborator Author

Prof looks centered to me:
image

}
}

.meeting-info-wrapper:nth-child(2n) div {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
.meeting-info-wrapper:nth-child(2n) div {
.meeting-info-wrapper:nth-child(even) div {

I don't actually care if you make this change since this is perfectly readable but just so you know, there are nth-child(even) and nth-child(odd) selectors

Co-authored-by: Gannon Prudhomme <gannonprudhomme@users.noreply.github.com>
@firejake308 firejake308 merged commit a1928d1 into master Apr 6, 2021
@firejake308 firejake308 deleted the frontend/instructor-select branch April 6, 2021 01:47
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
change Changes previously existing functionality frontend Anything related to the frontend - React, etc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make instructors selectable in SectionSelect
4 participants