Enable themes when custom CSS is enabled

[manuhabitela]

Describe the problem to be solved

Hey there 👋

For now, theme selection in user preferences is disabled when custom CSS is enabled.

This currently prevents a user to select the dark theme on an instance having custom CSS.

In the near future, some ongoing theme-related work will be finished. That will allow us to add, at least, a "high contrast" light theme to help people needing it. Again this won't be able on instances with custom CSS.

I don't think enabling custom CSS should, as a feature, result in themes disabled. Because the user pays the price: he can not select a dark theme, he can not select a more-distinguishable theme if he needs to (high contrast). I'm more inclined to say that, if you need custom CSS, you should make your custom CSS working with all possible themes. For example in our dinum instance, I feel like it's our responsability to write custom CSS that works with all themes, rather than only have one theme.

My guess @georgegevoian is you agree with me and this choice was made at the time so that themes can be shipped without creating unknown problems.

Describe the solution you would like

I see two ways of fixing this.

In both cases, one thing we would do first is add a new classname or attribute on the html tag with the current theme name. So that custom.css can have theme-specific code. This will be needed in addition to the current color-scheme rule, to distinguish light theme and high contrast light theme for example.

Then, either:

• just remove the enableCustomCss checks in the code so that themes are always enabled
• or add a new enableThemes config that would be enabled by default, unless enableCustomCss is true, to match current behavior. A new GRIST_ENABLE_THEMES env var would let us force the value of this config, allowing us to both have custom css and themes enabled explicitely.

First solution is straightforward but can easily break stuff for people currently using custom css and updating grist without paying too much attention on this change.

Second solution prevents surprises when updating grist. But that's the only benefit. It has a cost of adding a config/env var basically only there to help in the update. And it has the side effect of silently disabling themes if you happen to enable custom css, which I think is not what we'd want when moving forward (see above). I guess I explain this solution mostly to explain why I think it's not what we'd actually want haha.

I guess a middle ground would be:

• add the new theme classname on the html tag so that custom css can target themes
• warn people currently using custom css, in the changelog, or a console.warn at load, that custom css logic will change soon and that you can already update your custom css logic to target specific themes
• then in a later update, enable themes selection even when custom css is enabled.

That way current users are warned and we don't introduce any new code.

What do you think? If you agree with the idea I can take care of this after the first step with theme refactoring is done. Thanks!

[georgegevoian]

Custom CSS is a feature that's effectively an escape hatch - a last resort when there's no other option to achieve the customization you want. With the improvements to themes you're working on now, would you still have a need for custom CSS in your instance at DINUM for anything?

[manuhabitela]

Our use of custom css is mostly to change the primary color. Maybe I'm missing something but I don't think there is any way to do this without custom css? I mean I can't change a theme file or add a custom theme with some config, if I want to do this I'd need to change files locally before building.

With the rework of themes, I think we still need custom CSS to changes colors. But we can change variables in a more semantic way, instead of overriding the "light green" variable to say it's blue (which is a bit weird and misleading to maintain), we'd change the "light primary" variable for example.

[georgegevoian]

Oh, I see now. You're correct that there's no other way to override themes currently.

Going back to your original question: I wonder if there's a way to reuse the GRIST_UI_FEATURES env variable to accomplish the same as your second proposal. Perhaps it can default to including themes in the list, and treat its absence the same (unless, of course, GRIST_HIDE_UI_ELEMENTS explicitly includes themes or APP_STATIC_INCLUDE_CUSTOM_CSS is set). Then the UI can add the appropriate checks to disable or avoid rendering elements related to themes. The behavior is a little complex and unintuitive, but it might be enough to keep us moving for now without needing to change existing behavior or introduce new environment variables. How does that sound?