Added icon to honors sections

[firejake308]

Added an icon for honors sections and appropriate tests. Currently, the icon is gray to match th professor's name, and there isn't a whole lot of space between the text and the icon. Seems fine to me, but if y'all don't like it, it's an easy fix.

Closes #230

[gannonprudhomme]

Looks good, pretty straightforward. Only request would be to add a test where you have an instructor that has some honors sections and some non-honors sections, and test that it shows them in two separate blocks as functionally that's what should be happening.

Also rebase this with master once #253 is merged so we can double check that the course card instructor stuff is working as intended with this

[rachelconn]

I think that hovering over the honors icon should make help text appear that says "Honors" or "Honors Section" or something like that, just to make it a little easier for users. Luckily MaterialUI includes the Tooltip component so this shouldn't be too difficult to implement

[firejake308]

Good idea El El mar, may. 12, 2020 a la(s) 1:12 p. m., Ryan Conn < notifications@github.com> escribió:

…

I think that hovering over the honors icon should make help text appear that says "Honors" or "Honors Section" or something like that, just to make it a little easier for users. Luckily MaterialUI includes the Tooltip component so this shouldn't be too difficult to implement — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#251 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACM5PAKDEOCJQZTAVUOSRXLRRGGRLANCNFSM4M4OETXQ> .

[firejake308]

Got the tooltip and also separated regular vs honors sections of the same professor.

Added desired functionality

[gannonprudhomme]

Just a thought, but instead of grouping them "in order", we might want to globally group all of the sections for a professor together, regardless of whether the sections they have for a given course are in order or not. For instance, for PHYS 201 for Fall 2020, sections 506, 511-514, and 519-524 are all TBA sections. We might want to group all of these together so they're shown in one block, then we can sort all of the instructors group by the first section number they have, so the TBA block would show after section 505 and before section 510 (which isn't a TBA setion). That way no matter what sections a prof has (other than one having honors and non-honors sections) they'll only be one grouping of that professor.

For instance, PHYS 201 - Fall 2020 would look like:

Lewis Ford:
501 - 505

TBA:
506, 
511 - 514, 
519 - 524

Vladimir V. Khmelenko:
507 - 510

Katrin Becker:
515 - 518

Junaid M. Farooqi:
M01

(By default it looks like this)

Lewis Ford:
501 - 505

TBA:
506, 

Vladimir V. Khmelenko:
507 - 510

TBA:
511 - 514,

Katrin Becker:
515 - 518

TBA:
519 - 524

Junaid M. Farooqi:
M01

Thoughts?

[firejake308]

Force push was done after rebasing with master to resolve a conflict

[gannonprudhomme]

So first, the sorting of this isn't quite right. While it does group things together, it sorts by the instructor name, so for CSCE 121 - Fall 2020, the 200 honors section is at the bottom b/c the instructor's name is Thomas (should be at the top). They should be grouped together by the instructor's name, but the groups themselves should be sorted by the smallest section number they have in them. Also another good example for testing this is MATH 151 - Fall 2019.

Second, I'd add tests that test:

• That it correctly groups by instructor (do a setup similar to my previous comment)
• That it correctly sorts by the section number

[firejake308]

Two questions:

1. Where in the list would the TBAs end up? With the lowest TBA section number, or should TBA always be at the bottom?
2. Also, how would you deal with a professor who teaches both honors and non-honors, but the honors is like 200 and non-honors is like 599? Would they be together or separated?

[gannonprudhomme]

1. Where in the list would the TBAs end up? With the lowest TBA section number, or should TBA always be at the bottom?

It'd honestly probably be best to have TBA's be at the bottom, but I think it'd be kind of weird to hard code that in so I think the lowest TBA section number should be fine

2. Also, how would you deal with a professor who teaches both honors and non-honors, but the honors is like 200 and non-honors is like 599? Would they be together or separated?

I'd say make them separate

[firejake308]

Should we separate out the tests for grouping by instructor and sorting by section number? I think an integration test would be better at catching errors, but I'm open to adding more tests

[firejake308]

Also, if a professor has honors sections, their other sections will also be sorted to the top. I think the sorting algorithm is complicated enough as it is, so I didn't bother with fixing this relatively minor issue

[rachelconn]

Tests look good, but I believe we can simplify the logic for sorting sections quite a bit

[rachelconn]

A couple things about this sort:

• I don't think it works exactly as intended, for example when I try to see the results for MATH 151 in 202031, it shows TBA honors, then TBA non-honors, then more TBA honors, then more TBA non-honors sections
• I don't really like how much custom code this uses to sort, there are a lot of points it could potentially make mistakes and it isn't very easy to read

I've tested this code and it seems to work fine (it also makes it so we don't need to separately sort by instructor names like we do on line 155), another advantage is that it's easy to adapt if we want to change the sort order (for example, have sectionToArr check if the prof name is TBA, and if so set the field to ZZZZ or something so it shows up last):

function sortSections(sections: SectionSelected[]): SectionSelected[] {
  // converts a section to an array of relevant fields for sort
  const sectionToArr = (section: SectionSelected, idx: number): any[] => (
    [section.section.instructor.name, section.section.sectionNum, idx]);
  const sortedArrs = sections.map(sectionToArr).sort();
  // sort sections based on their positions in the sorted array of arrays
  const sorted = sortedArrs.map((section) => sections[section[2]]);
  return sorted;
}

EDIT: I realized that the order my implementation gives is different from what we want (it doesn't sort professors by their lowest section number) so I'm trying to think of another way we could do it

[firejake308]

Wow, that is a really clean way to write it! Thanks for the suggestion. I'll add a test for the TBA thing, then rewrite the sort using this version

[rachelconn]

I came up with a way to do it that displays professors ordered by their lowest section number (which I believe is how we wanted to do it) but is a little more complicated:

function sortSections(sections: SectionSelected[]): SectionSelected[] {
  // sort sections by sectionNum
  const sorted = sections.sort((a: SectionSelected, b: SectionSelected) => {
    if (a.section.sectionNum < b.section.sectionNum) return -1;
    return 1;
  });
  const sectionsForProfs: Map<string, SectionSelected[]> = new Map();
  // maps maintain key insertion order, so add all sections to map and remember order of professors
  sorted.forEach((section) => {
    const instructorName = section.section.instructor.name;
    if (sectionsForProfs.has(instructorName)) sectionsForProfs.get(instructorName).push(section);
    else sectionsForProfs.set(instructorName, [section]);
  });
  // sections are now grouped by professor and sorted by section num
  return [].concat(...sectionsForProfs.values());
}

I think the following change correctly puts TBA sections last, but I haven't found any courses that I can test this with/made any test data for it:

function sortSections(sections: SectionSelected[]): SectionSelected[] {
  // sort sections by sectionNum
  const sorted = sections.sort((a: SectionSelected, b: SectionSelected) => {
    if (a.section.sectionNum < b.section.sectionNum) return -1;
    return 1;
  });
  const sectionsForProfs: Map<string, SectionSelected[]> = new Map();
  // maps maintain key insertion order, so add all sections to map and remember order of professors
  const TBASections: SectionSelected[] = [];
  sorted.forEach((section) => {
    const instructorName = section.section.instructor.name;
    if (instructorName === 'TBA') {
      TBASections.push(section);
    } else if (sectionsForProfs.has(instructorName)) {
      sectionsForProfs.get(instructorName).push(section);
    } else sectionsForProfs.set(instructorName, [section]);
  });
  // sections are now grouped by professor and sorted by section num
  return [].concat(...sectionsForProfs.values(), TBASections);
}

[rachelconn]

One last thing I didn't notice until I left my mouse hovered over the honors button for a few seconds, once you address it I'll approve this

[rachelconn]

I don't think we want this titleAccess now that we have an actual tooltip, since now there are two of them:

[firejake308]

Unfortunately, I was using the built-in tooltip during testing, so I had to replace that with a testId. However, Material's tooltip looks better than the built-in, so I think that this is fine

Removed duplicate tooltips

[gannonprudhomme]

We're changing the sorting for this again, correct? Just make sure cause I forgot

Also really just wanna get this merged soon since it's been open for almost a month at this point

[gannonprudhomme]

Yeah, we're changing it to sort all honors sections to the top IIRC

Yeah I uh forgot about the meeting notes lmao

[gannonprudhomme]

ackkkk I did not mean to do that, whatta day

[rachelconn]

Approving since this now sorts the way we wanted to and you added a test for it, I'd like if you could address my comment before merging though

[rachelconn]

Is there a case where checking this instead of equality will make a difference? I don't really see a reason to change it back to how it was seeing as both of these should take pretty much the same amount of time, but curious why you made the change

[firejake308]

The actual name could either be TBAH or TBAR

[rachelconn]

Oh yeah that's right, I don't know how I didn't notice the order it's in (change name then check if it's TBA), I'd like if you could change it so it checks if the section is TBA first since there isn't really a reason to change the string first, whenever you do I can reapprove this real quick

[firejake308]

If I were using the site, I would want to still see honors sections at the top, even if the professor was TBA, so I changed it to behave that way. Lmk if y'all like that

[firejake308]

Rebased with master, should be ready for review

[gannonprudhomme]

Lets merged this mf 🚀