Skip to content

Commit

Permalink
(themes) refactor theme variables loading to improve maintenance
Browse files Browse the repository at this point in the history
Until now themes described each component variable. This made
creating a theme difficult: we had to override a bunch of hard-coded
component variables (around 435 total) if we wanted to change one color
shade used globally.

Some CSS variables were not defined in a theme, but as theme-agnostic
tokens, in cssVars.ts. But those tokens applied values related to the
light theme, and couldn't be overriden easily in the dark theme.

Also, the theme-related css variables were defined in cssVars, with
default values, matching the ones in the light theme. This lead to some
confusion because sometimes the fallback value described in cssVar
wasn't in sync with the actual value in theme.

The idea of this commit is to make it easier to write theme and update
theme files:

- a theme file now describes around 20 main design tokens, defining the
main text color, backgrounds, primary colors, etc. The hundreds of
variables it defined previously are now inside a `components` key to
explicitely show they are more specific. Most of those variables target
one of the main design tokens to help with updating the styles
globally.

- a new Base.ts theme file was added, that the actual theme files use as
a… base. It contains the design tokens we assume won't change 99% of the
time.

- to prevent too much context switching in our dev heads, now theme
tokens are defined with js object keys styled names, instead of css var
names (ie bgSecondary instead of 'bg-secondary').

- a new theme `tokens` object exposes all the CSS variables related to
the current theme, that the codebase can consume. They are meant to be
used just like cssVars `colors` and ` vars` are used today, and actually
the idea would be to eventually remove those cssVars exports so that
`tokens` is the only one used. Some `vars` were not migrated to a
matching `token` because they were not used in the codebase, but we kept
it to not break custom css/custom widget that may use the variables.

- a new theme `components` object exposes all the previous cssVars
`theme` variables. The current cssVars `theme` object just consumes this
new object, but we could also imagine removing it altogether in the
future.

- for all of this to work, now a theme is always loaded, even when
custom CSS is enabled, or when theme choice is actually unsupported
(forms). In this latter cases, the light theme is loaded.

Limitations:

- I tried to have more "theme-agnostic" variable names for the gray
shades and the primary color (green). The tricky part is, in the light
theme there are way less dark-shades and green shades used than in the
dark theme (colors are not a 1:1 match) so it's a bit hard to come up
with generic, theme-agnostic tokens. This can certainly be improved but
my guess is now is a good middle ground.

- The switch to js object keys names made me create big mapping objects
between js key and css variables, resulting in 500+ lines of code added
to the app bundle and the grist-plugin-api.js file. I'm really not happy
with this and would like to find a better solution.

Before/after stats for the theme "components" variables:
  - a theme had around 435 variables set before, all hard-coded strings
  - now a Base theme, common to dark and light, defines around 290
values, and around 10 of those are hard-coded, the other target the more
generic tokens, making it easier to update
  - a specific theme (dark or light) defines around 150 variables. In
each theme, around 60 of those are hard-coded.

That means while there is still a big room for improvement, themes
should be noticeably easier to maintain, as most values come from
generic tokens.
  • Loading branch information
manuhabitela committed Feb 2, 2025
1 parent eb45410 commit c54bcde
Show file tree
Hide file tree
Showing 16 changed files with 2,450 additions and 3,149 deletions.
1 change: 0 additions & 1 deletion app/client/app.css
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
/* global variables */
@layer grist-base, grist-theme, grist-custom;
:root {
--color-logo-row: #F9AE41;
--color-logo-col: #2CB0AF;
Expand Down
12 changes: 6 additions & 6 deletions app/client/components/ChartView.ts
Original file line number Diff line number Diff line change
Expand Up @@ -395,19 +395,19 @@ export class ChartView extends Disposable {
private _getPlotlyTheme(): Partial<Layout> {
const {colors} = gristThemeObs().get();
return {
paper_bgcolor: colors['chart-bg'],
plot_bgcolor: colors['chart-bg'],
paper_bgcolor: colors.components.chartBg.toString(),
plot_bgcolor: colors.components.chartBg.toString(),
xaxis: {
color: colors['chart-x-axis'],
color: colors.components.chartXAxis.toString(),
},
yaxis: {
color: colors['chart-y-axis'],
color: colors.components.chartYAxis.toString(),
},
font: {
color: colors['chart-fg'],
color: colors.components.chartFg.toString(),
},
legend: {
bgcolor: colors['chart-legend-bg'],
bgcolor: colors.components.chartLegendBg.toString(),
},
};
}
Expand Down
3 changes: 1 addition & 2 deletions app/client/models/AppModel.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import {LocalPlugin} from 'app/common/plugin';
import {DismissedPopup, DismissedReminder, UserPrefs} from 'app/common/Prefs';
import {isOwner, isOwnerOrEditor} from 'app/common/roles';
import {getTagManagerScript} from 'app/common/tagManager';
import {getDefaultThemePrefs, ThemePrefs, ThemePrefsChecker} from 'app/common/ThemePrefs';
import {getDefaultThemePrefs, ThemePrefs} from 'app/common/ThemePrefs';
import {getGristConfig} from 'app/common/urlUtils';
import {ExtendedUser} from 'app/common/UserAPI';
import {getOrgName, isTemplatesOrg, Organization, OrgError, UserAPI, UserAPIImpl} from 'app/common/UserAPI';
Expand Down Expand Up @@ -305,7 +305,6 @@ export class AppModelImpl extends Disposable implements AppModel {
public readonly userPrefsObs = getUserPrefsObs(this);
public readonly themePrefs = getUserPrefObs(this.userPrefsObs, 'theme', {
defaultValue: getDefaultThemePrefs(),
checker: ThemePrefsChecker,
}) as Observable<ThemePrefs>;

public readonly dismissedPopups = getUserPrefObs(this.userPrefsObs, 'dismissedPopups',
Expand Down
8 changes: 6 additions & 2 deletions app/client/ui/createPage.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import {Notifier} from 'app/client/models/NotifyModel';
import {buildSnackbarDom} from 'app/client/ui/NotifyUI';
import {addViewportTag} from 'app/client/ui/viewport';
import {attachCssRootVars} from 'app/client/ui2018/cssVars';
import {attachTheme} from 'app/client/ui2018/theme';
import {attachDefaultLightTheme, attachTheme} from 'app/client/ui2018/theme';
import {BaseAPI} from 'app/common/BaseAPI';
import {dom, DomContents} from 'grainjs';

Expand All @@ -22,7 +22,11 @@ export function createPage(buildPage: () => DomContents, options: {disableTheme?

addViewportTag();
attachCssRootVars('grist');
if (!disableTheme) { attachTheme(); }
if (disableTheme) {
attachDefaultLightTheme();
} else {
attachTheme();
}
setupLocale().catch(reportError);

// Add globals needed by test utils.
Expand Down
987 changes: 112 additions & 875 deletions app/client/ui2018/cssVars.ts

Large diffs are not rendered by default.

65 changes: 47 additions & 18 deletions app/client/ui2018/theme.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,14 @@ import { createPausableObs, PausableObservable } from 'app/client/lib/pausableOb
import { getStorage } from 'app/client/lib/storage';
import { getOrCreateStyleElement } from 'app/client/lib/getOrCreateStyleElement';
import { urlState } from 'app/client/models/gristUrlState';
import { Theme, ThemeAppearance, ThemeColors, ThemePrefs } from 'app/common/ThemePrefs';
import { getThemeColors } from 'app/common/Themes';
import { components, Theme, ThemeAppearance, ThemePrefs, ThemeTokens, tokens } from 'app/common/ThemePrefs';
import { getThemeTokens } from 'app/common/Themes';
import { getGristConfig } from 'app/common/urlUtils';
import { Computed, Observable } from 'grainjs';
import isEqual from 'lodash/isEqual';

const DEFAULT_LIGHT_THEME: Theme = {appearance: 'light', colors: getThemeColors('GristLight')};
const DEFAULT_DARK_THEME: Theme = {appearance: 'dark', colors: getThemeColors('GristDark')};
const DEFAULT_LIGHT_THEME: Theme = {appearance: 'light', colors: getThemeTokens('GristLight')};
const DEFAULT_DARK_THEME: Theme = {appearance: 'dark', colors: getThemeTokens('GristDark')};

/**
* A singleton observable for the current user's Grist theme preferences.
Expand Down Expand Up @@ -67,11 +67,11 @@ export function gristThemeObs() {
/**
* Attaches the current theme's CSS variables to the document, and
* re-attaches them whenever the theme changes.
*
* When custom CSS is enabled (and theme selection is then unavailable in the UI),
* default light theme variables are attached.
*/
export function attachTheme() {
// Custom CSS is incompatible with custom themes.
if (getGristConfig().enableCustomCss) { return; }

// Attach the current theme's variables to the DOM.
attachCssThemeVars(gristThemeObs().get());

Expand All @@ -83,6 +83,16 @@ export function attachTheme() {
});
}

/**
* Attaches the default light theme to the DOM.
*
* In some cases, theme choice is disabled (for example, in forms), but we still need to
* append the theme vars to the DOM so that the UI works.
*/
export function attachDefaultLightTheme() {
attachCssThemeVars(DEFAULT_LIGHT_THEME);
}

/**
* Returns the `Theme` from the given `themePrefs`.
*
Expand All @@ -104,25 +114,44 @@ function getThemeFromPrefs(themePrefs: ThemePrefs, userAgentPrefersDarkTheme: bo
appearance = userAgentPrefersDarkTheme ? 'dark' : 'light';
}

let nameOrColors = themePrefs.colors[appearance];
let nameOrTokens = themePrefs.colors[appearance];
if (urlParams?.themeName) {
nameOrColors = urlParams?.themeName;
nameOrTokens = urlParams?.themeName;
}

let colors: ThemeColors;
if (typeof nameOrColors === 'string') {
colors = getThemeColors(nameOrColors);
let themeTokens: ThemeTokens;
if (typeof nameOrTokens === 'string') {
themeTokens = getThemeTokens(nameOrTokens);
} else {
colors = nameOrColors;
themeTokens = nameOrTokens;
}

return {appearance, colors};
return {appearance, colors: themeTokens};
}

function attachCssThemeVars({appearance, colors: themeColors}: Theme) {
// Prepare the custom properties needed for applying the theme.
const properties = Object.entries(themeColors)
.map(([name, value]) => `--grist-theme-${name}: ${value};`);
function attachCssThemeVars({appearance, colors: themeTokens}: Theme) {
const properties = Object.entries(themeTokens || {})
.filter(([name]) => name !== 'components')
.map(([tokenName, value]) => {
if (tokenName in tokens) {
const cssProp = tokens[tokenName as keyof typeof tokens];
cssProp.value = value;
return cssProp.decl();
}
return undefined;
})
.filter((prop): prop is string => prop !== undefined);

properties.push(...Object.entries(themeTokens.components || {})
.map(([tokenName, value]) => {
if (tokenName in components) {
const cssProp = components[tokenName as keyof typeof components];
cssProp.value = value;
return cssProp.decl();
}
return undefined;
})
.filter((prop): prop is string => prop !== undefined));

// Include properties for styling the scrollbar.
properties.push(...getCssThemeScrollbarProperties(appearance));
Expand Down
21 changes: 21 additions & 0 deletions app/common/CssCustomProp.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
const VAR_PREFIX = 'grist';

export class CssCustomProp {
constructor(public name: string, public value?: string | CssCustomProp, public fallback?: string | CssCustomProp) {

}

public decl(): string | undefined {
if (this.value === undefined) { return undefined; }

return `--${VAR_PREFIX}-${this.name}: ${this.value};`;
}

public toString(): string {
let value = `--${VAR_PREFIX}-${this.name}`;
if (this.fallback) {
value += `, ${this.fallback}`;
}
return `var(${value})`;
}
}
Loading

0 comments on commit c54bcde

Please sign in to comment.