Refactor theming functions

[ob6160]

Setting the scene 🖼

Across Source, we use some nifty individual component-level theming functions.

They accept the theme object passed to us from emotion. They then destructure that object into the single component level theme object that it needs. If we pass in undefined, a default theming object is returned.

Here's an example of how that works right now to help illustrate:

// Theming function for the accordion
export const accordion = ({ accordion } = accordionDefault) => css`
  border-bottom: 1px solid ${accordion.borderPrimary};
`;

// Examples

const customTheme = {
  accordion: {
    borderPrimary: '#FF00FF',
  }
}

accordion(customTheme) // This would work, and `borderPrimary` would equal '#FF00FF'
accordion(undefined) // This would work, and `borderPrimary` would equal the value set in `accordionDefault.borderPrimary`

.. and the consumer uses it like this:

<div css={(theme) => [accordion(theme.accordion && theme), cssOverrides]}></div>

As you might have spotted, this implementation comes with the caveat that we must check that theme.accordion is defined before we pass in theme.

This makes a lot of sense! It's saying, "if accordion is defined on our theme object, go ahead and pass theme in".

.. it then follows that: "if accordion is not defined, pass in undefined and fall back on the default accordionDefault value for theme.accordion ".

To conclude this bit: This logic handily avoids circumstances where theme.accordion is not defined on the custom theme. This happens often, because not everybody wants to customise everything that Source offers!

So what needs doing here? 🔧

Simon 👨‍🎨 , our developer, is sitting down to make a shiny new variation of the Accordion component. Simon sets about using the accordion theming function defined above like this:

// note the difference here vs accordion(theme.accordion && theme)
<div css={(theme) => [accordion(theme), cssOverrides]}></div>

As far as Simon is concerned, everything is fine.. it all makes sense! The accordion method should destructure the theme into the accordion theming object and fall back to the default: accordionDefault if it's not found..
...
even the type checking passes!
...
Everything is looking great, this component will be ready to ship in no time ! 🥇 ☕️

Sadly, without the theme.accordion && theme check, poor Simon could easily hit a runtime error.

Let's look at why this is!

The error we just described will happen if accordion has not been defined on the users' custom theme. In this scenario, the accordion method is unable to fall back on its default value, because an object is being passed instead of undefined.

To illustrate, here's an example of a common case that could happen at runtime:

// Theming function for the accordion (same as before)
export const accordion = ({ accordion } = accordionDefault) => css`
  border-bottom: 1px solid ${accordion.borderPrimary};
`;

// A custom theme, but without accordion defined this time!
const unhappyCustomTheme = {
  button: {
    borderPrimary: '#FF00FF',
  }
}

// This would not work despite passing the type checker!
// At runtime, we would try to evaluate: `unhappyCustomTheme.accordion.borderPrimary`.
// This which would error due to `accordion` being undefined.
accordion(unhappyCustomTheme) 

...culminating in this delightful error, ruining Simons' otherwise tranquil Monday morning:

A Proposed Solution 🚙

The solution proposed here reworks our theming functions a little, making a few key changes. Instead of accepting the whole theme object, they have been refactored so that they only accept the theming object that they need to render.

With this approach, accordion only accepts the theme.accordion object OR undefined (in that case falling back to accordionDefault.accordion):

export const accordion = (accordion = accordionDefault.accordion) => css`
  border-bottom: 1px solid ${accordion.borderPrimary};
`;

and Simon is able to call the accordion theming function like this:

<div css={(theme) => [accordion(theme.accordion), cssOverrides]}></div>

This approach leaves no room for ambiguity! theme.accordion will be either defined or undefined ¹

This is great, it means that we can't pass in a theme where theme.accordion could possibly be undefined as we could before without the type checker erroring. 🎉

So, following this approach, the core changes made in this pull request change all of our theming functions to accept only the single theming object that is relevant to them as an optional parameter, which defaults to the default already defined.

Notes

This builds upon #1161

Footnotes

1. This is supported thanks to this change: https://github.com/guardian/source/pull/1195/files#diff-06f78fe3b5cad62762cc5b85c0f25c385f5065842bb33f4b3b44de3066e4b57fR17-R29 which builds upon Add as const type assertion on palette #1161 ↩

[SiAdcock]

Thanks @ob6160, this is a much tidier pattern: it's easier to read, sticks in your head and is simpler to maintain. Awesome work! 🏅

I really enjoyed reading the PR description too, an absolute gem 💎

I have one small comment, but other than that I'd say it's good to go!

[paperboyo]

@ob6160 wins Jimmy’s Award for Best Described and Most Informative PRs 2021!

Congratulations on behalf of The Committee 🥇

[SiAdcock]

Amazing, thanks again @ob6160 💎 💎 💎