diff --git a/site/src/components/Spinner/Spinner.tsx b/site/src/components/Spinner/Spinner.tsx index 8e331a1a6dd20..c985a762ea5eb 100644 --- a/site/src/components/Spinner/Spinner.tsx +++ b/site/src/components/Spinner/Spinner.tsx @@ -1,15 +1,18 @@ /** - * This component was inspired by - * https://www.radix-ui.com/themes/docs/components/spinner and developed using - * https://v0.dev/ help. + * @file This component was inspired by Radix's Spinner component. The animation + * settings were developed using Vercel's v0. + * + * {@link https://www.radix-ui.com/themes/docs/components/spinner} + * {@link https://v0.dev/} */ import isChromatic from "chromatic/isChromatic"; import { type VariantProps, cva } from "class-variance-authority"; -import type { ReactNode } from "react"; +import { type CSSProperties, type FC, useEffect, useState } from "react"; import { cn } from "utils/cn"; -const leaves = 8; +const SPINNER_LEAF_COUNT = 8; +const MAX_SPINNER_DELAY_MS = 2_000; const spinnerVariants = cva("", { variants: { @@ -23,55 +26,167 @@ const spinnerVariants = cva("", { }, }); -type SpinnerProps = React.SVGProps & - VariantProps & { - children?: ReactNode; - loading?: boolean; - }; +type SpinnerProps = Readonly< + React.SVGProps & + VariantProps & { + loading: boolean; -export function Spinner({ + /** + * Indicates whether the `children` prop should be unmounted during + * a loading state. Defaults to `false` - fully unmounting a child + * component has two main risks: + * 1. Hiding children can create invalid HTML. (For example, if you + * have an HTML input and a label, but only hide the input behind + * the spinner during loading state, the label becomes + * "detached"). This not only breaks behavior for screen readers + * but can also create nasty undefined behavior for some built-in + * HTML elements. + * 2. Unmounting a component will cause any of its internal state to + * be completely wiped. Unless the component has all of its state + * controlled by a parent or external state management tool, the + * component will have all its initial state once the loading + * transition ends. + * + * If you do need reset all the state after a loading transition + * and you can't unmount the component without creating invalid + * HTML, use a render key to reset the state. + * + * --- + * {@link https://react.dev/learn/you-might-not-need-an-effect#resetting-all-state-when-a-prop-changes} + */ + unmountChildrenWhileLoading?: boolean; + + /** + * Specifies whether there should be a delay before the spinner + * appears on screen. If not specified, the spinner always appears + * immediately when `loading` is true. + * + * Can help avoid page flickering issues. Example: + * 1. You have a modal that takes a moment to close, and it has + * Spinner content inside it. + * 2. The user triggers a loading transition, and you want to show + * the spinner at some point, especially if the transition takes + * long enough. + * 3. The problem is, if the spinner mounting and modal closing + * happen in quick enough succession, the user will see a brief + * spinner flicker before the modal closes, making the UI appear + * janky, even though it is accurately modeling the state of the + * application. + * 4. The solution, then, to keep the UI feeling polished, is to lie + * to the user and only show the loading state if some + * pre-determined period has lapsed. + */ + spinnerDelayMs?: number; + } +>; + +const leafIndices = 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 = ({ className, - size, loading, children, - ...props -}: SpinnerProps) { - if (!loading) { - return children; + size = "lg", + spinnerDelayMs = 0, + unmountChildrenWhileLoading = false, + ...delegatedProps +}) => { + // Disallow negative timeout values and fractional values, but also round + // the delay down if it's small enough that it might as well be immediate + // from a user perspective + let safeDelay = Number.isNaN(spinnerDelayMs) + ? 0 + : Math.min(MAX_SPINNER_DELAY_MS, Math.trunc(spinnerDelayMs)); + if (safeDelay < 100) { + safeDelay = 0; + } + /** + * Doing a bunch of mid-render state syncs to minimize risks of UI tearing + * during re-renders. It's ugly, but it's what the React team officially + * recommends. + * {@link https://react.dev/learn/you-might-not-need-an-effect#adjusting-some-state-when-a-prop-changes} + */ + const [delayLapsed, setDelayLapsed] = useState(safeDelay === 0); + const canResetLapseOnRerender = delayLapsed && !loading; + if (canResetLapseOnRerender) { + setDelayLapsed(false); + } + // Have to cache delay so that we don't "ping-pong" between state syncs for + // the delayLapsed state and accidentally create an infinite render loop + const [cachedDelay, setCachedDelay] = useState(safeDelay); + const delayWasRemovedOnRerender = + !delayLapsed && safeDelay === 0 && safeDelay !== cachedDelay; + if (delayWasRemovedOnRerender) { + setDelayLapsed(true); + setCachedDelay(safeDelay); } + useEffect(() => { + if (safeDelay === 0) { + return; + } + const id = window.setTimeout(() => setDelayLapsed(true), safeDelay); + return () => window.clearTimeout(id); + }, [safeDelay]); + const showSpinner = delayLapsed && loading; + // Conditional rendering logic is more convoluted than normal because we + // need to make sure that the children prop is always placed in the same JSX + // "slot" by default, no matter the value of `loading`. Even if the children + // prop value is exactly the same each time, the state for the associated + // component will get wiped if the parent changes return ( - - Loading spinner - {[...Array(leaves)].map((_, i) => { - const rotation = i * (360 / leaves); + <> + {showSpinner && ( + + Loading… + {leafIndices.map((index) => ( + + ))} + + )} - return ( - - ); - })} - + {/* + * Invert the condition (showSpinner && unmountChildrenWhileLoading) + * (which is the only one that should result in fully-unmounted + * content), and then if we still get content, handle the other + * three cases of the boolean truth table more granularly + */} + {(!showSpinner || !unmountChildrenWhileLoading) && ( + <> + {showSpinner && ( + This content is loading: + )} + {children} + + )} + ); -} +}; diff --git a/site/src/pages/OrganizationSettingsPage/CreateOrganizationPageView.tsx b/site/src/pages/OrganizationSettingsPage/CreateOrganizationPageView.tsx index cdb70c3158c06..5eb44d3ef1b3a 100644 --- a/site/src/pages/OrganizationSettingsPage/CreateOrganizationPageView.tsx +++ b/site/src/pages/OrganizationSettingsPage/CreateOrganizationPageView.tsx @@ -155,7 +155,7 @@ export const CreateOrganizationPageView: FC<