Suggested implementation of themes refactoring

[manuhabitela]

Context

Here is an example of implementation following discussions in #1295 (up until George's comment).

Proposed solution

Everything is detailed in comments and commit messages, here is a summary of it:

• 1st commit: add CSS layers to make sure default variables and themes variables do not collapse (this is helpful because the idea of this implementation is a theme can now directly override default variables)
• 2nd commit: instead of having cssVars.colors and cssVars.vars, have one big unique object that holds all the design tokens (cssVars.designTokens), that can all be overriden by a theme. Contrary to before where default colors could not be changed. The idea would be that the new object replaces the 2 others.
• 3rd commit: in cssVars.theme, make the specific component variables target the new design tokens. In the end, in each Theme, specific component variables can be removed, and we can decide to define only the more global design token values (see GristDark).
  • In a different branch, I also tried another usage as an alternative to 3rd commit: removing component-specific variables. This leads to more code changes across files but also to a smaller global theme-related code in the end.

Questions that popped up:

• on this implementation, now a theme has the possibility to define only a few variables, with the default values of cssVars being used for the undefined variables. I'm not sure this is a great idea as it could lead to oversights when implementing new themes or updating current ones. I'm not sure it's actually an issue either. As we can easily create a new Theme by extending an existing one anyway. What do you think?

Related issues

#1295

Has this been tested?

• 👍 yes, I added tests to the test suite
• 💭 no, because this PR is a draft and still needs work
• 🙅 no, because this is not relevant here
• 🙋 no, because I need help

ping @dsagal @georgegevoian

[georgegevoian]

Approach makes sense to me. Thank you.

on this implementation, now a theme has the possibility to define only a few variables, with the default values of cssVars being used for the undefined variables. I'm not sure this is a great idea as it could lead to oversights when implementing new themes or updating current ones. I'm not sure it's actually an issue either. As we can easily create a new Theme by extending an existing one anyway. What do you think?

Making all the tokens required in ThemeColors would be my preference. In fact, I'd be interested in taking the refactor further and trying to get it down to 3 layers: grist-base, grist-theme, and grist-custom. My thinking is a theme should always be applied, so we could try always applying the light theme on initial setup (i.e. from attachCssRootVars), which would replace the role that grist-tokens currently serves. Do you see any problems with doing that?

[manuhabitela]

Approach makes sense to me. Thank you.

Great :)

Making all the tokens required in ThemeColors would be my preference

So to be 100% clear, you mean at least this?

• a theme object (like GristLight.ts), should define at its root, all the colors, the paddings, font sizes, z-indexes, etc, as now the "design tokens" are a mix between colors and vars,

But do you also mean this?

• it should define in its legacyVariables prop (current naming in the PR), the 400+ component-specific variables, with stuff mostly targeting the design tokens.

I'd argue if we think those component-specific variables are kinda legacy and we eventually want to remove them, I'd say making them required in a theme object is maybe too much trouble? Since the idea is themes would not want to change those, as cssVars.theme would mostly default to design tokens.

Here are a few thoughts:

• if a theme *has* to implement all designTokens, there would be no need to define any value in cssVars.designTokens, since all actual values would always come from the current theme. The definition of those objects would then mostly help only to easily target css variables from the JS code when writing css. Maybe I could update the code to more reflect that and stop defining actual values in designTokens, but only define css variable keys?
• would it make sense to change how we set up values in a theme? Instead of having props matching css variable names, having them match the designTokens and theme keys. Maybe feels weird to have all the code target the "js props" while a theme targets "css variables"?
• I'd say having some 'BaseTheme' object that lists all the default stuff would be helpful. Like all the padding/fonts/zindex. So that a theme could be easily defined by listing all its colors, that wouldn't be in the BaseTheme, and merging the BaseTheme, for all the stuff that is common.

In fact, I'd be interested in taking the refactor further and trying to get it down to 3 layers: grist-base, grist-theme, and grist-custom. My thinking is a theme should always be applied, so we could try always applying the light theme on initial setup (i.e. from attachCssRootVars), which would replace the role that grist-tokens currently serves.

Yeah I guess this way of doing was more to show the structure, indeed "grist-tokens" is not necessary. I'd argue if *everything* is listed in a theme object, the concept of "grist-base" variables is also debatable: every design token variable is tied to the theme, and then there is the custom css layer that comes on top.

[georgegevoian]

So to be 100% clear, you mean at least this?

a theme object (like GristLight.ts), should define at its root, all the colors, the paddings, font sizes, z-indexes, etc, as now > the "design tokens" are a mix between colors and vars,

Only the variables we'd like to change from theme to theme. So yes to colors, but no to z-indexes.

Padding and font size are more interesting; I can see an argument that making them required would be more of a nuisance, as they won't always change across themes. A compromise might be to make the vars (e.g. fonts, padding, etc.) optional, and the colors required, with the vars being inherited from the base layer.

But do you also mean this?

it should define in its legacyVariables prop (current naming in the PR), the 400+ component-specific variables, with stuff mostly targeting the design tokens.
I'd argue if we think those component-specific variables are kinda legacy and we eventually want to remove them, I'd say making them required in a theme object is maybe too much trouble? Since the idea is themes would not want to change > those, as cssVars.theme would mostly default to design tokens.

No, I agree that it's too much trouble to make those required.

Here are a few thoughts:

• if a theme has to implement all designTokens, there would be no need to define any value in cssVars.designTokens, since all actual values would always come from the current theme. The definition of those objects would then mostly help only to easily target css variables from the JS code when writing css. Maybe I could update the code to more reflect that and stop defining actual values in designTokens, but only define css variable keys?
• would it make sense to change how we set up values in a theme? Instead of having props matching css variable names, having them match the designTokens and theme keys. Maybe feels weird to have all the code target the "js props" while a theme targets "css variables"?
• I'd say having some 'BaseTheme' object that lists all the default stuff would be helpful. Like all the padding/fonts/zindex. So that a theme could be easily defined by listing all its colors, that wouldn't be in the BaseTheme, and merging the BaseTheme, for all the stuff that is common.

I had a similar thing in mind. A base theme defining the default, non-color variables makes a lot of sense to me. The current system is awkward because things live in two places, as you said. If we can narrow that down to one, that'd be great!

[manuhabitela]

Hey @georgegevoian,

Here is an update to move things forward.

Summary

The idea is:

• Actual values are now only in themes, except for css vars that are really theme-agnostic, which means now only the z-index list. All other variables (colors, font families, paddings, font sizes, etc) can now be modified in a theme.

• New tokens and legacyVariables objects list CustomProp instances for all theme tokens. These are here mostly so that the codebase can consume the theme tokens.
  For example, instead of writing this in a component code:

  import {colors, vars, theme} from 'app/client/ui2018/cssVars';
  export const button = styled('button', `
    font-size: ${vars.mediumFontSize};
    color: ${colors.slate};
    background-color: ${theme.controlPrimaryBg};
  `);

  We would now write:

  import {tokens, legacyVariables} from 'app/common/ThemePrefs';
  export const button = styled('button', `
    font-size: ${tokens.mediumFontSize};
    color: ${tokens.slate};
    background-color: ${legacyVariables.controlPrimaryBg};
  `);

  For now, to prevent changing hundreds of files and have an even more difficult diff to read (and also while this is still an ongoing discussion :)), old cssVars colors, vars and theme are still there, but just target the new objects.

• A new Base theme implements the theme tokens that we assume won't change much between actual themes. It mostly lists old vars and theme variables, the grayscale that was in colors, and added generic variables. The theme variables are listed under a legacyVariables property as suggested before.

• Each theme must describe mandatory keys, that are not in the Base theme: specific colors.

• I started some work to add some generic variables to let us implement less things on a specific theme's legacyVariables. You can see around 230 legacy variables now target generic tokens, while 200 others are still implemented in each theme. This gist helps in finding common values mapping between the light and dark themes.

Feedback needed

Variable namings and impact on widget's styling

I made the choice to abstract the css variable names as much as possible. The goal is that we don't have to regularly switch in our heads between the consumed in the codebase name (tokens.textLight), vs the described in the theme name (theme-color-text-light).

So there is some code to map some js object keys to existing css var names.

Unfortunately I only figured out later that theme variables in widget iframes was loaded differently. And from what I understand I can't import common files in the plugin-related code? Which means for now I can't go find the css variable name matching the theme key in the plugin code, like I do in the default attachCssThemeVars.

Any advice on what I could do there?

My intuition would be to actually remove the js object key <> css var name manual mapping and just generate css variable names based on the JS keys. So that tokens.textLight would generate a css variable --grist-textLight automatically. I did a manual mapping and kept the current css names for potential backward compatibility with custom css files and to prevent having to update files here and there directly consuming css variables… But your input would be helpful, from my limited point of view I'd find it simpler if we didn't have to handle this mapping. I actually don't get what is the value in handling ourselves the css variable names instead of relying on our JS namings that we use across most of the app.

TS checkers

I had trouble with the ts-interface-checker related code, because it doesn't handle generics. I ended up just… removing the ThemePrefs-ti file. So I replaced a specific part of the app where some checkers were used (in gristUrls) to make the check manually. And I removed the checker in AppModel. I actually don't understand that much why this checker is needed here, since the themes are checked by typescript at build time and from my understand at this stage the theme can't come from a random runtime value?

I'm not against a little bit of explanation here if what I did is not ok, I assume I missed something :)

Next steps

If what you see seems globally find for you, here what I wanted to continue on:

• fix the theme variables loading for iframes
• make sure themes are always loaded whatever the case now, thinking for example of Forms
• actually remove cssVars.ts colors, vars and theme and make the whole codebase target the new tokens variable. I guess changing this in one big commit would be better than leaving 2 ways of consuming design tokens in the codebase
• think a little more about new generic names for the legacyVariables, meaning, continuing the work I started with the 230 vs 200 legacyVariables (see above). This includes both putting out of "legacyVariables" some interesting variables (like input or control related ones), and coming up with new generic variables that legacy variables would then consume. My goal is not to clean this very thoroughly as there are a lot of stuff. But at least cleaning the variables used in multiple places and come up with better namings. I don't see a lot of value in taking hours cleaning all the variables used in only specific places, but I see some in cleaning the variables used more globally.

What do you think?

Thanks a lot.

[georgegevoian]

Actual values are now only in themes, except for css vars that are really theme-agnostic, which means now only the z-index list. All other variables (colors, font families, paddings, font sizes, etc) can now be modified in a theme.

Sounds good. One small suggestion - and only if it's simple to implement - would be to split the exported tokens object into a grouping like colors, fontSizes, fontFamilies, etc. The motivation is purely to have shorter, more semantic variable references in code. Here's how I envision your example from earlier could look:

import {colors, fontSizes, legacyVariables} from 'app/common/ThemePrefs';
export const button = styled('button', `
  font-size: ${fontSizes.medium};
  color: ${colors.slate};
  background-color: ${legacyVariables.controlPrimaryBg};
`);

How does that look to you? I like it a bit better, but I'm also fine with how you've done it now.

TS checkers
I had trouble with the ts-interface-checker related code, because it doesn't handle generics. I ended up just… removing the ThemePrefs-ti file. So I replaced a specific part of the app where some checkers were used (in gristUrls) to make the check manually. And I removed the checker in AppModel. I actually don't understand that much why this checker is needed here, since the themes are checked by typescript at build time and from my understand at this stage the theme can't come from a random runtime value?

I'm not against a little bit of explanation here if what I did is not ok, I assume I missed something :)

Ok to remove and not worry about, for now. It was done very early on for the (ambitious) goal of supporting custom themes (where users could specify the values of individual variables). The checkers alone aren't sufficient for validation anyway (they don't actually check the values are valid CSS colors).

My intuition would be to actually remove the js object key <> css var name manual mapping and just generate css variable names based on the JS keys.

For preserving backward compatibility, it's really just the properties in colors and vars that need to be preserved. We've never publicly documented the properties in theme. Perhaps you can keep the manual mapping of those to their current names, and default to generating variable names for the remaining keys (which would shave off most of the redundant mappings).

The easiest fix for the plugin theme handling would be to move whatever theme-related code you need to /app/plugin/.... (Could maybe go in a file called GristTheme.ts.) Not being able to import from common code should be fixable, but the configuration is all Webpack, so it may be more painful than moving around the code.

I guess changing this in one big commit would be better than leaving 2 ways of consuming design tokens in the codebase

Actually, one big commit may be problematic because this repo doesn't contain all the UI code bundled in the SaaS build of Grist. Let's plan on having 2 ways of consuming tokens, with the aim of getting all the code using the old way migrated over fairly quickly in follow-up PRs. That'll give us some time to update any SaaS-only code to use the new variables.

Besides that, I think your plan for next steps sounds good. Thanks @manuhabitela.

[manuhabitela]

Thanks for the quick feedback :)

Sounds good. One small suggestion - and only if it's simple to implement - would be to split the exported tokens object into a grouping like colors, fontSizes, fontFamilies, etc. The motivation is purely to have shorter, more semantic variable references in code. Here's how I envision your example from earlier could look:
[...]

Sure. I can also make theme files have the tokens listed with this structure so that everything is consistent.

For preserving backward compatibility, it's really just the properties in colors and vars that need to be preserved. We've never publicly documented the properties in theme. Perhaps you can keep the manual mapping of those to their current names, and default to generating variable names for the remaining keys (which would shave off most of the redundant mappings).

Sorry I'm not sure I understood correctly. You mean keep the current mapping for old colors and vars, but don't worry about the theme (now legacyVariables) mapping anymore and just auto-generate css variable names based on js keys for those? [1]

The easiest fix for the plugin theme handling would be to move whatever theme-related code you need to /app/plugin/.... (Could maybe go in a file called GristTheme.ts.) Not being able to import from common code should be fixable, but the configuration is all Webpack, so it may be more painful than moving around the code.

Okay, will try to see about the webpack imports and fallback to just moving code if I struggle too much, good to know thanks.

Actually, one big commit may be problematic because this repo doesn't contain all the UI code bundled in the SaaS build of Grist. Let's plan on having 2 ways of consuming tokens, with the aim of getting all the code using the old way migrated over fairly quickly in follow-up PRs. That'll give us some time to update any SaaS-only code to use the new variables.

Good to know, thanks. If I can allow myself to change the css variable names of legacyVariables right now though (like I understood in one [1]), is there not a risk of breaking external code using those variables then? Not talking about the theme export here that I understand should stay as is, but the actual css variable names.

[georgegevoian]

For preserving backward compatibility, it's really just the properties in colors and vars that need to be preserved. We've never publicly documented the properties in theme. Perhaps you can keep the manual mapping of those to their current names, and default to generating variable names for the remaining keys (which would shave off most of the redundant mappings).

Sorry I'm not sure I understood correctly. You mean keep the current mapping for old colors and vars, but don't worry about the theme (now legacyVariables) mapping anymore and just auto-generate css variable names based on js keys for those? [1]

Yes. In other words, anything in /static/custom.css should continue to work after any changes you make land.

Good to know, thanks. If I can allow myself to change the css variable names of legacyVariables right now though (like I understood in one [1]), is there not a risk of breaking external code using those variables then? Not talking about the theme export here that I understand should stay as is, but the actual css variable names.

You're correct that it can break external code referencing those variable names. The --grist-theme- variables have always been undocumented, so I have less of an issue with changing them. They'll break regardless whenever a component in the UI is updated to no longer use the legacy variables, right?

[manuhabitela]

Okay well I'll leave current js name <> css names mappings as is. It's "just" one file having a couple big mappings after all, and it will prevent any surprises.

Sounds good. One small suggestion - and only if it's simple to implement - would be to split the exported tokens object into a grouping like colors, fontSizes, fontFamilies, etc. The motivation is purely to have shorter, more semantic variable references in code. Here's how I envision your example from earlier could look:

import {colors, fontSizes, legacyVariables} from 'app/common/ThemePrefs';
export const button = styled('button', `
  font-size: ${fontSizes.medium};
  color: ${colors.slate};
  background-color: ${legacyVariables.controlPrimaryBg};
`);

How does that look to you? I like it a bit better, but I'm also fine with how you've done it now.

So actually I started changing things as you suggested but quickly stumbled upon cases where I started questioning what the categorization should be (like bigControlFontSize should be in a fontSizes export, or a controls export, a forms export, etc) or how granular it should be (should we have colors.gray.medium, colors.primary.light etc).

In the end I'd rather leave everything as is for now, it makes for a simpler switch (keep the same naming of previous colors+vars namings in the new tokens object). We could think about better names later maybe? I feel like this adds another layer of things to solve.

[georgegevoian]

In the end I'd rather leave everything as is for now, it makes for a simpler switch (keep the same naming of previous colors+vars namings in the new tokens object). We could think about better names later maybe? I feel like this adds another layer of things to solve.

Ok with me!

[manuhabitela]

Ok, big update, hopefully you don't hate me too much 😬

I basically wrote a novel to explain everything in the latest commit but here is the gist of it since last time:

• cssVars exports should export the same css variables as before (colors, vars and theme). But now, most of the exported variables target the new theme variables instead of having hard-coded string values. Note that some vars were not migrated in the new theme tokens, as I didn't find any usage of them in the codebase. I kept them for backward compatibility of external code but figured it wouldn't be helpful to keep them in some new code. Also, a couple variables have differences between their js name and css variable, surely typos made when they were created, but I didn't fix any to prevent breaking stuff.

• I renamed legacyVariables to simply components as in the end I think they are here to stay and grow unless there is some rework to try to have 1:1 matchs between light and dark generic design tokens. There were simply too many dark shades, gray shades, and green shades used in one theme but not the other, it didn't allow to easily come up with new generic tokens that could be used in both themes at the same time.

• After going through basically all the theme variables, I came up with a bunch of new generic theme tokens. Naming was a bit hard, because light and dark themes don't have the same number of colors at all and often don't match 1:1… But now naming is a bit more abstract, making it maybe less weird to understand that we deal with theme-agnostic tokens. If you are curious and want to have a headache, I ended up creating myself a helper app to have a bigger vision of grist colors: https://w2vtqv.csb.app/

• I updated the TS config of the plugin directory so that I could import common code and this allowed me to make the new theme tokens available in the grist-plugin-api.js file.

• Now, theme variable are always appended to the DOM, even in form pages, otherwise forms wouldn't work anymore, since most of cssVars exports don't have fallback values anymore and just target theme tokens. We just force the light theme in that case.

Questions/thoughts

⚠️ How to handle grist-plugin-api breaking changes?

Something alarms me about the grist-plugin-api changes.

From what I understand, most custom widgets rely on the grist-plugin-api.js file hosted on https://docs.getgrist.com. For example the grist calendar widget targets https://docs.getgrist.com/grist-plugin-api.js even on self-hosted instances.

That means that if we merge this as is, when https://docs.getgrist.com/grist-plugin-api.js is updated, self hosted instances that are not updated yet will break.

How do you manage breaking changes in this specific file? I guess this would not be the first time such a case occurred?

Shouldn't I come up with some tests in the grist-plugin-api file, to handle both "versions" of the theme loading?

Bundle size

1: The addition of my big js name <> css name mapping is not that big of deal for the app, as it just replaces the big cssVar theme object. But I must admit I'm not happy with that in the end because now it must be added also in grist-plugin-api, so it adds 500+ lines of dumb JS code to load in custom widgets.

2: And the fact that we decide to stop having fallback values written in cssVars (as a way to have only one place in the code where we define theme values, as we mentioned before), makes it mandatory for the forms to also load theme variables. Meaning we append 500+ variables for pages that don't really need them.

So I'm not quite sure about those choices, even if they should end up simplifying DX, I feel the cost is maybe a bit high. Your input is appreciated. I'm not sure what to do especially about (1).

Besides that, this is ready.

I still have one weird bug with charts not using correct styles, I will deal with this tomorrow. edit: bug is fixed.

[paulfitz]

That means that if we merge this as is, when https://docs.getgrist.com/grist-plugin-api.js is updated, self hosted instances that are not updated yet will break.
How do you manage breaking changes in this specific file? I guess this would not be the first time such a case occurred?

In practice we've done this by trying not to make breaking changes to that file. For theming, @georgegevoian is the expert, but as I understood it the logical involvement that file had in theming was to inject a bunch of material sent to it from the surrounding app. Is it at all possible that you can move some of your changes into the material the surrounding app sends to onThemeChange handler? Perhaps the handler would need to upgrade first, have that change disseminate, and then once all versions of Grist are up to date enough (including the one serving docs.getgrist.com/grist-plugin-api.js) you could make a second step to start sending revised theme material from apps? That's very vague, @georgegevoian might be able to be more specific or have a better idea.

[manuhabitela]

Okay thanks to your help guys I think I managed to understand better and find a better solution for the grist-plugin-api thing :) Will keep you posted.

[manuhabitela]

Alright!

Latest commit (that I'd fixup before merging) should make the grist-plugin-api issue go away.

The final idea is:

• stick to "js keys" naming for both consuming theme tokens in the app (like tokens.bgSecondary) and describing theme files (like GristLight.ts has a bgSecondary key). From my pov this is better than before, where we had to mentally switch between seeing theme.bgSecondary in the codebase, while writing bg-secondary in the theme files.

• but internally keep the way theme variables are attached to the dom (in the attachCssThemeVars functions) in order to prevent having to change the grist-plugin-api file. The trick is to parse the theme object and modify it before sending it to the grist-plugin-api context (with the new convertThemeKeysToCssVars function).

To make this work I had to adapt the new format to the old way of attaching variables, so I added a few tests to make sure we don't make mistakes when writing new theme tokens.

I think this is good to review @georgegevoian.

One thing I noticed when searching for code sending "theme messages", as in the grist-plugin-api, is code in SafeBrowser. Didn't check what this actually is but I may have to change stuff in there. This doesn't prevent to review this I think.

If any of you guys want to review @hexaltation @fflorent we can take some time in a call for context, I know this is a big chunk to swallow 🙈

[github-actions]

Deployed commit dcf06c8e7099a970b017583825ce9fb611fba72f as https://grist-manuhabitela-grist-core-issue-1295-theme-refactoring.fly.dev (until 2025-03-07T10:22:04.693Z)

[github-actions]

Deployed commit 62d0a688683afeb293663315c848d7fa1b713af3 as https://grist-manuhabitela-grist-core-issue-1295-theme-refactoring.fly.dev (until 2025-03-07T11:57:34.687Z)

[github-actions]

Deployed commit 62d304787cb253efb1bf6bb3d4457a1e6a30dc47 as https://grist-manuhabitela-grist-core-issue-1295-theme-refactoring.fly.dev (until 2025-03-07T13:51:16.897Z)