chore: get initial version of spinner update in place

Parkreiner · Parkreiner · commit 87fd0c223021 · 2025-02-07T17:00:45.000Z
diff --git a/site/src/components/Spinner/Spinner.tsx b/site/src/components/Spinner/Spinner.tsx
@@ -6,7 +6,7 @@
 
 import isChromatic from "chromatic/isChromatic";
 import { type VariantProps, cva } from "class-variance-authority";
-import { type FC, useEffect, useState } from "react";
+import { type CSSProperties, type FC, useEffect, useState } from "react";
 import { cn } from "utils/cn";
 
 const SPINNER_LEAF_COUNT = 8;
@@ -27,12 +27,44 @@ type SpinnerProps = Readonly<
 	React.SVGProps<SVGSVGElement> &
 		VariantProps<typeof spinnerVariants> & {
 			loading: boolean;
+
+			/**
+			 * Indicates whether the children prop should be unmounted during
+			 * a loading state. Defaults to false - unmounting HTML elements
+			 * like form controls can lead to invalid HTML, so this prop should
+			 * be used with care and only if it prevents render performance
+			 * issues.
+			 */
 			unmountedWhileLoading?: boolean;
+
+			/**
+			 * Specifies whether there should be a delay before the spinner
+			 * appears on screen. If not specified, the spinner always appears
+			 * immediately.
+			 *
+			 * Can help avoid page flickering issues. (e.g., You have a modal
+			 * that takes a moment to close, and it has Spinner content inside
+			 * it. The user triggers a loading transition, and you want to show
+			 * the spinner at some point if a transition takes long enough, but
+			 * if the spinner mounting and modal closing happen in too quick of
+			 * a succession, the UI looks janky. So even though you might flip
+			 * the loading state immediately, you want to wait a second to show
+			 * it in case the modal can close quickly enough. It's lying to the
+			 * user in a way that makes the UI feel more polished.)
+			 */
 			spinnerStartDelayMs?: number;
 		}
 >;
 
 const leavesIterable = Array.from({ length: SPINNER_LEAF_COUNT }, (_, i) => i);
+const animationSettings: CSSProperties = isChromatic()
+	? {}
+	: {
+			transitionDuration: `${0.1 * SPINNER_LEAF_COUNT}s`,
+			transitionTimingFunction: "ease-in-out",
+			animationIterationCount: "infinite",
+		};
+
 export const Spinner: FC<SpinnerProps> = ({
 	className,
 	size,
@@ -42,6 +74,11 @@ export const Spinner: FC<SpinnerProps> = ({
 	unmountedWhileLoading = false,
 	...delegatedProps
 }) => {
+	/**
+	 * @todo Figure out if this conditional logic causes a component to lose
+	 * state. I would hope not, since the children prop is the same in both
+	 * cases, but I need to test this out
+	 */
 	const showSpinner = useShowSpinner(loading, spinnerStartDelayMs);
 	if (!showSpinner) {
 		return children;
@@ -67,26 +104,28 @@ export const Spinner: FC<SpinnerProps> = ({
 						width="2"
 						height="5.5"
 						rx="1"
-						className={
-							// 0.8 is hard-coded because of Tailwind; the value
-							// should always be (0.1 * SPINNER_LEAF_COUNT)
-							isChromatic() ? "" : "animate-[loading_0.8s_ease-in-out_infinite]"
-						}
 						style={{
+							...animationSettings,
 							transform: `rotate(${leafIndex * (360 / SPINNER_LEAF_COUNT)}deg)`,
 							transformOrigin: "center",
 							animationDelay: `${-leafIndex * 0.1}s`,
 						}}
 					/>
 				))}
 			</svg>
-			{!unmountedWhileLoading && <div className="sr-only">{children}</div>}
+
+			{!unmountedWhileLoading && (
+				<div className="sr-only">
+					This content is loading:
+					{children}
+				</div>
+			)}
 		</>
 	);
 };
 
 // Not a big fan of one-time custom hooks, but it helps insulate the main
-// component from the chaos of handling all these states, when the ultimate
+// component from the chaos of handling all these state syncs, when the ultimate
 // result is a simple boolean. V8 will be able to inline the function definition
 // in some cases anyway
 function useShowSpinner(
