From e27d79eaf0896d51e2a4acf2027e99833b76364c Mon Sep 17 00:00:00 2001 From: Brian Ingles Date: Wed, 10 Jan 2024 17:19:58 -0600 Subject: [PATCH 1/4] Improved preload variable handling #1695 --- .../components/src/theme/ThemeProvider.tsx | 15 +++- .../components/src/theme/ThemeUtils.test.ts | 45 ++++++++--- packages/components/src/theme/ThemeUtils.ts | 79 ++++++++++++++----- 3 files changed, 104 insertions(+), 35 deletions(-) diff --git a/packages/components/src/theme/ThemeProvider.tsx b/packages/components/src/theme/ThemeProvider.tsx index c051d4a208..19ad76a7d1 100644 --- a/packages/components/src/theme/ThemeProvider.tsx +++ b/packages/components/src/theme/ThemeProvider.tsx @@ -1,6 +1,10 @@ import { createContext, ReactNode, useEffect, useMemo, useState } from 'react'; import Log from '@deephaven/log'; -import { DEFAULT_DARK_THEME_KEY, ThemeData } from './ThemeModel'; +import { + DEFAULT_DARK_THEME_KEY, + DEFAULT_PRELOAD_DATA_VARIABLES, + ThemeData, +} from './ThemeModel'; import { calculatePreloadStyleContent, getActiveThemes, @@ -30,11 +34,13 @@ export interface ThemeProviderProps { * tell the provider to activate the base themes. */ themes: ThemeData[] | null; + defaultPreloadValues?: Record; children: ReactNode; } export function ThemeProvider({ themes: customThemes, + defaultPreloadValues = DEFAULT_PRELOAD_DATA_VARIABLES, children, }: ThemeProviderProps): JSX.Element | null { const baseThemes = useMemo(() => getDefaultBaseThemes(), []); @@ -71,9 +77,10 @@ export function ThemeProvider({ // Override fill color for certain inline SVGs (the originals are provided // by theme-svg.scss) - overrideSVGFillColors(); + overrideSVGFillColors(defaultPreloadValues); - const preloadStyleContent = calculatePreloadStyleContent(); + const preloadStyleContent = + calculatePreloadStyleContent(defaultPreloadValues); log.debug2('updateThemePreloadData:', { active: activeThemes.map(theme => theme.themeKey), @@ -87,7 +94,7 @@ export function ThemeProvider({ preloadStyleContent, }); }, - [activeThemes, selectedThemeKey, customThemes] + [activeThemes, selectedThemeKey, customThemes, defaultPreloadValues] ); useEffect(() => { diff --git a/packages/components/src/theme/ThemeUtils.test.ts b/packages/components/src/theme/ThemeUtils.test.ts index c91c75e168..931571c1cb 100644 --- a/packages/components/src/theme/ThemeUtils.test.ts +++ b/packages/components/src/theme/ThemeUtils.test.ts @@ -59,9 +59,9 @@ describe('calculatePreloadStyleContent', () => { } it('should set defaults if css variables are not defined', () => { - expect(calculatePreloadStyleContent()).toEqual( - expectedContent(DEFAULT_PRELOAD_DATA_VARIABLES) - ); + expect( + calculatePreloadStyleContent(DEFAULT_PRELOAD_DATA_VARIABLES) + ).toEqual(expectedContent(DEFAULT_PRELOAD_DATA_VARIABLES)); }); it('should resolve css variables', () => { @@ -71,7 +71,9 @@ describe('calculatePreloadStyleContent', () => { ); document.body.style.setProperty('--dh-color-bg', 'orange'); - expect(calculatePreloadStyleContent()).toEqual( + expect( + calculatePreloadStyleContent(DEFAULT_PRELOAD_DATA_VARIABLES) + ).toEqual( expectedContent({ ...DEFAULT_PRELOAD_DATA_VARIABLES, '--dh-color-loading-spinner-primary': 'pink', @@ -96,7 +98,10 @@ describe.each([document.body, document.createElement('div')])( it('should return empty string if property does not exist and no default value exists', () => { asMock(computedStyle.getPropertyValue).mockReturnValue(''); - const resolver = createCssVariableResolver(targetElement); + const resolver = createCssVariableResolver( + targetElement, + DEFAULT_PRELOAD_DATA_VARIABLES + ); expect(getComputedStyle).toHaveBeenCalledWith(targetElement); @@ -113,7 +118,10 @@ describe.each([document.body, document.createElement('div')])( (key, value) => { asMock(computedStyle.getPropertyValue).mockReturnValue(''); - const resolver = createCssVariableResolver(targetElement); + const resolver = createCssVariableResolver( + targetElement, + DEFAULT_PRELOAD_DATA_VARIABLES + ); expect(getComputedStyle).toHaveBeenCalledWith(targetElement); @@ -126,7 +134,10 @@ describe.each([document.body, document.createElement('div')])( name => `resolved:${name}` ); - const resolver = createCssVariableResolver(targetElement); + const resolver = createCssVariableResolver( + targetElement, + DEFAULT_PRELOAD_DATA_VARIABLES + ); expect(getComputedStyle).toHaveBeenCalledWith(targetElement); @@ -387,7 +398,7 @@ describe('overrideSVGFillColors', () => { : 'red' ); - overrideSVGFillColors(); + overrideSVGFillColors(DEFAULT_PRELOAD_DATA_VARIABLES); expect(getComputedStyle).toHaveBeenCalledWith(document.body); expect(document.body.style.removeProperty).toHaveBeenCalledWith(key); @@ -418,12 +429,22 @@ describe('preloadTheme', () => { preloadTheme(); - const styleEl = document.querySelector('style'); + const [styleElDefaults, styleElPrevious] = + document.querySelectorAll('style'); - expect(styleEl).not.toBeNull(); - expect(styleEl?.innerHTML).toEqual( - preloadData?.preloadStyleContent ?? calculatePreloadStyleContent() + expect(styleElDefaults).not.toBeNull(); + expect(styleElDefaults?.innerHTML).toEqual( + calculatePreloadStyleContent(DEFAULT_PRELOAD_DATA_VARIABLES) ); + + if (preloadData?.preloadStyleContent == null) { + expect(styleElPrevious).toBeUndefined(); + } else { + expect(styleElPrevious).toBeDefined(); + expect(styleElPrevious?.innerHTML).toEqual( + preloadData?.preloadStyleContent + ); + } }); }); diff --git a/packages/components/src/theme/ThemeUtils.ts b/packages/components/src/theme/ThemeUtils.ts index 6211e1ecfa..4b9cc44073 100644 --- a/packages/components/src/theme/ThemeUtils.ts +++ b/packages/components/src/theme/ThemeUtils.ts @@ -31,13 +31,19 @@ export type VarExpressionResolver = (varExpression: string) => string; * happens before themes are fully loaded so that we can style things like the * loading spinner and background color which are shown to the user early on in * the app lifecycle. + * @defaultPreloadValues Default values to use if a preload variable is not set. */ -export function calculatePreloadStyleContent(): CssVariableStyleContent { - const resolveVar = createCssVariableResolver(document.body); +export function calculatePreloadStyleContent( + defaultPreloadValues: Record +): CssVariableStyleContent { + const resolveVar = createCssVariableResolver( + document.body, + defaultPreloadValues + ); // Calculate the current preload variables. If the variable is not set, use // the default value. - const pairs = Object.keys(DEFAULT_PRELOAD_DATA_VARIABLES).map( + const pairs = Object.keys(defaultPreloadValues).map( key => `${key}:${resolveVar(key as ThemePreloadColorVariable)}` ); @@ -47,12 +53,14 @@ export function calculatePreloadStyleContent(): CssVariableStyleContent { /** * Create a resolver function for calculating the value of a css variable based * on a given element's computed style. If the variable resolves to '', we check - * DEFAULT_PRELOAD_DATA_VARIABLES for a default value, and if one does not exist, + * `defaultValues` for a default value, and if one does not exist, * return ''. * @param el Element to resolve css variables against + * @param defaultValues Default values to use if a variable is not set. */ export function createCssVariableResolver( - el: Element + el: Element, + defaultValues: Record ): (varName: ThemeCssVariableName) => string { const computedStyle = getComputedStyle(el); @@ -67,12 +75,25 @@ export function createCssVariableResolver( return value; } - return ( - DEFAULT_PRELOAD_DATA_VARIABLES[varName as ThemePreloadColorVariable] ?? '' - ); + return defaultValues[varName as ThemePreloadColorVariable] ?? ''; }; } +/** + * Create a style tag containing preload css variables and add to the head. + * @param id The id of the style tag + * @param preloadStyleContent The css variable content to add to the style tag + */ +export function createPreloadStyleElement( + id: `theme-preload-${string}`, + preloadStyleContent: CssVariableStyleContent +): void { + const style = document.createElement('style'); + style.id = id; + style.innerHTML = preloadStyleContent; + document.head.appendChild(style); +} + /** * Extracts all css variable expressions from the given record and returns * a set of unique expressions. @@ -378,18 +399,35 @@ export function getThemeKey(pluginName: string, themeName: string): string { /** * Preload minimal theme variables from the cache. + * @defaultPreloadValues Optional default values to use if a preload variable is not set. */ -export function preloadTheme(): void { - const preloadStyleContent = - getThemePreloadData()?.preloadStyleContent ?? - calculatePreloadStyleContent(); +export function preloadTheme( + defaultPreloadValues: Record = DEFAULT_PRELOAD_DATA_VARIABLES +): void { + const previousPreloadStyleContent = + getThemePreloadData()?.preloadStyleContent; + + const defaultPreloadStyleContent = + calculatePreloadStyleContent(defaultPreloadValues); + + log.debug('Preloading theme content:', { + defaultPreloadStyleContent, + previousPreloadStyleContent, + }); - log.debug('Preloading theme content:', `'${preloadStyleContent}'`); + createPreloadStyleElement( + 'theme-preload-defaults', + defaultPreloadStyleContent + ); - const style = document.createElement('style'); - style.id = 'theme-preload'; - style.innerHTML = preloadStyleContent; - document.head.appendChild(style); + // Any preload variables that were saved by last theme load should override + // the defaults + if (previousPreloadStyleContent != null) { + createPreloadStyleElement( + 'theme-preload-previous', + previousPreloadStyleContent + ); + } } /** @@ -406,9 +444,12 @@ export function preloadTheme(): void { * just change the background color instead of relying on this util, but this * is not always possible. e.g.