feat: unique number into button #393

nikshitak · 2024-10-24T16:55:45Z

Added a button that lets users copy the unique course ID. There's a checkmark that pops up indicating that the ID has been successfully copied. Button design follows figma wireframe.

Resolves #359

This change is

github-actions · 2024-10-24T16:56:05Z

Your pull request title did not conform to conventional commits standards. Our upcoming automated release pipeline will automatically determine
the proper release version based on your pull request title.
Cheat Sheet

feat: A new feature
fix: A bug fix
docs: Documentation only changes
style: Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc)
refactor: A code change that neither fixes a bug nor adds a feature
perf: A code change that improves performance
test: Adding missing tests or correcting existing tests
build: Changes that affect the build system or external dependencies (example scopes: gulp, broccoli, npm)
ci: Changes to our CI configuration files and scripts (example scopes: Travis, Circle, BrowserStack, SauceLabs)
chore: Other changes that don't modify src or test files
revert: Reverts a previous commit

github-actions · 2024-10-24T16:56:11Z

Your pull request title did not conform to conventional commits standards. Our upcoming automated release pipeline will automatically determine
the proper release version based on your pull request title.
Cheat Sheet

feat: A new feature
fix: A bug fix
docs: Documentation only changes
style: Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc)
refactor: A code change that neither fixes a bug nor adds a feature
perf: A code change that improves performance
test: Adding missing tests or correcting existing tests
build: Changes that affect the build system or external dependencies (example scopes: gulp, broccoli, npm)
ci: Changes to our CI configuration files and scripts (example scopes: Travis, Circle, BrowserStack, SauceLabs)
chore: Other changes that don't modify src or test files
revert: Reverts a previous commit

github-actions · 2024-10-24T16:56:36Z

Your pull request title did not conform to conventional commits standards. Our upcoming automated release pipeline will automatically determine
the proper release version based on your pull request title.
Cheat Sheet

feat: A new feature
fix: A bug fix
docs: Documentation only changes
style: Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc)
refactor: A code change that neither fixes a bug nor adds a feature
perf: A code change that improves performance
test: Adding missing tests or correcting existing tests
build: Changes that affect the build system or external dependencies (example scopes: gulp, broccoli, npm)
ci: Changes to our CI configuration files and scripts (example scopes: Travis, Circle, BrowserStack, SauceLabs)
chore: Other changes that don't modify src or test files
revert: Reverts a previous commit

Samathingamajig · 2024-10-24T23:06:16Z

Please include screenshots of before/after your changes

Samathingamajig

Several files (in red) should be reverted to their original state, they shouldn't be changed in this pr.

The one orange, pnpm-lock.yaml, and the blue one, ThemeColors.ts, shouldn't change either, and they have their own comments for more details

Samathingamajig · 2024-10-24T23:08:59Z

src/shared/types/ThemeColors.ts

@@ -6,6 +6,7 @@ export const colors = {
    ut: {
        burntorange: '#BF5700',
        black: '#333F48',
+        white: '#FFFFFF',


This is not needed. White is already provided by unocss as a color, e.g. text-white. To use the white you made here, it w would be text-ut-white, but again, this doesn't need to be here since pure white is just a color and not specific to our design system.

Samathingamajig · 2024-10-24T23:15:03Z

src/views/components/common/PopupCourseBlock.tsx

@@ -34,13 +36,13 @@ export default function PopupCourseBlock({
    dragHandleProps,
 }: PopupCourseBlockProps): JSX.Element {
    const [enableCourseStatusChips, setEnableCourseStatusChips] = useState<boolean>(false);
+    const [isCopied, setIsCopied] = useState<boolean>(false); // Add state to track if copied


the comment here is unnecessary, isCopied with useState<boolean> is clear enough 👍

Samathingamajig · 2024-10-24T23:25:48Z

src/views/components/common/PopupCourseBlock.tsx

+                className='bg-transparent px-2 py-0.25 text-white btn'
+                color={colors.secondaryColor}
+                onClick={handleCopy}
+                style={{ display: 'flex', backgroundColor: colors.secondaryColor, color: 'text-white' }}


few things here:

Unless there's something I'm not seeing, display: 'flex' here should just be the flex class, instead of in the style

'text-white' is not a valid CSS color, it should be just 'white' if you want white. There is a class called text-white which would work here, but [see next point]

I think that the text color should be based on the contrast compared to the secondary color, not always white. @IsaDavRod and @Razboy20 might have more info on if that's necessary. It would be something like the line above const fontColor = pickFontColor(colors.primaryColor); but for the secondary color

using the backgroundColor: colors.secondaryColor in style makes sense though, since there aren't classes for those 👍

That's the error I'm getting when I'm trying to set color to fontColor

Samathingamajig · 2024-10-24T23:31:13Z

pnpm-lock.yaml

This file shouldn't change, since you didn't install any new packages for this. You might be using an outdated version of pnpm?

This file shouldn't change.

.vscode/launch.json

.vscode/settings.json

LICENSE.md

chromatic.config.json

doprz

Let's revert back the files that don't require/shouldn't be changed.

doprz · 2024-10-26T18:41:48Z

public/json/departments.json

This file shouldn't change as your PR didn't need changes here. Are you formatting via pnpm run prettier:fix, manually, or another way?

I think it seems to be auto formatting? I checked my settings and turned the option that I thought was causing the issue off, but it still seems to be auto formatting. What I ended up doing is just copy paste the files that changed from the main branch.

nikshitak · 2024-10-26T21:16:04Z

Before	After

Razboy20 · 2024-10-27T20:33:17Z

src/views/components/common/PopupCourseBlock.tsx

+            <div
+                onClick={handleClick}
+                className={clsx(
+                    'h-full w-full inline-flex items-center justify-center gap-1 rounded pr-3 focusable cursor-pointer text-left',
+                    className
+                )}
+            >


Rather than making a new div here, the onClick should stay where it originally was.

Razboy20 · 2024-10-27T20:34:31Z

src/views/components/common/PopupCourseBlock.tsx

+    const handleCopy = () => {
+        navigator.clipboard.writeText(formattedUniqueId);
+        setIsCopied(true);
+        setTimeout(() => setIsCopied(false), 1000);
+    };


an e: MouseEventshould be accepted as the first argument, and e.preventDefault() should be called, as to not also open a background calendar tab when clicking on this button.

(will happen when the other PR comment is resolved)

Razboy20 · 2024-10-27T20:40:51Z

src/views/components/common/PopupCourseBlock.tsx

+    const handleCopy = () => {
+        navigator.clipboard.writeText(formattedUniqueId);
+        setIsCopied(true);
+        setTimeout(() => setIsCopied(false), 1000);


If you copy the text twice, the previous timeout isn't cancelled, thus the check icon can disappear faster than expected. Rather, a better methodology would be to debounce when the copy state is set to false. (Can probably be done by having the timeout id (returned by setTimeout) be stored in isCopied (perhaps renamed to copyTimeoutId)—it should be set to undefined when it is not active, and to an id when it is.

attempt to clearTimeout(timeoutId), then setTimeoutId(setTImeout(...)). or similar.

Feel free to ping on discord for advice :)

…m/nikshitak/UT-Registration-Plus into feature/unique-number-into-button

src/pages/background/background.ts

src/shared/util/updateBadgeText.ts

Samathingamajig · 2024-11-02T04:20:06Z

src/views/components/common/PopupCourseBlock.tsx

    const formattedUniqueId = course.uniqueId.toString().padStart(5, '0');
+    const [copyTimeoutId, setCopyTimeoutId] = useState<NodeJS.Timeout | undefined>(undefined);


copyTimeoutId should be a ref (useRef), not a useState, since setState calls are queued until the next render. not doing so could lead to issues when a user clicks multiple times in the same render (aside: does that even matter 🤔)

https://www.freecodecamp.org/news/react-state-vs-ref-differences-and-use-cases/#heading-asynchronous-updates

edit: I would store whether we're currently waiting in another state, and the timeout id in a ref

Samathingamajig · 2024-11-02T04:21:47Z

src/views/components/common/PopupCourseBlock.tsx


    const handleClick = async () => {
        await background.switchToCalendarTab({ uniqueId: course.uniqueId });
        window.close();
    };

+    const handleCopy = (event: React.MouseEvent<HTMLElement>) => {
+        if (copyTimeoutId) {


check for copyTimeoutId !== undefined, since

It's technically possible for the timeout id to be 0

You initialize it to undefined

You check for undefined later

Samathingamajig · 2024-11-02T04:23:50Z

src/views/components/common/PopupCourseBlock.tsx

+
+            <button
+                className='flex bg-transparent px-2 py-0.25 text-white btn'
+                color={colors.secondaryColor}


you set backgroundColor and the text color in the style prop, what's the point of this color prop?

Samathingamajig · 2024-11-02T04:25:16Z

src/views/components/common/PopupCourseBlock.tsx

@@ -91,6 +108,20 @@ export default function PopupCourseBlock({
                    <StatusIcon status={course.status} className='h-5 w-5' />
                </div>
            )}
+
+            <button
+                className='flex bg-transparent px-2 py-0.25 text-white btn'


why is text-white (applies color: #FFFFFF via CSS) here, when you set the text color in the style prop? (text-white, being a class, is completely overridden by color: buttonColor, since style has greater precedence than class[Name])

IsaDavRod

Please address Sam's comment about timeout (if you spam click the button, the checkmark will show for a longer time after the last click)
Can the minimum height of the course block's div be 50px?
Can the height of the unique number copy button be 30px?
It looks like this PR disables some functionality too. When the user clicks the course block in the main popup, it should open the Calendar view and the respective injected cc popup. See prod build as an example.

doprz

Please review previous comments and fix the merge conflict.

…ind code, fixed previous bugs

…er-into-button' into feature/unique-number-into-button

nikshitak · 2024-11-17T17:55:05Z

I looked at the Chromatic error and it seemed like the error was stemming from the CourseCatalogInjectedPopup component, but I don't think anything in PopupCourseBlock.tsx has any elements from CourseCatalogInjectedPopup and I haven't modified CourseCatalogInjectedPopup besides pulling from the main branch.

Samathingamajig · 2024-11-26T22:50:01Z

src/views/components/common/PopupCourseBlock.tsx

        >
            <div
                style={{
                    backgroundColor: colors.secondaryColor,
                }}
                className='flex items-center self-stretch rounded rounded-r-0 cursor-move!'
                {...dragHandleProps}
+                onClick={handleClick}


this handleClick was moved to the wrong place, you should be able to click on the PopupCourseBlock and be taken to the calendar to that specific course, with the exception being a click on the unique button

Samathingamajig · 2024-11-26T22:50:58Z

src/views/components/common/PopupCourseBlock.tsx

@@ -96,6 +114,19 @@ export default function PopupCourseBlock({
                    <StatusIcon status={course.status} className='h-5 w-5' />
                </div>
            )}
+
+            <button
+                className='flex bg-transparent px-2 py-0.25 text-white btn'


what is the intention of the bg-transparent here?

Samathingamajig · 2024-11-26T22:53:56Z

src/views/components/common/PopupCourseBlock.tsx

@@ -96,6 +114,19 @@ export default function PopupCourseBlock({
                    <StatusIcon status={course.status} className='h-5 w-5' />
                </div>
            )}
+
+            <button
+                className='flex bg-transparent px-2 py-0.25 text-white btn'


the buttons are too large compared to the height of the PopupCourseBlock itself

Current:

Desired:

Samathingamajig · 2024-11-26T23:10:21Z

src/views/components/common/PopupCourseBlock.tsx

+        if (copyWait !== undefined) {
+            clearTimeout(copyWait);
+        }
+
+        event.preventDefault();
+        navigator.clipboard.writeText(formattedUniqueId);
+        copyTimeoutIdRef.current += 250;
+
+        const newTimeoutId = setTimeout(() => {
+            setCopyWait(undefined);
+        }, copyTimeoutIdRef.current);
+        setCopyWait(newTimeoutId);
+    };


this makes it so that each subsequent click makes the checkmark show for 250 more milliseconds

when I said to use a ref, I meant use a ref to hold the timeout id, I meant something like

const [showCopyIcon, setShowCopyIcon] = useState<boolean>(false); const copyTimeoutRef = useRef<number | undefined>(undefined); // ... const handleCopy = (event: React.MouseEvent<HTMLElement>) { if (copyTimeoutRef.current !== undefined) { clearTimeout(copyTimeoutRef.current); } event.preventDefault(); navigator.clipboard.writeText(formattedUniqueId); showCopyIcon(true); copyTimeoutRef.current = window.setTimeout(() => { showCopyIcon(false); }, 750); };

probably should bring in a react-friendly debouncing function instead of writing this from scratch, though, since there are most likely other places that will need debouncing and it's easy to get wrong

nikshitak added 3 commits October 24, 2024 11:43

feat(ui): added copy course id button

d615262

fix(code): removed comments

193f0c0

fix(ui): added checkmark, removed copied text

ceef14f

nikshitak changed the title ~~Feature/unique number into button~~ feat/unique number into button Oct 24, 2024

nikshitak changed the title ~~feat/unique number into button~~ feat: unique number into button Oct 24, 2024

Samathingamajig requested changes Oct 24, 2024

View reviewed changes

doprz reviewed Oct 26, 2024

View reviewed changes

.vscode/launch.json Outdated Show resolved Hide resolved

doprz reviewed Oct 26, 2024

View reviewed changes

.vscode/settings.json Outdated Show resolved Hide resolved

doprz reviewed Oct 26, 2024

View reviewed changes

LICENSE.md Outdated Show resolved Hide resolved

doprz reviewed Oct 26, 2024

View reviewed changes

chromatic.config.json Outdated Show resolved Hide resolved

doprz requested changes Oct 26, 2024

View reviewed changes

fix(changes): refactored code to follow standards

2d16719

DereC4 assigned nikshitak Oct 26, 2024

fix(code): reverted structure files to original version

8207850

nikshitak requested review from doprz and Samathingamajig October 26, 2024 21:04

Razboy20 requested changes Oct 27, 2024

View reviewed changes

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

nikshitak added 4 commits October 28, 2024 12:01

Merge branch 'feature/unique-number-into-button' of https://github.co…

aeee21b

…m/nikshitak/UT-Registration-Plus into feature/unique-number-into-button

feat(code): refactored to add timeout and stopped default action

65dd362

feat(code): refactored to add timeout and stopped default action

1fdc1c2

fix(code): ran prettier

79e283b

nikshitak requested a review from Razboy20 October 29, 2024 16:31

Samathingamajig requested changes Oct 29, 2024

View reviewed changes

src/pages/background/background.ts Outdated Show resolved Hide resolved

src/shared/util/updateBadgeText.ts Outdated Show resolved Hide resolved

Samathingamajig force-pushed the feature/unique-number-into-button branch from 82c75e6 to 79e283b Compare November 2, 2024 04:13

Samathingamajig requested changes Nov 2, 2024

View reviewed changes

nikshitak added 2 commits November 4, 2024 20:36

fix(code): adjusted css styling for button:

70d8d54

Merge branch 'main' into feature/unique-number-into-button

5eeabc8

nikshitak requested a review from Samathingamajig November 5, 2024 02:37

fix(code): added useref function

0214226

IsaDavRod requested changes Nov 16, 2024

View reviewed changes

doprz requested changes Nov 16, 2024

View reviewed changes

DereC4 added this to the v2.1.0 milestone Nov 17, 2024

nikshitak and others added 6 commits November 17, 2024 11:12

fix(code): revert commit, ensured previous functionality, fixed tailw…

38e6a5c

…ind code, fixed previous bugs

Merge remote-tracking branch 'refs/remotes/origin/feature/unique-numb…

28d38d8

…er-into-button' into feature/unique-number-into-button

Merge branch 'main' into feature/unique-number-into-button

1e1ddae

fix(prettier): ran prettier

89f8b18

fix(Chromatic): attempt to fix Chromatic error

bcbe9e2

fix(Chromatic): revert to previous code

b480957

DereC4 requested review from IsaDavRod and doprz November 19, 2024 21:34

doprz added 2 commits November 19, 2024 20:35

Merge branch 'main' into feature/unique-number-into-button

32f4e7c

Merge branch 'main' into feature/unique-number-into-button

dfd827d

Samathingamajig requested changes Nov 26, 2024

View reviewed changes

Merge branch 'main' into feature/unique-number-into-button

6debab2

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: unique number into button #393

feat: unique number into button #393

nikshitak commented Oct 24, 2024 •

edited by Samathingamajig

Loading

github-actions bot commented Oct 24, 2024

github-actions bot commented Oct 24, 2024

github-actions bot commented Oct 24, 2024

Samathingamajig commented Oct 24, 2024

Samathingamajig left a comment

Samathingamajig Oct 24, 2024

Samathingamajig Oct 24, 2024

Samathingamajig Oct 24, 2024

nikshitak Oct 26, 2024

Samathingamajig Oct 24, 2024

doprz Oct 26, 2024

doprz left a comment

doprz Oct 26, 2024

nikshitak Oct 26, 2024

nikshitak commented Oct 26, 2024

Razboy20 Oct 27, 2024

Razboy20 Oct 27, 2024

Razboy20 Oct 27, 2024

Samathingamajig Nov 2, 2024

Samathingamajig Nov 2, 2024

Samathingamajig Nov 2, 2024

Samathingamajig Nov 2, 2024

IsaDavRod left a comment

doprz left a comment

nikshitak commented Nov 17, 2024

Samathingamajig Nov 26, 2024

Samathingamajig Nov 26, 2024

Samathingamajig Nov 26, 2024

Samathingamajig Nov 26, 2024

		const formattedUniqueId = course.uniqueId.toString().padStart(5, '0');
		const [copyTimeoutId, setCopyTimeoutId] = useState<NodeJS.Timeout \| undefined>(undefined);

feat: unique number into button #393

Are you sure you want to change the base?

feat: unique number into button #393

Conversation

nikshitak commented Oct 24, 2024 • edited by Samathingamajig Loading

github-actions bot commented Oct 24, 2024

github-actions bot commented Oct 24, 2024

github-actions bot commented Oct 24, 2024

Samathingamajig commented Oct 24, 2024

Samathingamajig 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

doprz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikshitak commented Oct 26, 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

IsaDavRod left a comment

Choose a reason for hiding this comment

doprz left a comment

Choose a reason for hiding this comment

nikshitak 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

nikshitak commented Oct 24, 2024 •

edited by Samathingamajig

Loading