[Web] Refactor theme

[rudream]

e counterpart: https://github.com/gravitational/teleport.e/pull/1026

Purpose

This PR refactors our theme implementation in preparation for adding a light theme and updating our existing dark theme. Currently, our theme file has vague and confusing names as well as duplicate colours.

This PR doesn't change any existing colours, so everything in the app should appear the same. This theme file also isn't final and will likely need a few more tweaks as the new colours for the design system are finalized.

Apologies for the large PR, it wasn't possible to make these changes procedurally without breaking the app.

Implementation

This new structure for our primary colours is based on "elevation" (you can read more on this concept here). The colours are organized by the perceived depth of the elements in the UI. The further back an element is, the more "sunken" it is, and the more forwards it is, the more "elevated" it is (think CSS z-index). A sunken colour would represent the background of the app, surface would be the primary surface where most content is located (such as tables), and anything more elevated than that would be things such as popovers, menus, and dialogs.

I'd appreciate any suggestions on better ways to structure and name our colours.

[public-teleport-github-review-bot]

@rudream - this PR will require admin approval to merge due to its size. Consider breaking it up into a series smaller changes.

[hatched]

The PR would be a lot smaller without the yarn-error.log 😉

[zmb3]

Looks good to me. I ran the app and poked around various screens and nothing seemed out of place.

[ravicious]

This new structure for our primary colours is based on "elevation" (you can read more on this concept here. The colours are organized by the perceived depth of the elements in the UI. The further back an element is, the more "sunken" it is, and the more forwards it is, the more "elevated" it is (think CSS z-index).

I skimmed through the PR and the article and this approach does seem much better than the current one. I'm curious if there are any others that were considered?

I also wonder if long term we'll need to have something even more purpose-oriented (see gravitational/webapps#692 (comment)). That is, I wonder if the elevation-based approach will be enough when someone decides to make their own theme. From the looks of web/packages/design/src/theme/theme.ts it might turn out that it's enough but I'm not 100% sure because I've never done that. 😶‍🌫️

I wish I had any specific recommendations to give. But I think once you get to making a light theme it'll be clear if there's anything missing.

[kimlisa]

i'm curious, do we have a rule now where we should explicitly get colors from our theme? if we can't find the color in our theme, add it to the theme? because i'm pretty sure we have some rogue colors and or hard coded colors that wasn't from the theme (i'm guilty)

[kimlisa]

does this mean, it's a color to make a button popout? perhaps just popout?

[rudream]

This is actually a highlighted item on a dropdown. It's called popoutItem in relation to the theme.colors.levels.popout colour meant to be used for the background of the dropdown, I realize now that this naming is confusing. I'll rename this to highlightedPopout and move it out of buttons: {} and next to popout

[kimlisa]

i'm seeing brand used mostly to mean as a background color. but i think i also saw it used sparingly as a text color, input box hover? or border color. it would be nice if we could do something like this if we are to enforce the use of theme colors:

colors.input.someColor, colors.labels.someColor, colors.bg.someColor, colors.text.someColor, colors.button.border.someColor, so all of these might contain the same color, but it leaves out the guess work on what colors are available to me for certain elements, and it acts as like a doc as to what currently might be in use for this particular element?

sometimes i find myself finding the component where i thought i saw the same color being used, go into that component, and get the right color name.

we also already have a block for button, so why not for input, labels, etc. But maybe too much unnecessary work? Idk, i'm not requiring change, it's just a thought.

[ravicious]

colors.input.someColor, colors.labels.someColor, colors.bg.someColor, colors.text.someColor, colors.button.border.someColor, so all of these might contain the same color, but it leaves out the guess work on what colors are available to me for certain elements, and it acts as like a doc as to what currently might be in use for this particular element?

I suppose it depends on how we want to differentiate between different kinds of elements. If we want to change the look based on something like a kind prop, e.g. <Button kind="warning">, then having a separate set of colors seems to not be necessary. If we want to enable a more free-form mixing and matching then it'd make sense to document what is allowed for a given element. But my gut tells me it's best to restrict possible uses as much as possible.

[rudream]

i'm seeing brand used mostly to mean as a background color. but i think i also saw it used sparingly as a text color, input box hover? or border color.

The brand colours are the bright purple accent colours that are associated with Teleport (eg. our logo), I think it makes sense to keep them defined under brand rather than creating subcategories for specific components, since these are usually used minor accent colours and I don't think we will ever have components using different brand-themed purple accent colours.

I agree with @ravicious in that we should only need to break down our theme into subcategories ie colors.buttons when the components have different kinds with each kind having a unique set of colours. In the case of buttons, each button kind has varying background colours, border colour, colours for hover states etc., this makes it necessary to subcategorize them as opposed to having a large amount of colours defined directly in theme.colors such as theme.colors.buttonPrimaryDefault, theme.colors.buttonPrimaryHover, theme.colors.buttonBorderBorderHover etc.

So far, buttons has been the only component type where I have found this to be necessary. However with the new theme, there likely will be new unique designs for things like inputs and labels where it would make sense to do as you suggested and break them down into their own subgroupings. The theme in this PR is not final and there will likely be more cases introduced by the new themes that it needs to handle.

[ravicious]

if we can't find the color in our theme, add it to the theme? because i'm pretty sure we have some rogue colors and or hard coded colors that wasn't from the theme

I was going to say that I feel like we could add a color to the theme only when working on reusable components since adding a color to the theme requires you to find a good color for both contexts (the Web UI vs Connect).

But I realized that the situation is even more complex because having a light theme in the Web UI will be another context, not to mention people being able to create their own themes.

It feels like if you want to use a custom color, you have to add it to the theme while keeping the general principles of the theme in mind.

---

BTW, when the time comes to add support for custom themes, Grzegorz and I already did some research on how to safely handle letting people insert arbitrary CSS values in #21965.

[rudream]

do we have a rule now where we should explicitly get colors from our theme?

@kimlisa Yes, there might be some instances where it doesn't make sense to, but we want to make sure our themes (dark/light/custom) are able to manipulate colours properly.

[ravicious]

I was about to leave an approval but then I noticed that the suggestions in the command bar in Connect look different. ;)

Other than that I just left some minor comments and a forward-looking one about the general approach.

[ravicious]

I dove deeper into that linked Material Design article as well as their section on colors and I looked how elevation is implemented in their web components.

It looks like in Material, elevation is related strictly to representing the level on which any given element sits. It seems to describe things like shadows and opacity, not necessarily colors for which Material has a whole separate system.

The reason I looked closer at those Material Design articles is because I started to wonder why we need a separate set of colors for buttons and why it isn't grounded in the concept of elevation like the colors under levels. ;)

---

Anyway, looking at changes in components such as SideNav, it's clear how much more helpful this new approach is. I think it'll definitely make it easier to migrate towards the light theme and perhaps even unifying the colors of the Web UI and Connect.

Though before we let people add custom themes, I feel like we should consider if a more comprehensive and perhaps more coherent system wouldn't be more useful. I'm again thinking of the discrepancy between levels and buttons colors. The proposed system might be good enough for us internally but once we decide to support custom themes, it'd be nice to make it as easy to understand as possible. I feel like if I had to make a custom theme from scratch, I'd have a much easier time with the mentioned Material color system and its tokens rather than a set of colors based on elevations plus some other sets for text and buttons.

But again, this is just feedback for the future, the proposed solution here completely covers the goals you outlined in the PR description. I'm not even sure if we devs should take charge of designing the color systems, it feels like something that should involve the design team and be synced with their tools as well.

[ravicious]

@rudream Do you plan to backport this to v10 as well?

[ravicious]

Also, I'm going to take the liberty to include #23979 in your backports, just so that we don't have to create another set of PRs.

[rudream]

@rudream Do you plan to backport this to v10 as well?

I didn't plan on it, the reason I backported this at all was to make backporting new features that involve colours easier. Afaik we aren't adding any features to v10 anymore. The new themes are only going to be v13+

[ravicious]

I was thinking that it'd still make backporting some misc changes/bugfixes much easier. But since v13 is right around the corner I do agree that we can omit the backport here (🤞).