Layla/finish edit classes frontend #928

laylaliuuu · 2025-03-03T01:37:36Z

Summary

Finishes Editing classes frontend, resolved Sophie's PR from last semester.

Fix the inconsistency in hover effects in former classes
Cleaned up the style code in .tsx file to .cscc

Test Plan

Notes

Breaking Changes

None

I have updated the documentation accordingly.
My PR adds a @ts-ignore

dti-github-bot · 2025-03-03T01:37:50Z

[diff-counting] Significant lines: 571.

NIDHI2023 · 2025-03-03T03:13:57Z

src/styles/courses/CourseCard.scss

+    role-color-ta:"#BF7913"; // the color for "TA" 
+    role-color-professor: "green"; // a purple thats closer to our brand colors- #726CFF
+    selectedBackgroundColor-role-student: "rgba(214, 234, 254, 0.4)"; //  "Student" role background color
+    selectedBorderColor-role-student: "#77BBFA"; //  "Student" role border color 


I think the variable names in the css files are a nice addition to make the code more organized

I also like the variable names as well, I think its a great idea! I'm not sure if its an error on my end, but for some reason in the CourseCard.scss file the variables are not recognized as variables with the $ starting character?

Good catch Annie, this is what it looks like on my end with and without the $:

NIDHI2023

Great work, the hover issue seems to be fixed for me! A few more things, I think Richard mentioned a change to the font style specifically from sans-serif to Roboto, I think the font-size Sophie took care of already.

Also, when running I was getting this new warning that should probably be addressed:

You could resolve these by adding 'filterOnActiveAndRole' and 'currentCourses' into the useEffect dependency arrays as suggested. However, since filterOnActiveAndRole is an inline function, React sees it as a "new" function on every render of the CourseSelection component. So even if the logic inside filterOnActiveAndRole does not change, its reference will still be different after each render, which could unnecessarily trigger a lot of useEffect calls.

I think you could look into useCallback to ensure that the function reference remains stable unless one of its dependencies changes - so something like this:
const filterOnActiveAndRole = useCallback(() => { return allCourses .... }); }, [allCourses, CURRENT_SEMESTER, user.userId]);

anchen9

Hi Layla! The hover state and documentation look great, good work on finish up Sophie's PR! Could you attach before & after screenshots to the PR as well?

anchen9 · 2025-03-03T03:53:33Z

src/styles/courses/CourseCard.scss

+    role-color-ta:"#BF7913"; // the color for "TA" 
+    role-color-professor: "green"; // a purple thats closer to our brand colors- #726CFF
+    selectedBackgroundColor-role-student: "rgba(214, 234, 254, 0.4)"; //  "Student" role background color
+    selectedBorderColor-role-student: "#77BBFA"; //  "Student" role border color 


I also like the variable names as well, I think its a great idea! I'm not sure if its an error on my end, but for some reason in the CourseCard.scss file the variables are not recognized as variables with the $ starting character?

NIDHI2023 · 2025-03-03T04:25:59Z

src/styles/courses/CourseCard.scss

+
+            //check coursecard for hex color of prof
+            border: 2px solid #BF7913;
+            color: #BF7913;


I think the Prof color is hardcoded here, maybe use one of the variables you defined?

should i just delete it here? Because I dont think it make sense here.

EricWeng23 · 2025-03-03T04:26:01Z

src/components/includes/CourseSelection.tsx

-        setFormerCourses(allCourses.filter((course) => {
-            return course.semester !== CURRENT_SEMESTER;
-        }));
+    const filterOnActiveAndRole = () => {


I think an optimization that can be made for the filterOnActiveAndRole function is to use the React Hook useMemo to "memoize" the calculation of all the filtering and sorting of all courses. The current implementation would have to run the filtering and sorting every time the page renders, which may result in latency. This is probably not a significant issue right now since QMI only has a limited number of courses, but once the course list expands for allCourses, it would be computation-heavy. An example of useMemo would be:

const filterOnActiveAndRole = useMemo(() => { ... }, [dependency array])

NIDHI2023 · 2025-03-03T04:29:04Z

src/components/includes/CourseCard.tsx

+                            className="courseRole"
+                            style={{
+                                border: "2px solid " + roleColor,
+                                color: roleColor,


So here's where Sophie's trying to use the roleColor variable above - if the css variables don't work out you might just need to hardcode values like "green" for professor and "#BF7913" for TA

laylaliuuu · 2025-03-11T06:22:55Z

src/components/includes/CourseCard.tsx

+            ${roleString === "" ? "editable" : "ineditable"}`}
            onClick={selectCourse}


I don't think I need this, as all of them should be ineditable (clickable?)

laylaliuuu · 2025-03-11T06:26:48Z

src/components/includes/CourseCard.tsx

+    let roleString = "";
+    let roleColor = "";
+    let selectedBackgroundColor = "var(selected-background-color)"; // variables can be changed based on different roles
+    let selectedBorderColor = "var(selected-border-color)";
+    if (role === "ta") {
+        roleString = "TA";
+        roleColor = "#var(role-color-ta)";
+    } else if (role === "professor") {
+        roleString = "PROF";
+        roleColor = "#var(role-color-professor)";
+    } else {
+        selectedBackgroundColor = "selectedBackgroundColor-role-student";
+        selectedBorderColor = "selectedBorderColor-role-student";
    }


I feel like this can be deleted too, but we can talk about it tomorrow, because there are few places that used roleString and i don't know if that will change the behavior of the roles.

laylaliuuu · 2025-03-11T07:07:22Z

src/styles/courses/CourseSelection.scss

+                    flex: 1;
+                    border-radius: 15px;
+                    border: none;
+                    font-size: 18px;


I added the font here, but I think "Roboto" is already coded as the font for the entire page at top, but I just added it here again. Also I changed the font size to a little bit smaller to match the design.

swang235 and others added 13 commits November 6, 2024 20:41

edit classes frontend

28c2670

lint fixes

afbece3

lint fixes 2

b8f555c

pr comments

656e3a2

Merge remote-tracking branch 'origin/master' into sophie-edit-classes

41097ae

fix merge conflict and margin

58a8ded

make all classes hoverable after saved classes

eed7814

change search bar input size

71dcc5a

Merge branch 'master' into layla/finish-edit-classes-frontend

76fa8c3

move styles code from CourseCard.tsx to associated .scss file

97758f8

Change empty components <></> to use && instead

87801e5

fix the hover inconsisitency on former classes (student-role)

4393b11

documentations

39bb8da

laylaliuuu requested a review from a team as a code owner March 3, 2025 01:37

NIDHI2023 reviewed Mar 3, 2025

View reviewed changes

NIDHI2023 requested changes Mar 3, 2025

View reviewed changes

anchen9 self-requested a review March 3, 2025 03:40

anchen9 reviewed Mar 3, 2025

View reviewed changes

NIDHI2023 reviewed Mar 3, 2025

View reviewed changes

EricWeng23 reviewed Mar 3, 2025

View reviewed changes

NIDHI2023 reviewed Mar 3, 2025

View reviewed changes

laylaliuuu commented Mar 11, 2025

View reviewed changes

laylaliuuu added 3 commits March 11, 2025 03:09

address the PR comments

87b7012

fix edited selected border style

6692009

fix code style problem for built

99d1f15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Layla/finish edit classes frontend #928

Layla/finish edit classes frontend #928

laylaliuuu commented Mar 3, 2025

dti-github-bot commented Mar 3, 2025 •

edited

Loading

NIDHI2023 Mar 3, 2025

anchen9 Mar 3, 2025

NIDHI2023 Mar 3, 2025

NIDHI2023 left a comment •

edited

Loading

anchen9 left a comment

anchen9 Mar 3, 2025

NIDHI2023 Mar 3, 2025

laylaliuuu Mar 11, 2025

EricWeng23 Mar 3, 2025

NIDHI2023 Mar 3, 2025

laylaliuuu Mar 11, 2025

laylaliuuu Mar 11, 2025

laylaliuuu Mar 11, 2025

		${roleString === "" ? "editable" : "ineditable"}`}
		onClick={selectCourse}

Layla/finish edit classes frontend #928

Are you sure you want to change the base?

Layla/finish edit classes frontend #928

Conversation

laylaliuuu commented Mar 3, 2025

Summary

Test Plan

Notes

Breaking Changes

dti-github-bot commented Mar 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NIDHI2023 left a comment • edited Loading

Choose a reason for hiding this comment

anchen9 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dti-github-bot commented Mar 3, 2025 •

edited

Loading

NIDHI2023 left a comment •

edited

Loading