feat(ui): course color picker #382

EthanL06 · 2024-10-23T00:39:32Z

Resolves #352

This change is

Update

…stration-Plus into feature/color-picker

… utilities

…property

IsaDavRod

Here are some things I've noticed:

Async/Other courses should also have the color picker button
The color picker button should be closer to the top right corner of the course block and, along with the color palette, should be slightly bigger. See the video below showing this PR vs the Figma component (Figma shows palette and color picker button as bigger):
https://github.com/user-attachments/assets/b2f13887-afdf-4c40-ae67-57c0542df7cb
Color picker button should have a shadow like other buttons.
Ignore the Invert color button. That can be hidden for now. (I kind of want to make it a gear button that takes folks to the settings page to customize how colors work)
There is a bug where courses glitch out of the grid when opening the color palette, and is fixed upon refresh. See video:
https://github.com/user-attachments/assets/e7ae7ea2-bc63-4812-9838-2f35869d3c1e

Razboy20

Reviewing on mobile so I'll have to come back and take a look at this later, but overall looking good.

I think it would be best if we used headless ui paradigms for rendering the floating elements rather than dealing with them manually, and that also enables easier animation/transitions.

Also, preferably (although I know it already exists in the codebase) remove types from JSDoc annotations, as you're repeating information given elsewhere-that is, through typescript.

I recommend trying to follow https://tsdoc.org/

src/views/hooks/useSchedules.ts

src/shared/util/colors.ts

src/views/components/calendar/CalendarCourseCell.tsx

src/views/components/calendar/CalendarCourseCellColorPicker/HexColorEditor.tsx

…pdate props

…pdate related props

… based on preview color

doprz · 2024-11-17T06:13:28Z

Reviewing on mobile so I'll have to come back and take a look at this later, but overall looking good.

I think it would be best if we used headless ui paradigms for rendering the floating elements rather than dealing with them manually, and that also enables easier animation/transitions.

Also, preferably (although I know it already exists in the codebase) remove types from JSDoc annotations, as you're repeating information given elsewhere-that is, through typescript.

I recommend trying to follow https://tsdoc.org/

This is now fixed with #430

…nce hook

doprz · 2024-11-18T00:37:41Z

src/shared/util/colors.ts

+/**
+ * Returns a darker shade of the given hex color by reducing the RGB values by the specified offset.
+ *
+ * @param color - The hexadecimal color value to darken.
+ * @param offset - The amount to reduce each RGB component by (default is 20).
+ * @returns The darker shade of the given hex color.
+ * @throws If the provided color is not a valid hex color.
+ */
+export function getDarkerShade(color: HexColor, offset: number = 20): HexColor {
+    const rgb = hexToRGB(color);
+    if (!rgb) {
+        throw new Error('color: Invalid hex.');
+    }
+
+    const [r, g, b] = rgb.map(c => Math.max(0, c - offset));
+    if (r === undefined || g === undefined || b === undefined) {
+        throw new Error('RGB values are undefined');
+    }
+
+    return `#${r.toString(16).padStart(2, '0')}${g.toString(16).padStart(2, '0')}${b.toString(16).padStart(2, '0')}`;
+}
+


Generally when you want to get a darker shade of something, HSL values are much nicer to work with. I would use HSL values for this.

doprz · 2024-11-18T00:38:37Z

src/shared/util/colors.ts

@@ -145,15 +192,15 @@ export function getUnusedColor(
        ...c,
        colorway: getColorwayFromColor(c.colors.primaryColor),
    }));
-    const usedColorways = new Set(scheduleCourses.map(c => c.colorway));
+    const usedColorways = new Set(scheduleCourses.map(c => c.colorway.colorway));


c.colorway.colorway could be a bit confusing, let's try renaming it a bit for code clarity.

doprz · 2024-11-18T00:41:30Z

src/views/components/calendar/CalendarCourseCellColorPicker/HexColorEditor.tsx

 /**
 * Utility component to allow the user to enter a valid hex color code
 *
- * @param {HexColorEditorProps} props - the props for the component
- * @param {string} props.hexCode - the current hex color code displayed in this component. Note that this code does not
+ * @param props - the props for the component
+ * @param props.hexCode - the current hex color code displayed in this component. Note that this code does not
 * include the leading '#' character since it is already included in the component. Passed down from the parent component.
- * @param {React.Dispatch<React.SetStateAction<string>>} props.setHexCode - set state fn to control the hex color code from parent
- * @returns {JSX.Element} - the hex color editor component
+ * @param props.setHexCode - set state fn to control the hex color code from parent
+ * @returns the hex color editor component


Please pull changes from main to include changes from #430 and refactor the jsdoc for HexColorEditor using the tsdoc spec.

doprz · 2024-11-18T00:43:34Z

src/views/components/calendar/CalendarGrid.tsx

@@ -69,7 +70,9 @@ export default function CalendarGrid({
                .map(() => (
                    <div className='h-4 flex items-end justify-center border-r border-gray-300' />
                ))}
-            {courseCells ? <AccountForCourseConflicts courseCells={courseCells} setCourse={setCourse} /> : null}
+            <ColorPickerProvider>
+                {courseCells ? <AccountForCourseConflicts courseCells={courseCells} setCourse={setCourse} /> : null}


null isn't needed here. Refactor to conditionally render HexColorEditor if courseCells is true. Utilize the logical and && operator instead of the ternary operator.

https://react.dev/learn/conditional-rendering#logical-and-operator-

src/views/components/calendar/CalendarGrid.tsx

doprz · 2024-11-18T00:44:59Z

src/views/contexts/ColorPickerContext.tsx

+/**
+ * Provides the color picker context to its children.
+ *
+ * @param props - The properties for the ColorPickerProvider component.
+ * @param props.children - The child components that will have access to the color picker context.
+ * @returns The provider component that supplies the color picker context to its children.
+ */
+export const ColorPickerProvider = ({ children }: ColorPickerProviderProps) => {
+    const colorPicker = useColorPicker();
+
+    return <ColorPickerContext.Provider value={colorPicker}>{children}</ColorPickerContext.Provider>;
+};
+
+/**
+ * Custom hook to use the ColorPicker context.
+ * Throws an error if used outside of a ColorPickerProvider.
+ * @returns {ColorPickerInterface} The color picker context value.
+ */


Refactor jsdoc to use tsdoc spec. Refer to previous comment.

doprz · 2024-11-18T00:47:56Z

src/views/hooks/useDebounce.tsx

+import { useCallback, useEffect, useRef } from 'react';
+
+type Timer = ReturnType<typeof setTimeout>;
+type SomeFunction<T extends unknown[]> = (...args: T) => void;


nit. Not the best naming for this.

doprz · 2024-11-18T00:49:37Z

src/views/hooks/useDebounce.tsx

+/**
+ * Custom hook to debounce a function call.
+ *
+ * @param func - The original, non-debounced function.
+ * @param delay - The delay (in ms) for the function to return.
+ * @returns The debounced function, which will run only if the debounced function has not been called in the last (delay) ms.
+ */
+export function useDebounce<T extends unknown[]>(func: SomeFunction<T>, delay: number = 1000): SomeFunction<T> {
+    const timer = useRef<Timer>();
+
+    useEffect(
+        () => () => {
+            if (timer.current) {
+                clearTimeout(timer.current);
+            }
+        },
+        []
+    );
+
+    const debouncedFunction = useCallback(
+        (...args: T) => {
+            if (timer.current) {
+                clearTimeout(timer.current);
+            }
+            timer.current = setTimeout(() => {
+                func(...args);
+            }, delay);
+        },
+        [func, delay]
+    );
+
+    return debouncedFunction;
+}


Good. We can also use lodash for this https://www.npmjs.com/package/lodash

do we really want to bring in lodash? last commit 3 years ago, last publish on NPM is 4 years ago

I'm fine with either. Just worth mentioning.

doprz · 2024-11-18T00:50:00Z

Also please resolve merge conflicts.

Samathingamajig · 2024-11-18T01:25:01Z

src/views/components/calendar/CalendarCourseCell.tsx

+                    'absolute text-black transition-all ease-in-out',
+                    'group-focus-within:pointer-events-auto group-hover:pointer-events-auto group-focus-within:opacity-100 group-hover:opacity-100 gap-y-0.75',


why are these two different strings? just line lengths?

Samathingamajig · 2024-11-18T01:26:59Z

src/views/components/calendar/CalendarCourseCell.tsx

+                onClick={e => {
+                    e.stopPropagation();
+                }}
+                className={clsx(


the color picker (and related) should have screenshot:opacity-0! so that it doesn't show up in screenshot if it's opened

Samathingamajig · 2024-11-18T01:33:42Z

src/views/components/common/Button.tsx

                } satisfies React.CSSProperties
            }
            className={clsx(
                'btn',
                {
-                    'text-white! bg-opacity-100 hover:enabled:shadow-md active:enabled:shadow-sm shadow-black/20':
+                    '!text-white bg-opacity-100 hover:enabled:shadow-md active:enabled:shadow-sm shadow-black/20':


this line doesn't need to be changed. in the majority of the codebase, we use the ! (the !important CSS modifier) after the name of a specific class. officially, tailwind puts it at the front, but UnoCSS (which we use) supports both

Samathingamajig · 2024-11-18T01:38:28Z

src/views/hooks/useDebounce.tsx

+/**
+ * Custom hook to debounce a function call.
+ *
+ * @param func - The original, non-debounced function.
+ * @param delay - The delay (in ms) for the function to return.
+ * @returns The debounced function, which will run only if the debounced function has not been called in the last (delay) ms.
+ */
+export function useDebounce<T extends unknown[]>(func: SomeFunction<T>, delay: number = 1000): SomeFunction<T> {
+    const timer = useRef<Timer>();
+
+    useEffect(
+        () => () => {
+            if (timer.current) {
+                clearTimeout(timer.current);
+            }
+        },
+        []
+    );
+
+    const debouncedFunction = useCallback(
+        (...args: T) => {
+            if (timer.current) {
+                clearTimeout(timer.current);
+            }
+            timer.current = setTimeout(() => {
+                func(...args);
+            }, delay);
+        },
+        [func, delay]
+    );
+
+    return debouncedFunction;
+}


do we really want to bring in lodash? last commit 3 years ago, last publish on NPM is 4 years ago

Samathingamajig · 2024-11-18T01:39:51Z

src/views/hooks/useSchedules.ts

+    const determineSecondaryColor = (color: HexColor): HexColor => {
+        try {
+            const { colorway: primaryColorWay, index: primaryIndex } = getColorwayFromColor(color);
+            const { secondaryColor } = getCourseColors(primaryColorWay, primaryIndex, 400);
+
+            if (!secondaryColor) {
+                throw new Error('Secondary color not found');
+            }
+
+            return secondaryColor;
+        } catch (e) {
+            const secondaryColor = getDarkerShade(color, 80);
+            return secondaryColor;
+        }
+    };


why is this function defined inside of another function? should this function be in its own file, moved into a utils file related to colors, or be part of this file?

IsaDavRod

Just as a note taken from Discord convo: This PR does not let me add a course to schedules where at least one course block is a custom color.

…rator

EthanL06 added 4 commits October 21, 2024 11:59

Merge pull request #1 from Longhorn-Developers/main

705ee07

Update

fix: update CourseCellColorPicker.tsx background to white

ec92787

feat: add color picker to CalendarCourseCell component

883f01c

Merge branch 'main' of https://github.com/Longhorn-Developers/UT-Regi…

5836479

…stration-Plus into feature/color-picker

DereC4 added the feature label Oct 23, 2024

EthanL06 added 2 commits October 25, 2024 02:52

feat: add color picker functionality to update course colors

33c6525

fix: type issues with storybook components

b087676

IsaDavRod self-requested a review October 27, 2024 21:43

EthanL06 added 5 commits October 28, 2024 00:43

feat: add useColorPicker hook, isValidHexColor and updateCourseColors…

7295d94

… utilities

refactor: color picker logic and UI components

06f2bfb

refactor: update useFlattenedCourseSchedule hook to include courseID …

d4d77d7

…property

refactor: update storybook calendar components with updated props

2232c5d

refactor: update color picker ui logic to account for position of cell

21d568e

EthanL06 marked this pull request as ready for review October 28, 2024 06:54

EthanL06 added 2 commits October 28, 2024 02:01

fix: revert back to error handling for invalid rgb

742fc14

refactor: update jsdocs

06661e1

IsaDavRod requested changes Oct 28, 2024

View reviewed changes

Razboy20 requested changes Oct 28, 2024

View reviewed changes

EthanL06 added 4 commits November 1, 2024 19:30

refactor: integrate ColorPickerContext into Calendar components and u…

e715b52

…pdate props

refactor: integrate ColorPickerContext into Calendar components and u…

6de13b1

…pdate related props

refactor: change JSDocs comments and remove unused color inversion state

11d1cc7

refactor: update story components

ff1da91

DereC4 assigned EthanL06 Nov 5, 2024

EthanL06 added 5 commits November 5, 2024 17:58

feat: add functionality for selecting secondary course colors

08e536f

refactor: enhance HexColorEditor to dynamically adjust tag icon color…

80b4cad

… based on preview color

refactor: simplify JSDoc comment in useColorPicker hook

1ffbed6

fix: revert Button component

324f353

refactor: update CalendarCourseCell component positioning and styling

b820091

EthanL06 requested review from IsaDavRod and Razboy20 November 13, 2024 01:57

EthanL06 added 3 commits November 12, 2024 20:14

fix: correct types in color.ts

d2d4c10

feat: add getDarkerShade function to compute darker shades of hex colors

e152c0d

feat: add shadow to color picker button

f4dcacc

EthanL06 added 2 commits November 17, 2024 18:07

fix: update button size in ColorPatch component

8d89145

feat: implement debounced input for hex color editor and add useDebou…

68a4929

…nce hook

doprz requested changes Nov 18, 2024

View reviewed changes

Samathingamajig requested changes Nov 18, 2024

View reviewed changes

IsaDavRod requested changes Nov 18, 2024

View reviewed changes

doprz added 4 commits November 17, 2024 21:50

chore: utilize the logical and && operator instead of the ternary ope…

d33ed4e

…rator

Merge branch 'main' into feature/color-picker

d30c089

Merge branch 'main' into feature/color-picker

720b780

Merge branch 'main' into feature/color-picker

8416b02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ui): course color picker #382

feat(ui): course color picker #382

EthanL06 commented Oct 23, 2024 •

edited by Samathingamajig

Loading

IsaDavRod left a comment •

edited by Samathingamajig

Loading

Razboy20 left a comment

doprz commented Nov 17, 2024

doprz Nov 18, 2024

doprz Nov 18, 2024

doprz Nov 18, 2024

doprz Nov 18, 2024

doprz Nov 18, 2024

doprz Nov 18, 2024

doprz Nov 18, 2024

Samathingamajig Nov 18, 2024

doprz Nov 18, 2024

doprz commented Nov 18, 2024

Samathingamajig Nov 18, 2024

Samathingamajig Nov 18, 2024

Samathingamajig Nov 18, 2024

Samathingamajig Nov 18, 2024

Samathingamajig Nov 18, 2024

IsaDavRod left a comment •

edited

Loading

		'absolute text-black transition-all ease-in-out',
		'group-focus-within:pointer-events-auto group-hover:pointer-events-auto group-focus-within:opacity-100 group-hover:opacity-100 gap-y-0.75',

feat(ui): course color picker #382

Are you sure you want to change the base?

feat(ui): course color picker #382

Conversation

EthanL06 commented Oct 23, 2024 • edited by Samathingamajig Loading

IsaDavRod left a comment • edited by Samathingamajig Loading

Choose a reason for hiding this comment

Razboy20 left a comment

Choose a reason for hiding this comment

doprz commented Nov 17, 2024

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

doprz commented Nov 18, 2024

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

IsaDavRod left a comment • edited Loading

Choose a reason for hiding this comment

EthanL06 commented Oct 23, 2024 •

edited by Samathingamajig

Loading

IsaDavRod left a comment •

edited by Samathingamajig

Loading

IsaDavRod left a comment •

edited

Loading