RFC: Theming in Source React Component props

[oliverabrahams]

We are updating the implementation of themes in Source React components from a request for a new dark mode for DCAR. This involves a few changes to the recommended way themes are handled in our Source React components:

• Introduction of a theme Prop: We are introducing a new theme prop in our Source React components. This will allow consumers to apply a custom theme to any component. This approach is necessary because the ThemeProvider in Emotion is not compatible with Islands in their current form.

• Support and Deprecation Plan for ThemeProvider: While we will continue to support the ThemeProvider, we plan to phase out the old themes. Instead, we will adopt new theme objects that align more closely with the Source components in figma.

• Reducing Dependency on Emotion: These changes will also help in gradually moving our Source React components away from Emotion.

• Discouraging CssOverrides: We want to move away from using CssOverrides as the recommended method for altering these components.

Previously, setting a theme for components like ChoiceCard was done using the ThemeProvider:

<ThemeProvider theme={myTheme}>
  <ChoiceCard>
    Select
  </ChoiceCard>
</ThemeProvider>

Updated way of providing a theme. Any variables you supply in the object are optional. You can override one or multiple of the defaults

const myTheme: ChoiceCardTheme = {backgroundSelected: palette.brand[500]}
<ChoiceCard theme={myTheme}>
  Select
</ChoiceCard>

Here is the PR with the proposal for the choice card component. https://github.com/guardian/csnx/pull/1070/files.

Here is the storybook https://635a6fffacd30d393597c1ff-qtrtddfvkq.chromatic.com/?path=/docs/choicecard--docs.

[mxdvl]

This would be a benefical API for dotcom-rendering 👏

Looking at the associated PR, it seems like consumers are able to pass a partial theme, which I think is more dangerous than beneficial. When consumers opt for a provided theme, it should be complete. When consumers opt for for a non-standard theme, all colours must be controlled by the consumer so there are no suprises in the final colour combination.

This is the approach we’ve taken for the Tabs and ExpandingWrapper kitchen components.

Here’s an example which may illustrate my point:

const defaultTheme = {
  background: 'black',
  text: 'white',
};

type Props = {
  theme?: typeof defaultTheme;
  children: React.ReactNode;
}

const Button = ({ theme = defaultTheme }: Props) =>
  <button style={{ background: theme.background, color: theme.text }}>
    {children}
  </button>

// if you allow partial theme, you get too low a contrast, here 2.1
<Button theme={{ text: 'darkred' }}>Click me if you can</Button>
// if you force consumers to provide a full theme, they are responsible for an accessible ratio, here 6.5
<Button theme={{ text: 'darkred', background: 'pink',  }}>Click me</Button>

[mxdvl]

requiring a full theme will prompt engineers to go back to their designers to get the full thing – @joecowton1

Yes, exactly that, the types will make it clearer that consumers have opted for a custom theme.

You could also spread the properties from an existing theme and override one – @jamesmockett

Yes, I think that is a much more explicit approach, as the code then reflects the design decision.

[joecowton1]

@mxdvl ok thanks for clarifying, i'm not entirely convinced it will work in that way but happy to give it a go

[sndrs]

if you allow partial theme, you get too low a contrast, here 2.1

As @jamesmockett notes, requiring consumers to do this:

<Button theme={{ ...defaultTheme , text: 'darkred' }}>Click me if you can</Button>

doesn't stop anyone creating an inaccessible button.

Is it making it less likely? Nothing stops you using defaultTheme with zero knowledge of it, so if it is, i think it can only be because it makes using a custom theme more complex.

That will apply to legitimate uses too. There's no DX difference between:

const inaccessibleButton = () => 
	<Button theme={{ ...defaultTheme , text: 'darkred' }}>Click me if you can</Button>

and

const accessibleButton = () => 
	<Button theme={{ ...defaultTheme , text: 'hotpink' }}>Click me</Button>

I'm not clear how disallowing theme={{ text: 'darkred' }} but allowing theme={{ ...defaultTheme , text: 'darkred' }} guides people away from accidentally creating poor-contrast components?

[sndrs]

would we not expect it to be the designers who should be considering contrast when providing colour overrides?

👍

If we want to prevent engineers shipping poor-contrast interfaces, regardless of the design, we should probably add some contrast checking to the components themselves.

I'm not sure that would be a good idea though. It would mean shipping a component framework that cannot render designs, which is surely the opposite of what we want?

[mxdvl]

Is it making it less likely? Nothing stops you using defaultTheme with zero knowledge of it, so if it is, i think it can only be because it makes using a custom theme more complex.
[…]
I'm not clear how disallowing theme={{ text: 'darkred' }} but allowing theme={{ ...defaultTheme , text: 'darkred' }} guides people away from accidentally creating poor-contrast components?
– @sndrs

The second one is as explicit as possible in JS terms that you must consider all relevant properties of the theme object. My understanding of using custom themes is that they should be possible but somewhat harder/more complex as a reminder that you’re opting out of the existing predetermined themes:

1. easiest – no theme prop passed, sensible light mode default
2. medium – pass full theme object from Source
3. hardest – pass a completely custom theme object, if you decide to partially spread an existing theme to create it, that’s up to you

Does that make resonate at all? It’s not a major issue in any case as allowing a partial object still allows consumers to do the spreading themselves – which we’d continue doing in dotcom-rendering.

[oliverabrahams]

We understand the rationale for making it more challenging for consumers to apply only a partial theme to components. However, we believe the responsibility for ensuring that the provided colours are accessible and complete should rest with the designer. As such, we prefer not to obligate consumers to spread the default theme when only a different background colour is needed. Our primary concern is that increasing the difficulty for consumers to customise components might lead them to resort to less advisable methods, such as using cssOverrides.