Bug fix: Subject cards now say (Not offered) if "Show in Session" filter button is used #241

shergillmanav19 · 2021-07-06T00:59:01Z

made subject cards unclickable and added a warning hover for subjects that are not offered for that specific term

Overview

Closes #194

…that are not offered for the term

aomi · 2021-07-06T01:02:47Z

Not sure why the preview link isn't showing but here it is:
https://staging-clockwork--courseup-ba4f8o0z.web.app/scheduler/202105

aomi · 2021-07-06T02:24:06Z

src/common/sidebar/containers/SidebarContainer.tsx

@@ -52,9 +52,28 @@ export function SidebarContainer({ searchQuery, term }: SidebarContainerProps):
  const loading = subjectsLoading || coursesLoading;

  // sorts the list of subjects alphabetically
-  const sortedSubjects = useMemo(() => subjects?.sort((a, b) => (a.subject > b.subject ? 1 : -1)), [subjects]);
+  let sortedSubjects = useMemo(() => subjects?.sort((a, b) => (a.subject > b.subject ? 1 : -1)), [subjects]);


Maybe extend the type of subjects[] with a inSession: boolean instead?

aomi · 2021-07-06T04:00:38Z

src/lib/fetchers.tsx

@@ -112,6 +112,7 @@ export interface Seat {
 export interface KualiSubject {
  subject: string;
  title: string;
+  inSession?: boolean;


This file isn't stable (it's generated) so I suggest making a type that extends this one instead.

keithradford · 2021-07-07T00:37:11Z

How about something like this design wise? It emulates how chakra deals with disabled buttons by basically just butting a grey tint over it. Additionally I think it should have this appearance permanently, not just on hover. Lastly, the pointer on hover should be not-allowed or just the default cursor so it doesn't appear clickable.

aomi · 2021-07-12T05:14:48Z

src/common/sidebar/containers/SidebarContainer.tsx

+  let sortedSubjects = useMemo(() => inSessionSubjects?.sort((a, b) => (a.subject > b.subject ? 1 : -1)), [
+    inSessionSubjects,
+  ]);
  const parsedCourses = useMemo(() => computeParsedCourses(courses), [courses]);
+  //if filter is set then we want to add (Not offered) to subjects
+
+  if (filter) {
+    sortedSubjects = sortedSubjects?.filter((subject) => {
+      if (parsedCourses[subject.subject]) {
+        subject.inSession = true;
+        return subject;
+      } else {
+        subject.inSession = false;
+        return subject;
+      }
+    });
+  } else {
+    sortedSubjects = sortedSubjects?.filter((subject) => {
+      subject.inSession = true;
+      return subject;
+    });
+  }


It would be better to add this filter logic within the sortedSubjects / computeParsedCourses as this avoids sidesteps the optimization of the useMemo.

aomi · 2021-07-20T05:47:40Z

src/lib/types.ts

+export interface KualiSubjectInSession {
+  inSession?: boolean;
+}
+export type InSessionSubject = KualiSubject & KualiSubjectInSession;


This can be combined into one type declaration. Also, I would suggest not calling it InSessionSubject as we might use it for other stuff :)

aomi · 2021-07-20T05:49:18Z

src/common/sidebar/containers/SidebarContainer.tsx

+  // re assign the type of subjects so inSession property is available
+  const inSessionSubjects: InSessionSubject[] | null = subjects;


This isn't required since you're mapping against the original thing. You can just return a InSessionSubject from the map below.

aomi · 2021-07-20T05:50:27Z

src/common/sidebar/components/Card.tsx

+      <Box
+        bgColor={selected ? undefined : 'white'}
+        bgGradient={selected ? 'linear(to-l, #2e95d1, #7cbce2)' : undefined}
+        color={selected ? 'white' : 'black'}
+        boxShadow="md"
+        py={2}
+        px={4}
+        my={1}
+        cursor={!schedule ? 'pointer' : 'auto'}
+        _hover={{
+          bgGradient: schedule ? undefined : selected ? undefined : 'linear(to-l, #39c686, #80dbb1)',
+
+          color: schedule ? undefined : 'white',
+        }}
+      >
+        <Flex direction="row" alignItems="center" justifyContent="space-between">
+          <VStack alignItems="start" spacing="0">
+            <Text fontSize="lg" fontWeight="bold" p={0} m={0}>
+              {subject} {code}
+            </Text>
+            <Text fontSize="sm" fontWeight="normal" p={0} m={0}>
+              {title}
+            </Text>
+          </VStack>
+          {buttons(code, schedule)}
+        </Flex>
+      </Box>
+    );
+  } else {
+    return (
+      <Box
+        bgColor={selected ? (inSessionSubject ? undefined : '#d3d3d3') : inSessionSubject ? 'white' : '#d3d3d3'}
+        bgGradient={selected ? 'linear(to-l, #2e95d1, #7cbce2)' : undefined}
+        color={selected ? 'white' : 'black'}
+        boxShadow="md"
+        py={2}
+        px={4}
+        my={1}
+        cursor={!schedule ? (inSessionSubject ? 'pointer' : 'auto') : 'auto'}
+        _hover={{
+          bgGradient: schedule
+            ? undefined
+            : selected
+            ? undefined
+            : inSessionSubject
+            ? 'linear(to-l, #39c686, #80dbb1)'
+            : 'linear(to-l, #d3d3d3, #d3d3d3)',
+          color: schedule ? undefined : 'white',
+        }}
+      >
+        <Flex direction="row" alignItems="center" justifyContent="space-between">
+          <VStack alignItems="start" spacing="0">
+            <Text fontSize="lg" fontWeight="bold" p={0} m={0}>
+              {subject} {code}
+            </Text>
+            <Text fontSize="sm" fontWeight="normal" p={0} m={0}>
+              {title}
+            </Text>
+          </VStack>
+          {buttons(code, schedule)}
+        </Flex>
+      </Box>


Can this be combined and just conditionally pass in the props?

github-actions · 2021-07-26T01:53:04Z

Visit the preview URL for this PR (updated for commit 98184c0):

https://staging-clockwork--pr241-manav-bug-fix-subjec-cjj4lbdx.web.app

_{(expires Wed, 11 Aug 2021 03:52:56 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

aomi

Some visual issues I'm seeing:

The gray background colouring appears to be spilling over the defined card. This is due to the usage of the same gray so maybe switching it to be lighter/darker will help clear that up?

Also, I don't think the hover against the text needs to be present when a card is disabled.

The warning icon is low resolution. While I really like the tool-tip the icon usage seems a bit out of place. I'd maybe side on removing it for and add something like this once we've refined it a bit more. Interested in hearing your thoughts as well as @keithradford and @FlyteWizard

aomi · 2021-07-27T02:33:28Z

src/common/sidebar/components/SubjectsList.tsx

+        if (subject.inSession) {
+          return (
+            <LinkBox
+              as={Link}
+              to={{
+                pathname: `/${route}/${term}/${subject.subject}`,
+                search: pid ? `?pid=${pid}` : undefined,
+              }}
+              key={index}
+            >
+              <Card subject={subject.subject} inSessionSubject={subject.inSession} title={subject.title} />
+            </LinkBox>
+          );
+        } else {
+          return (
+            <Box key={index}>
+              <Card subject={subject.subject} inSessionSubject={subject.inSession} title={subject.title} />
+            </Box>
+          );
+        }


This can use tenary conditional so you can remove the ifs and returns.

aomi · 2021-07-27T02:35:18Z

src/common/sidebar/components/Card.tsx

+      bgColor={
+        selected
+          ? inSessionSubject === undefined
+            ? undefined
+            : inSessionSubject
+            ? undefined
+            : '#d3d3d3'
+          : inSessionSubject === undefined
+          ? 'white'
+          : inSessionSubject
+          ? 'white'
+          : '#d3d3d3'
+      }


This seems a bit extreme and could likely be simplified somehow. Or even just moving this into separate variables so it's easier to read would be great.

…he color of bg to the SidebarContainer color (#e4e4e4)

… ternary operation to subjectlist

aomi · 2022-01-31T00:07:31Z

Given the outdated nature and lack of user reports about this issue closing.

made course cards unclickable and added a warning hover for subjects …

9e08a4a

…that are not offered for the term

shergillmanav19 requested a review from a team as a code owner July 6, 2021 00:59

aomi reviewed Jul 6, 2021

View reviewed changes

made linear gradient to grey, added inSession property to KualiSubject

bf6765f

aomi reviewed Jul 6, 2021

View reviewed changes

extended InSessionSubject interface for Subject type

9514a29

shergillmanav19 added 2 commits July 11, 2021 17:09

addressed PR comments

b352f09

created type file which stores inSessionSubject currently

4995f8e

aomi reviewed Jul 12, 2021

View reviewed changes

moved the filter logic inside useMemo hook

889a272

aomi reviewed Jul 20, 2021

View reviewed changes

addressed comments

46d7bcd

addressed PR comments

b61bac6

shergillmanav19 requested a review from aomi July 26, 2021 05:13

aomi reviewed Jul 27, 2021

View reviewed changes

shergillmanav19 added 2 commits August 3, 2021 20:43

added a variable for when inSessionSubject is undefined and changed t…

8a88a01

…he color of bg to the SidebarContainer color (#e4e4e4)

added notallowedicon instead of low resolution warning sign and added…

98184c0

… ternary operation to subjectlist

shergillmanav19 requested a review from aomi August 4, 2021 04:22

aomi closed this Jan 31, 2022

aomi deleted the manav/bug-fix/subject-with-no-courses branch February 8, 2022 18:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug fix: Subject cards now say (Not offered) if "Show in Session" filter button is used #241

Bug fix: Subject cards now say (Not offered) if "Show in Session" filter button is used #241

shergillmanav19 commented Jul 6, 2021

aomi commented Jul 6, 2021

aomi Jul 6, 2021

aomi Jul 6, 2021

keithradford commented Jul 7, 2021

aomi Jul 12, 2021

aomi Jul 20, 2021

aomi Jul 20, 2021

aomi Jul 20, 2021

github-actions bot commented Jul 26, 2021 •

edited

Loading

aomi left a comment •

edited

Loading

aomi Jul 27, 2021

aomi Jul 27, 2021

aomi commented Jan 31, 2022

		// re assign the type of subjects so inSession property is available
		const inSessionSubjects: InSessionSubject[] \| null = subjects;

Bug fix: Subject cards now say (Not offered) if "Show in Session" filter button is used #241

Bug fix: Subject cards now say (Not offered) if "Show in Session" filter button is used #241

Conversation

shergillmanav19 commented Jul 6, 2021

Overview

aomi commented Jul 6, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keithradford commented Jul 7, 2021

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

github-actions bot commented Jul 26, 2021 • edited Loading

aomi left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aomi commented Jan 31, 2022

github-actions bot commented Jul 26, 2021 •

edited

Loading

aomi left a comment •

edited

Loading