Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New Palette Proposal #7766

Closed
wants to merge 3 commits into from
Closed

Conversation

JamieB-gu
Copy link
Contributor

@JamieB-gu JamieB-gu commented May 22, 2023

Why?

The DCAR project means bringing support for apps articles to DCR. As part of this we need to come up with a mechanism for handling dark mode. This draft PR is a proposal for one possible mechanism, and some examples of how it might be used. It includes:

  • A new example palette module; an iteration on the current decidePalette module.
  • An example of how a component might use this module, in HeadlineExample.
  • Some example stories for this component, with dark mode support.

It builds on the: ideas in #4241 and #5454; some of the existing code in apps-rendering; discussions with, and prior work undertaken by, @georgeblahblah and @Georges-GNM .

Most types and values in this PR have JSDoc comments, with some examples, explaining what they do.

Note: The stories are written using Component Story Format 3, which is the default for Storybook 71.

Architecture Overview

The idea is based on CSS custom properties (also known as CSS variables)2, which allow you to define a set of custom properties3 that can be accessed in other CSS declarations via the var function4. In this case, we can set up our palette colours as CSS custom properties (automated in practice; this shows an example output):

const lightPalette = css`
  --headline-colour: #1a1a1a;
  --headline-background-colour: #ffffff;
`;

const darkPalette = css`
  --headline-colour: #f6f6f6;
  --headline-background-colour: #121212;
`;

and access them throughout our components to set color, background-color and so on:

const styles = css`
  color: palette('--headline-colour');
  background-color: palette('--headline-background-colour');
`;

const Headline = () => <h1 css={styles}>A short headline</h1>;

We then swap the values of these custom properties between their light and dark variants, based on user preference. Here is one approach:

const styles = css`
  :root {
    ${lightPalette}
  }

  @media (prefers-color-scheme: dark) {
    :root {
      ${darkPalette}
    }
  }
`;

Features

Dark Mode Support

The primary reason for introducing this approach is dark mode support. However, we also have to account for the fact that the apps currently support dark mode, but dotcom does not. Therefore we need a way to use the same components for dotcom and apps but only display them in dark mode on the apps. We also need to make sure we don't ship unused dark mode CSS to dotcom for performance reasons. On the other hand, we still want to allow for the possibility that we may add dark mode to dotcom at some point in the future, and in a way that doesn't impact performance by downloading unused dark mode colours in light mode and vice versa.

The idea here is to provide parallel palettes containing identical colour names: one for light mode and one for dark mode. For the apps we can include both and swap between them in pure CSS using the prefers-color-scheme media query5:

const styles = css`
  :root {
    ${lightPalette}
  }

  @media (prefers-color-scheme: dark) {
    :root {
      ${darkPalette}
    }
  }
`

For dotcom we just include the light palette, so there's no dark mode and no dark mode colours get shipped to users. If in future we do want to add the dark mode colours to dotcom, we may opt to use <link> tags with prefers-color-scheme set in the media attribute6, which allows us to conditionally load one or other of the palettes based on user preference:

<link
  rel="stylesheet"
  href="light.css"
  media="(prefers-color-scheme: light)"
/>

<link
  rel="stylesheet"
  href="dark.css"
  media="(prefers-color-scheme: dark)"
/>

These palettes could also be loaded and applied dynamically in client-side JavaScript if required.

Less CSS

Compared to the current dark mode solution used in apps-rendering (AR), this option results in less CSS being written and shipped to users. At the moment, we add dark mode styles to a component in AR like this:

const styles = (format: ArticleFormat): SerializedStyles => css`
  color: ${text.headline(format)};
  background-color: ${background.headline(format)};

  ${darkModeCss`
    color: ${text.headlineDark(format)};
    background-color: ${background.headlineDark(format)};
  `}
`;

The proposal in this PR would mean avoiding having to write this extra dark mode block for many components, as you only need to set colour properties to the CSS variable once and have them swap between light and dark mode automatically:

const styles = css`
  color: ${palette('--headline-colour')};
  background-color: ${palette('--headline-background-colour')};
`;

The reason it also means less CSS shipped is that each of those darkModeCss blocks generates an additional prefers-color-scheme media query block, which contains the dark mode declarations to override the light mode ones. Those blocks would not be needed with the proposed approach, as you can set the entire dark mode palette with a single media query (demonstrated in the "Dark Mode Support" section above).

Text Search For Colour Names

This proposal suggests using the CSS variable name in the source code in two places:

  1. In the palette object as keys for the palette decision functions.
  2. In a component's CSS declarations to look up the palette colour.

This should make it easy to connect what you can see within a browser with the corresponding code that's applying the CSS rules. The CSS variable names will be visible in the inspector, and can be copied and searched for in the codebase to find both the component where they're applied and the palette function used to derive the colour. This should be an improvement on the current situation.

Dark Mode Stories

The current apps-rendering (AR) solution for dark mode does not provide an easy way to write dark mode stories. There's no way to control the prefers-color-scheme browser preference programmatically in JavaScript, so it's difficult to force components into their dark mode variants for snapshotting in storybook and chromatic.

As seen in the example stories in this proposal, this becomes much easier with custom properties. For any given story you can wrap the component in an element that has those properties applied, and have them propagate down to apply light or dark mode:

const story = (
  <div css={darkModePalette}>
    <Component />
  </div>
);

Naming Conventions

The naming conventions used in this proposal differ from the current pattern. They aren't strictly tied to the technical implementation, and therefore we could choose to adopt this new architecture without also switching to these new conventions.

The approach taken in this proposal is for colour names to match the CSS properties they're intended to be used with. For example, --headline-colour for the color property, --headline-background-colour for the background-color property, --headline-border-colour for the border-color property and so on. The intention is to make it very clear what each colour is for. Also, by putting the component name first, it may be easier to scan through the list of colours more quickly (there are far fewer that start with the word "headline" in this new pattern, for example, than would start with the word "text" in the existing pattern). Thanks to @georgeblahblah for a suggestion on this.

CSS-only

Finally, and briefly, this is a CSS-only solution, so it doesn't require client-side JavaScript to be available and initialised to swap between light and dark mode colour schemes.

Footnotes

  1. https://storybook.js.org/blog/storybook-7-0/#csf3-with-improved-type-safety

  2. https://developer.mozilla.org/en-US/docs/Web/CSS/Using_CSS_custom_properties

  3. https://developer.mozilla.org/en-US/docs/Web/CSS/--*

  4. https://developer.mozilla.org/en-US/docs/Web/CSS/var

  5. https://developer.mozilla.org/en-US/docs/Web/CSS/@media/prefers-color-scheme

  6. https://developer.mozilla.org/en-US/docs/Web/HTML/Element/link#media

- Uses CSS vars to provide colours
- Adds support for dark mode
- Adds an example component and story
@OllysCoding
Copy link
Contributor

OllysCoding commented May 30, 2023

I will admit I'm a big fan of this solution - I like that it keeps the best of both worlds, (hopefully) ease of development through a clear palette interface & easy-to-understand approach to picking colours like we have now, combined with a clean use of CSS variables & browser features.

There is one question I can't figure out the answer to with this approach though: Multiple palettes on one page.

Palette varies by format - any format combination you put in would produce a different set of colours. The proposed solution works great in the case where we just have one format per-page, but this is never case in DCR. On articles we have different formats & therefor palettes for rich links & onwards contents, and on fronts we have different palettes both for every individual card but also overrides at the container level.

A solution with CSS variables would need to factor in that we can't have a single variable for each palette entry, and instead it would need multiple. This opens some questions - could we have multiple CSS variables per palette? Perhaps they could pre prefixed with a hash of format & container tags? If so when generating the css to declare the variables how do we avoid needing to generate every possible combination that might be needed?

One potential solution or simplification could be to break out the 'card palette' and 'article palette' into their own systems (it'd definitely be fair to say we're overloading palette by having both in one at the moment anyway) , the article palette could be used as shown in this PR, but the card palette (which would still need to support dark mode for onwards content on articles) would need a more complex solution - this might be the best compromise so if we went this way it'd be nice to outline how we could support darkmode this way.

@github-actions
Copy link

"This PR is stale because it has been open 30 days with no activity. Unless a comment is added or the “stale” label removed, this will be closed in 3 days"

@github-actions github-actions bot added the Stale label Jun 30, 2023
@JamieB-gu JamieB-gu removed the Stale label Jun 30, 2023
@github-actions
Copy link

"This PR is stale because it has been open 30 days with no activity. Unless a comment is added or the “stale” label removed, this will be closed in 3 days"

@github-actions github-actions bot added the Stale label Jul 31, 2023
@github-actions
Copy link

github-actions bot commented Aug 4, 2023

This PR was closed because it has been stalled for 3 days with no activity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants