circuit-ui icon indicating copy to clipboard operation
circuit-ui copied to clipboard

[RFC] Add abstraction layer to theme

Open connor-baer opened this issue 4 years ago • 11 comments

Summarize the feature

I propose to add an abstraction layer on top of the current design tokens. Instead of referencing e.g. the base colors (p500) directly, components would use semantically named values (buttonColor).

Basic example

const theme = {
  colors: {
    p100: "#EDF4FC",
    p200: "#DAEAFF",
    p300: "#AFD0FE",
    p400: "#7FB5FF",
    p500: "#3388FF",
    p700: "#1760CE",
    p900: "#003C8B",
    // ...
  },
  // ...
};

// ...becomes...

const theme = {
  button: {
    primary: {
      color: colors.white,
      background: colors.p500
      // ...
    },
    // ...
  },
  // ...
};

It would be used like so:

const variantStyles = ({ theme, variant }) => css`
  color: ${theme.button[variant].color};
  background: ${theme.button[variant].backgroundColor};
  // ...
`;

const Button = styled.button(variantStyles, /* ... */);

Motivation

This approach has multiple benefits:

  • The semantic value names make it clear when and where a value should be used. This ensures consistency, e.g. when implementing a custom button.
  • The specific values allow for greater customization and themeing of the components. Want to keep blue as the primary color, but turn all primary buttons orange? That's possible now!
  • The previous point also applies to darkmode. A proper darkmode does not simply invert the colors but requires careful mapping of the colors.

Migration

This is a major breaking change and thus will need to be rolled out in stages:

  1. Define schema and add new semantic values to the theme.
  2. (needs investigation) Mark base values as deprecated by logging a warning in a getter function.
  3. Gradually migrate Circuit UI and our applications to use the new values.
  4. Remove the old, raw values in a major version.

connor-baer avatar Sep 28 '20 22:09 connor-baer

Okay, I have a few concerns with this addition:

  • As I see it, making clear where a color should be used is more of a design responsibility, as when I'm building a new screen I'm not the one deciding the colors it uses. So, perhaps this abstraction wouldn't need to be present in the code if visual specs already follow design system guidelines in the first place.
  • It could add more complexity in usage, however small; because it replaces getting a hex from the spec and finding its name in the theme (something simple) with searching for the color within the correct element (something that requires more careful usage).
  • Migration sounds potentially very cumbersome.

That said, I'm not sure I see the benefits being greater than the risks/costs of adding this abstraction layer (I don't know about other contexts, but I don't see conversations about implementing dark mode in the near future).

larimaza avatar Sep 30 '20 16:09 larimaza

As I see it, making clear where a color should be used is more of a design responsibility, as when I'm building a new screen I'm not the one deciding the colors it uses. So, perhaps this abstraction wouldn't need to be present in the code if visual specs already follow design system guidelines in the first place.

True. The abstraction layer would be a reflection of those design guidelines and thus would simplify communication between designers and developers.

It could add more complexity in usage, however small; because it replaces getting a hex from the spec and finding its name in the theme (something simple) with searching for the color within the correct element (something that requires more careful usage).

The design spec should include the semantic colors that should be used. That's not always the case, so there's some truth to this 👍

Migration sounds potentially very cumbersome.

That's my concern as well. Fortunately, it could be done gradually.

I don't see conversations about implementing dark mode in the near future

While there is no concrete timeline for it yet, this is very much on the roadmap and preliminary research and work is already in progress.

connor-baer avatar Oct 15 '20 09:10 connor-baer

Sorry, closed it accidentaly 😂

Hmm, if dark mode is on the roadmap and we're already working on it this might bring more benefits than I initially thought. That said, I'd like to know more about your thoughts on what a gradual migration would look like :)

larimaza avatar Oct 23 '20 15:10 larimaza

My idea for a gradual migration looks as follows:

  1. Add the new values to the theme so they can be used alongside the existing ones.
  2. Encourage users to switch to the new values, e.g. by adding a @deprecated JSDOC comment to the old values. Editors like VS Code should pick this up and strike-through the values to visually inform the user of their deprecation.
  3. After a reasonable amount of time once most old values have been replaced, we'd publish a breaking version to remove the old values.

connor-baer avatar Nov 27 '20 19:11 connor-baer

I think this is ultimately what have to do to have a proper "theme", like light and dark or brand-related themes. If I'm not mistaken, Brad Frost also has some examples around this in his Atomic Design book, where he explains how a single themable atomic design system scaled across multiple brands within a company (i.e., every component has specific properties that can be adjusted to make it look differently).

Every component in a design system needs to have a schema of themable CSS properties for every one of its variants. The styled system docs describe how a theme API using variants may look and why (pretty much what @connor-baer shared). With TypeScript, I think it would make sense for the components to export the types for their variant themes so they can be used in the application theme.

Of course, what a change like this requires is that design considers the themability of every component beforehand and provides these specifications to developers.

Regarding the API, at the moment -- unless this has changed -- I think every variant has it's own style function. I think it would make sense to keep that to cleanly separate the different variants. Using your example, I would make the following proposal.


// This proposed variant style
const variantStyles = ({ theme, variant }) => css`
  color: ${theme.button[variant].color};
  background: ${theme.button[variant].backgroundColor};
  // ...
`;

const Button = styled.button(variantStyles, /* ... */);

// becomes:
const primaryStyles = ({ theme, variant }) => variant === 'primary' && css`
  color: ${theme.button[variant].color};
  background: ${theme.button[variant].backgroundColor};
  // ...
`;

const Button = styled.button(primaryStyles, /* ... */);

I think this would make it easier to track the various variants, as their style schemas are likely to not overlap perfectly (i.e., you would have one function for the shared themable CSS properties and one for each variant with the variant specific styles).

felixjung avatar Dec 07 '20 11:12 felixjung

Ah, also I think this needs to happen outside of design-tokens. The themes are component-specific and therefore belong with the component library. The tokens are used as input to the component theme. You get a theme hierarchy where the tokens are the outer theme and the component themes are the inner theme. You can then nest these with multiple theme providers to prevent unnecessary re-renders, I think.

The emotion docs have an example of this.

import * as React from 'react'
import styled from '@emotion/styled'
import { ThemeProvider, withTheme } from '@emotion/react'

// object-style theme

const theme = {
  backgroundColor: 'green',
  color: 'red'
}

// function-style theme; note that if multiple <ThemeProvider> are used,
// the parent theme will be passed as a function argument

const adjustedTheme = ancestorTheme => ({ ...ancestorTheme, color: 'blue' })

class Container extends React.Component {
  render() {
    return (
      <ThemeProvider theme={theme}>
        <ThemeProvider theme={adjustedTheme}>
          <Text>Boom shaka laka!</Text>
        </ThemeProvider>
      </ThemeProvider>
    )
  }
}

In our use case, I think things get a bit more complicated because the values in the inner component themes depend on the design token theme. For example:

const adjustedTheme = ancestorTheme => ({ ...ancestorTheme, color: 'blue' });

would become

const adjustedTheme = tokens => ({ ...tokens, color: tokens.colors.b500 });

On the implementation side, this would then lead you to every component exporting a createTheme function to produce the component's theme object based on the design tokens.

export const createButtonTheme = tokens => ({
  primary: {
    backgroundColor: tokens.colors.b500,
  },
  secondary: {
    backgroundColor: tokens.colors.n300,
  },
})

These component theme functions can be combined in a global theme function in Circuit UI

import { createTheme as createButtonTheme } from 'components/Button';
import { createTheme as createTextTheme } from 'components/Text';

export const createCircuitTheme = tokens => ({
  ...tokens,
  button: createButtonTheme(tokens),
  text: createTextTheme(tokens),
})

When applying this to the Emotion example you would get the following in an application.

import styled from '@emotion/styled'
import { ThemeProvider } from '@emotion/react'

import { light } from '@sumup/design-tokens'
import { createTheme } from '@sumup/circuit-ui'

export const App = ({ children }) => (
      <ThemeProvider theme={light}>
        <ThemeProvider theme={createTheme}>
          {children}
        </ThemeProvider>
      </ThemeProvider>
    )
  }
}

Yes this is a lot of JS at runtime. Potentially developing a way to create complete application themes in userland at build time would be preferable.

In an alternative API, the component themes could use strings for the values of CSS properties. For example, the Button theme could be defined statically like so:

const buttonTheme = {
  primary: {
    backgroundColor: 'b500',
  },
  secondary: {
    backgroundColor: 'n300',
  },
}

You can then adjust usage in the component styles accordingly:

const primaryStyles = ({ theme, variant }) => variant === 'primary' && css`
  background-color: ${theme.colors[theme.button.primary.backgroundColor]};
`

This would still be type safe, but I think it's butt ugly.

felixjung avatar Dec 07 '20 11:12 felixjung

I think this is still missing something. You need a way to tell the createComponentTheme functions which theme to produce, i.e. light vs. dark. Maybe in the theme switcher logic?

import styled from '@emotion/styled'
import { ThemeProvider } from '@emotion/react'

import { tokens } from '@sumup/design-tokens'
import { createTheme } from '@sumup/circuit-ui'

export const App = ({ children }) => (
      <ThemeProvider theme={tokens}>
        <ThemeProvider theme={createTheme('light')}>
          {children}
        </ThemeProvider>
      </ThemeProvider>
    )
  }
}

createTheme then returns a function that takes the tokens as input and ensures the correct values are used in the component theme.

felixjung avatar Dec 07 '20 12:12 felixjung

Hey 👋

This definitely sounds like a nice improvement to me. 🙂 One of the aspects that makes more sense for me is this:

The specific values allow for greater customization and themeing of the components. Want to keep blue as the primary color, but turn all primary buttons orange? That's possible now!

Decoupling of the Theme colours and the components/elements colours sounds great. 💯 👍 As mentioned it will be a considerable breaking change.

@connor-baer maybe we can have some nice scripts that could help with the breaking changes? 🙂

hleote avatar Feb 19 '21 15:02 hleote

Maybe we can have some nice scripts that could help with the breaking changes? 🙂

We'll investigate how we can reduce the work for the migration, but I'm afraid that fully automating it is not possible. I expect that each generic theme color will be mapped to multiple semantic colors and it will require a human with context to make the replacements.

connor-baer avatar Feb 21 '21 14:02 connor-baer

@mcntsh made a great point offline: Using CSS variables instead of static strings could be a major boost for performance as it would shift the responsibility for updating the component styles when switching between themes to the browser.

In theory, we could move away from a JSON theme entirely (except for media query values) and use CSS variables directly. We would lose out on type checking and autocomplete though.

Since some legacy browsers don't support CSS variables, this is blocked by https://github.com/sumup-oss/circuit-ui/issues/790.

connor-baer avatar Feb 24 '21 07:02 connor-baer

An update here: we're moving forward with the semantic abstraction for color tokens, albeit with a slightly different shape proposal.

Semantic theme shape

We're planning on adding new, semantic color values to @sumup/design-tokens that should eventually entirely replace our hardcoded color values. The proposed shape looks roughly like this:

[element][role][state]

element: background, foreground, border
role: default, subtle, accent, notify, accentSubtle
state: idle, hover, active

There's more context and discussion in this (internal) RFC.

The main difference with what has been discussed in this thread so far is that semantic tokens are not component-aware, they're "global" semantic colors. This makes the theme's implementation simpler, which also means that it makes creating a custom theme easier. Theming a single component is still possible by wrapping it in Emotion's ThemeProvider.

POC

A POC branch was started at #1447. It contains a subset of the necessary changes inside Circuit UI, as well as a live example in Storybook.

Migration strategy

  • in an upcoming minor: add the semantic color tokens to the theme, and deprecate hardcoded color tokens. From that point on, teams should start migrating to semantic color tokens
  • in a future major (not the upcoming v5 to give more time for migrating): stop exporting hardcoded color tokens. When an app has been fully migrated to use semantic color tokens, dark mode (#788) will technically be possible

robinmetral avatar Apr 05 '22 15:04 robinmetral

Completed in #1880.

connor-baer avatar Feb 12 '23 01:02 connor-baer