feat: Theming - Moved ThemeProvider updates into effect (#1682)

Moved ThemeProvider updates into effect resolves #1669
deephaven · Dec 13, 2023 · a09bdca · a09bdca
1 parent b2972f0
commit a09bdca
Show file tree

Hide file tree

Showing 9 changed files with 30 additions and 62 deletions.
diff --git a/packages/auth-plugins/src/LoginForm.tsx b/packages/auth-plugins/src/LoginForm.tsx
@@ -49,7 +49,7 @@ export function LoginForm({
           >
             {isLoggingIn && (
               <span>
-                <LoadingSpinner className="loading-spinner-vertical-align" />
+                <LoadingSpinner className="mr-2 loading-spinner-vertical-align" />
                 <span className="btn-normal-content">Logging in</span>
                 <span className="btn-hover-content">Cancel</span>
               </span>

diff --git a/packages/chart/src/ChartThemeProvider.tsx b/packages/chart/src/ChartThemeProvider.tsx
@@ -1,4 +1,4 @@
-import { createContext, ReactNode, useEffect, useState } from 'react';
+import { createContext, ReactNode, useMemo } from 'react';
 import { useTheme } from '@deephaven/components';
 import defaultChartTheme, { ChartTheme } from './ChartTheme';
 
@@ -20,20 +20,7 @@ export function ChartThemeProvider({
 }: ChartThemeProviderProps): JSX.Element {
   const { activeThemes } = useTheme();
 
-  const [chartTheme, setChartTheme] = useState<ChartTheme | null>(null);
-
-  // The `ThemeProvider` that supplies `activeThemes` also provides the corresponding
-  // CSS theme variables to the DOM by dynamically rendering <style> tags whenever
-  // the `activeThemes` change. Painting the latest CSS variables to the DOM may
-  // not happen until after `ChartThemeProvider` is rendered, but they should be
-  // available by the time the effect runs. Therefore, it is important to derive
-  // the chart theme in an effect instead of deriving in a `useMemo` to ensure
-  // we have the latest CSS variables.
-  useEffect(() => {
-    if (activeThemes != null) {
-      setChartTheme(defaultChartTheme());
-    }
-  }, [activeThemes]);
+  const chartTheme = useMemo(defaultChartTheme, [activeThemes]);
 
   return (
     <ChartThemeContext.Provider value={chartTheme}>

diff --git a/packages/chart/src/MockChartModel.ts b/packages/chart/src/MockChartModel.ts
@@ -20,7 +20,7 @@ class MockChartModel extends ChartModel {
 
   static smoothing = 1.5;
 
-  static _theme: ChartTheme;
+  static _theme: ChartTheme | null;
 
   static get theme(): ChartTheme {
     /* eslint-disable no-underscore-dangle */

diff --git a/packages/components/src/RandomAreaPlotAnimation.tsx b/packages/components/src/RandomAreaPlotAnimation.tsx
@@ -1,7 +1,7 @@
 /* eslint-disable react-hooks/exhaustive-deps */
 /* eslint-disable react/display-name */
 
-import React, { useEffect, useState, useRef } from 'react';
+import React, { useEffect, useState, useRef, useMemo } from 'react';
 import debounce from 'lodash.debounce';
 import { assertNotNull } from '@deephaven/utils';
 import './RandomAreaPlotAnimation.scss';
@@ -42,14 +42,9 @@ function getRandomAreaPlotAnimationThemeColors(): ThemeColors {
 const RandomAreaPlotAnimation = React.memo(() => {
   const { activeThemes } = useTheme();
 
-  const [themeColors, setThemeColors] = useState<ThemeColors | null>(null);
-
-  // Resolving css variables has to run in `useEffect` or `useLayoutEffect` so
-  // that we know React has updated the DOM with any styles set by the
-  // ThemeProvider.
-  useEffect(() => {
-    setThemeColors(getRandomAreaPlotAnimationThemeColors());
-  }, [activeThemes]);
+  const themeColors = useMemo(getRandomAreaPlotAnimationThemeColors, [
+    activeThemes,
+  ]);
 
   const canvas = useRef<HTMLCanvasElement>(null);
   const container = useRef<HTMLDivElement>(null);
@@ -85,10 +80,6 @@ const RandomAreaPlotAnimation = React.memo(() => {
 
   // Returns the background fill create offscreen as pattern
   function createPatternFill(): CanvasPattern | null | undefined {
-    if (themeColors == null) {
-      return null;
-    }
-
     const { foregroundFill, foregroundStroke } = themeColors;
 
     // create the off-screen canvas
@@ -189,10 +180,6 @@ const RandomAreaPlotAnimation = React.memo(() => {
    * @param timestamp passed in callback from requestAnimationFrame
    */
   function drawCanvas(timestamp?: DOMHighResTimeStamp): void {
-    if (themeColors == null) {
-      return;
-    }
-
     lastTimestamp = lastTimestamp ?? timestamp;
 
     const { background, foregroundStroke, gridColor } = themeColors;
@@ -343,11 +330,9 @@ const RandomAreaPlotAnimation = React.memo(() => {
   }, [themeColors]);
 
   return (
-    themeColors && (
-      <div className="random-area-plot-animation-container" ref={container}>
-        <canvas ref={canvas} className={shade ? 'shade' : ''} />
-      </div>
-    )
+    <div className="random-area-plot-animation-container" ref={container}>
+      <canvas ref={canvas} className={shade ? 'shade' : ''} />
+    </div>
   );
 });
 

diff --git a/packages/components/src/theme/ThemeProvider.tsx b/packages/components/src/theme/ThemeProvider.tsx
@@ -36,9 +36,11 @@ export interface ThemeProviderProps {
 export function ThemeProvider({
   themes: customThemes,
   children,
-}: ThemeProviderProps): JSX.Element {
+}: ThemeProviderProps): JSX.Element | null {
   const baseThemes = useMemo(() => getDefaultBaseThemes(), []);
 
+  const [value, setValue] = useState<ThemeContextValue | null>(null);
+
   const [selectedThemeKey, setSelectedThemeKey] = useState<string>(
     () => getThemePreloadData()?.themeKey ?? DEFAULT_DARK_THEME_KEY
   );
@@ -88,18 +90,17 @@ export function ThemeProvider({
     [activeThemes, selectedThemeKey, customThemes]
   );
 
-  const value = useMemo(
-    () => ({
+  useEffect(() => {
+    setValue({
       activeThemes,
       selectedThemeKey,
       themes,
       setSelectedThemeKey,
-    }),
-    [activeThemes, selectedThemeKey, themes]
-  );
+    });
+  }, [activeThemes, selectedThemeKey, themes]);
 
   return (
-    <ThemeContext.Provider value={value}>
+    <>
       {activeThemes == null ? null : (
         <>
           {activeThemes.map(theme => (
@@ -109,8 +110,12 @@ export function ThemeProvider({
           ))}
         </>
       )}
-      <SpectrumThemeProvider>{children}</SpectrumThemeProvider>
-    </ThemeContext.Provider>
+      {value == null ? null : (
+        <ThemeContext.Provider value={value}>
+          <SpectrumThemeProvider>{children}</SpectrumThemeProvider>
+        </ThemeContext.Provider>
+      )}
+    </>
   );
 }
 

diff --git a/packages/console/src/console-history/ConsoleHistoryResultInProgress.tsx b/packages/console/src/console-history/ConsoleHistoryResultInProgress.tsx
@@ -68,7 +68,7 @@ class ConsoleHistoryResultInProgress extends Component<
         })}
       >
         <span className="badge">
-          <LoadingSpinner />
+          <LoadingSpinner className="ml-2" />
           Running... {TimeUtils.formatElapsedTime(elapsed)}&nbsp;
           <Button
             className="console-history-result-in-progress-cancel"

diff --git a/packages/iris-grid/src/IrisGridThemeProvider.tsx b/packages/iris-grid/src/IrisGridThemeProvider.tsx
@@ -1,5 +1,5 @@
 import { useTheme } from '@deephaven/components';
-import { createContext, ReactNode, useEffect, useState } from 'react';
+import { createContext, ReactNode, useMemo } from 'react';
 import { createDefaultIrisGridTheme, IrisGridThemeType } from './IrisGridTheme';
 
 export type IrisGridThemeContextValue = Partial<IrisGridThemeType>;
@@ -16,16 +16,7 @@ export function IrisGridThemeProvider({
 }: IrisGridThemeProviderProps): JSX.Element {
   const { activeThemes } = useTheme();
 
-  const [gridTheme, setGridTheme] = useState<IrisGridThemeContextValue>({});
-
-  useEffect(
-    function refreshIrisGridTheme() {
-      if (activeThemes != null) {
-        setGridTheme(createDefaultIrisGridTheme());
-      }
-    },
-    [activeThemes]
-  );
+  const gridTheme = useMemo(createDefaultIrisGridTheme, [activeThemes]);
 
   return (
     <IrisGridThemeContext.Provider value={gridTheme}>

diff --git a/packages/iris-grid/src/sidebar/CustomColumnBuilder.tsx b/packages/iris-grid/src/sidebar/CustomColumnBuilder.tsx
@@ -368,7 +368,7 @@ class CustomColumnBuilder extends Component<
       >
         {isCustomColumnApplying && (
           <span>
-            <LoadingSpinner className="loading-spinner-vertical-align" />
+            <LoadingSpinner className="mr-2 loading-spinner-vertical-align" />
             <span className="btn-normal-content">Applying</span>
           </span>
         )}

diff --git a/packages/iris-grid/src/sidebar/TableCsvExporter.tsx b/packages/iris-grid/src/sidebar/TableCsvExporter.tsx
@@ -538,7 +538,7 @@ class TableCsvExporter extends Component<
           >
             {isDownloading && (
               <span>
-                <LoadingSpinner className="loading-spinner-vertical-align" />
+                <LoadingSpinner className="mr-2 loading-spinner-vertical-align" />
                 <span className="btn-normal-content">Downloading</span>
                 <span className="btn-hover-content">Cancel</span>
               </span>