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

Add support for multiple color modes #18

Merged
merged 316 commits into from
Mar 10, 2021
Merged

Add support for multiple color modes #18

merged 316 commits into from
Mar 10, 2021

Conversation

BinaryMuse
Copy link
Contributor

@BinaryMuse BinaryMuse commented Jul 28, 2020

This PR introduces color modes to Primer Primitives. Modes are defined in SCSS files in data/modes/, and at build-time these are converted into SCSS files with CSS variables for use in Primer CSS, and JSON files (for now) for use in Primer Components and other non-CSS based systems.

The SCSS files in data/modes/ that define color modes must adhere to the following rules:

  • There must be a top-level $scale map that defines the colors of the scale
  • There must be a top-level $functional map that defines the functional colors
  • Every color mode must implement a scale and functional maps with the exact same keys and number of values

Otherwise, authors of the color mode SCSS files may use any imports, functions, mixins, and the like, just as in any other SCSS file.

[Edit] See my comment below for updated information.

This is a MAJOR breaking change and should NOT be merged until otherwise specified.

TODO

  • Generate scale data based on mode definition
  • Generate functional data based on mode definition
  • Ensure all built modes are compatible
  • Generate TS files and integrate with package compilation
  • Fill out full list of functional colors based on current work
  • Look into making this generic enough for other types of modes (e.g. spacing, typography, etc.)
  • Change color scale variables to single digit: $gray-500 -> $gray-5
  • Consider allowing mix-blend-mode: multiply; to be changed to a different value in dark mode.
  • Rename icon-folder to something that doesn't start with icon-. The icon- prefix is reserved for only global variables.
  • By adding "overlay" global colors we can remove some of the component colors e.g. autocomplete-bg

  1. Add new colors -> Add support for multiple color modes #18 (this PR)
  2. Use new colors -> 🎨 Color Modes css#1131
  3. Toggle new colors -> https://github.com/github/github/pull/155266

@simurai
Copy link
Contributor

simurai commented Jul 30, 2020

Awesome.. 🎉

Will be interesting how many functional variables we end up needing.

Another thought is: Can we get away with keeping the scales “private”. Like we would use them for the functional variables, but don’t add it to a theme. So that people don’t just start to use the color scale directly and it becomes more unpredictable to change the values. This might have the effect of needing lots more functional variables. Like --bg-btn or so.

@BinaryMuse
Copy link
Contributor Author

BinaryMuse commented Jul 30, 2020

Another thought is: Can we get away with keeping the scales “private”. Like we would use them for the functional variables, but don’t add it to a theme. So that people don’t just start to use the color scale directly and it becomes more unpredictable to change the values.

@simurai Yeah, I think you're right here, allowing direct access to the scale values is probably a road to a maintainability nightmare.

Do you think we'd ever need access to the scale for one-off things? For example, if a color doesn't work quite right for a certain feature we could override it for a specific mode, e.g.

.my-thing {
  color: var(--some-functional-color);

  @include mode("dark") {
    // fix up the shade for this mode
    color: var(--some-scale-color);
  }
}

We could also update stylelint to catch incorrect usage as well.

@simurai
Copy link
Contributor

simurai commented Jul 31, 2020

Do you think we'd ever need access to the scale for one-off things?

Hopefully that doesn't happen too often, but yeah, probably can't be avoided entirely. There is always some edge cases to be found. 😆

For dotcom, we could also keep these overrides in a separate folder, similar what we currently do with /hacks. Might be easier to keep an eye on them. And if there is a pattern used often, it can be turned into a new functional variable.

@BinaryMuse
Copy link
Contributor Author

Alright, as of the latest commits, things have changed somewhat.

Flexible mode types

Primer Primitives now supports mode definitions for any kind of mode data, not just color modes. For example, we could have condensed, normal, and relaxed spacing modes by placing three mode definitions in data/spacing/.

Mode compilation

Mode definitions are compiled to SCSS (with CSS vars), JSON, TypeScript, and JavaScript (via TypeScript compilation).

Mode definitions

Modes are still defined in individual SCSS files inside data/{mode_type}/{mode_name}.scss, but now the only thing exported from the definition files are the contents of the top-level variable $export. $export should be a map, and can contain sub-maps, lists of plain values, or plain values.

For example, here is the beginning of the light color mode definition (note that $scale is a map that contains color values and lists of color values):

$export: (
  scale: $scale,

  // Pure functional

  text: (
    primary: $gray-900,
    secondary: $gray-600,
    tertiary: $gray-500,
    placeholder: $gray-300,
    link-primary: $blue-500,
    link-secondary: $gray-900,
    link-accent: $blue-500,
    warning: $tbd,
    danger: $tbd,
    success: $tbd,
  ),

  ic: (
    primary: $gray-500,
    secondary: $gray-400,
    tertiary: $orange-600,
    // ...

Definition formatting

When building the primitives, maps and variables are translated differently in CSS and JSON/JS. When at all possible, use maps to group related colors together. For example, given the following mode definition:

$export: (
  // this is a map
  text: (
    primary: $black,
  )

  // this is a plain value
  text-secondary: $gray-900,
)

The CSS output for each would look the same:

--text-primary: #...;
--text-secondary: #...;

but the JS and JSON output would be different:

{
  text: {
    primary: "..."
  },
  "textSecondary": "#..."
}

The former style, with the nested map, is favored over the latter style, with the entire variable spelled out, so that JS and JSON consumers can consume values using a syntax like "text.primary".

Lists

Currently in Primer CSS, we have colors defined by 100s, like this:

$blue-000: ...;
$blue-100: ...;
$blue-200: ...;

In order to support multiple environments, these are now built as lists, and the compiled CSS will use the index of the list as part of the variable name. For example:

$blue: (#..., #..., #..., #..., #...,);

This will get compiled in CSS to:

--scale-blue-0: #...;
--scale-blue-1: #...;
--scale-blue-2: #...;

and to a plain array of strings in JSON/JS.

@BinaryMuse BinaryMuse marked this pull request as ready for review August 6, 2020 18:25
@simurai
Copy link
Contributor

simurai commented Aug 7, 2020

Definition formatting

That's super nice not having to write out the entire name. Also should help with readability having these groups.

Currently in Primer CSS, we have colors defined by 100s, like this:

I assume the reason was to allow adding "in-betweens" without having to change the rest of the names.. like adding a 150.

$blue-000: ...;
$blue-100: ...;
$blue-150: ...;
$blue-200: ...;

But we haven't done that so far, so maybe fine to just use the indexes. If we really wanna add more later, some --scale-blue-2 -> --scale-blue-3 find+replace should be ok to make the changes.

@BinaryMuse
Copy link
Contributor Author

BinaryMuse commented Aug 10, 2020

I assume the reason was to allow adding "in-betweens" without having to change the rest of the names

If we really wanna add more later, some --scale-blue-2 -> --scale-blue-3 find+replace should be ok to make the changes.

Yes, I think you're right all around, although the old format doesn't work well for the JS version. Since we shouldn't be using the scale values directly very much (at all???) I'm hoping it won't be a huge issue.

@vdepizzol
Copy link
Contributor

For example, here is the beginning of the light color mode definition (note that $scale is a map that contains color values and lists of color values):

@BinaryMuse I wonder if something more specific like $color-scale would make it clearer. I'm starting to see where the definition of UI scales fits in Primer, and I've been wondering if $ui-scale is the right variable name to keep the normal and large/relaxed definitions. Thank you!

@BinaryMuse
Copy link
Contributor Author

For example, here is the beginning of the light color mode definition (note that $scale is a map that contains color values and lists of color values):

@BinaryMuse I wonder if something more specific like $color-scale would make it clearer. I'm starting to see where the definition of UI scales fits in Primer, and I've been wondering if $ui-scale is the right variable name to keep the normal and large/relaxed definitions. Thank you!

Yeah, that definitely makes sense — although I think it's likely we will not be exposing the scale directly (though we may change our minds 🙈)

@BinaryMuse
Copy link
Contributor Author

@simurai I made a few changes to the build script:

  1. If any variables are missing in one file or another, the build script will error and also tell you which ones.

    $ yarn build
    yarn run v1.21.1
    $ rm -rf dist
    $ ts-node ./script/build.ts && tsc
    Invalid modes for type 'colors'. The following variables are missing in one or more modes:
    Variable alert-warn-bg is missing in modes: dark
    Variable alert-warn-border is missing in modes: dark
    Variable alert-warn-icon is missing in modes: dark
    Variable bg-tertiary is missing in modes: light
    Variable blankslate-icon is missing in modes: light
    Variable border-inverse is missing in modes: light
    Variable btn-primary-box-shadow is missing in modes: dark
    error Command failed with exit code 1.
    
  2. If you want the build script to skip a particular mode, you can assign it to the PRIMER_SKIP environment variable, or specify more than one by using commas, e.g. PRIMER_SKIP=colors/light,colors/dark.

    $ PRIMER_SKIP=colors/dark yarn build
    yarn run v1.21.1
    $ rm -rf dist
    $ ts-node ./script/build.ts && tsc
    Built mode data
    ✨  Done in 3.73s.
    
  3. Each exported CSS variable is now prefixed with the name of the mode type, e.g. --color-bg-canvas. You can change the prefix by editing data/{type}/prefix, which is how we have --color- instead of --colors-.

@simurai
Copy link
Contributor

simurai commented Sep 11, 2020

Added the dark colors 36490c8 but reversed the order of the scale:

image

Initially just as a shortcut because it allowed me to skip making changes to the rest of dark.scss. But now I'm wondering if we should keep it like this?

Option A

$gray-900: #24292e; // looks dark in light.scss
$gray-900: #F0F6FC; // looks light in dark.scss

text-primary: $gray-900, // in light.scss
text-primary: $gray-900, // in dark.scss

color: var(--color-text-primary); // in Primer CSS/dotcom

It would also allow us to use the scales like

$gray-200: #e1e4e8; // looks light in light.scss
$gray-200: #21262D; // looks dark in dark.scss

scale-grey-2: $gray-200, // in light.scss
scale-grey-2: $gray-200, // in dark.scss

background-color: var(--color-scale-grey-2); // in Primer CSS/dotcom

So you can think of it like this:

  • Higher numbers (~900) are typically used for foregrounds
  • Lower numbers (000~) are typically used for backgrounds

Option B

Without flipping the order of the scale for dark, all the --color-scale would become kinda useless and we would have to create countless custom variables like

$gray-200: #e1e4e8; // looks light in light.scss
$gray-700: #21262D; // looks dark in dark.scss

my-component-bg: $gray-200, // in light.scss
my-component-bg: $gray-700, // in dark.scss

background-color: var(--color-my-component-bg); // in Primer CSS/dotcom

Eventually we still wanna make tweaks here and there by adding custom variables, but using option A we already have a high chance of making it "usable".

Risks

  • Option A: People would use --color-scale a lot since it "somewhat" works and wouldn't create custom variables to improve each mode separately.
  • Option: B: Takes a lot longer and the amount of custom variables would balloon. There is also the risk of people mis-using existing functional variables instead of creating a custom variable. Like when needing $gray-300 use the existing --color-text-placeholder for something that isn't really placeholder text.

@colebemis shared this tweet:

slack-imgs

With option A we could quickly get to the 2nd version and still be able to tweak it to make it look like the 3rd version.
With option B we would have to slowly convert to the 3rd version.

/cc @primer/design-systems

@simurai
Copy link
Contributor

simurai commented Sep 15, 2020

@BinaryMuse There is something interesting when mixing colors with other units, like this case for box-shadow:

shadow: (
  small: 0 1px 0 rgba($black, 0.04),
)

it compiles to: 👇

--color-shadow-small-0: 0;
--color-shadow-small-1: 1px;
--color-shadow-small-2: 0;
--color-shadow-small-3: rgba(27,31,35,0.04);

Not a lot of combinations work but something that seems to work is:

shadow: (
  shadow-small: 0 1px 0 rgba($black, 0.04),
),

👇

--color-shadow-shadow-small: 0 1px 0 rgba(27,31,35,0.04);

Or input-shadow works too. Can't really figure out the pattern.

@BinaryMuse
Copy link
Contributor Author

@BinaryMuse There is something interesting when mixing colors with other units, like this case for box-shadow

Ah, sorry about that. This is tricky to do in SCSS because we represent arrays of values as SCSS lists, and a property like 0 1px 0 rgba($black, 0.04) is also represented as a list. So we need to figure out if it's an array or a single property. Right now, the code is hacked to look for the term "shadow" in the key, since that's where this normally comes up, but it mistakenly didn't check the entire key, just the most recent sub-key. That's fixed now.

@simurai
Copy link
Contributor

simurai commented Sep 17, 2020

Another open question: Currently we replace variables and utility classes that have a reference to "dark/light" with functional names: $text-gray-dark -> --color-text-primary. But what should we do with the other colors like .text-purple, .bg-orange etc.

image

It's hard to come up with a good functional name. And adding custom variables and classes like .registry-user-name or --color-communitystats-summary-circle-bg feels too specific.

Should we keep the current color utility classes and just migrate SASS variables like $text-purple to --color-text-purple? Even for red and green, it might not always make sense to use names like "danger/success".

The mini guide for "using/adding colors" would then look like this:

  1. Use existing functional variables if possible -> --color-text-success
  2. Add a new functional variable if it gets used by multiple components/views -> --color-text-done
  3. Add a new component variable if it gets used in just a single component -> --color-beta-label-text
  4. Add a new custom app variable if it gets used in multiple places (partials) -> --color-commit-verified-text
  5. Use color classes for one-offs -> .color-text-green
  6. Use color scale variables/classes for one-offs -> .color-scale-green-3 or --color-scale-green-3. This only works if we do option A.

There is a risk that we skip 2-4 and only do 5 or 6. But that might still be better than "mis-using" functional variables and using --color-text-success for everything that needs to look green. And if we do that, it becomes hard to change later and we would have to go hunt for all the use cases and decide if it really is "success" or something else.

@simurai
Copy link
Contributor

simurai commented Sep 29, 2020

Having done the primer/primitives -> primer/css -> dotcom dance a few times, I'm not so sure anymore about keeping all the component and app variables in primer/primitives. 🤔 It's pretty time consuming making small updates across multiple repos. Here an alternative:

  • Color scale and functional variables -> Keep in primer/primitives.
  • Component variables -> Keep in primer/primitives. But when adding a new component or refactoring an existing one, it's ok to define the variables first in primer/css and only move to primer/primitives once things are "stable" (and less likely need a lot of updates).
  • App variables -> Move to dotcom. I just can't see other Hubbers willing to add new variables to primer/primitives when they get asked in a code review. We would probably end up adding everything to /hacks, so maybe better to have an official place for these variables.

In addition to a few shared app variables, we could also consider having "single use" variables that can be used with a mixin:

// SCSS

.my-class {
  @include color-mode(color, gray-2, gray-7); // property, light, dark
}

// output CSS

[data-color-mode="light"] .my-class {
  --color: var(--color-scale-gray-2);
}

[data-color-mode="dark"] .my-class {
  --color: var(--color-scale-gray-7);
}

.my-class {
  color: var(--color);
}

Not sure if that's possible? But when authoring new styles, you don't have to think about a variable name that probably is only used once and check if it that name is still available. It would get auto generated based on class name and property and just toggles the value depending on the color mode. It also avoids having lots of merge conflicts in a big light.scss and dark.scss file because things typically get added at the bottom. Anyways, not urgent, but maybe something to keep in mind.

@BinaryMuse
Copy link
Contributor Author

BinaryMuse commented Sep 30, 2020

Having done the primer/primitives -> primer/css -> dotcom dance a few times, I'm not so sure anymore about keeping all the component and app variables in primer/primitives.

I was afraid of this, but it's good feedback. I think the alternative you specified is probably a good middle ground.

Another option that you and I have discussed that I'd like to bring to the larger group is the idea of removing Primer Primitives and including the infrastructure introduced by this PR in Primer CSS itself. This would mean one less hoop to jump through to add or change color variables, making work in Primer CSS (where we generally first add and iterate on new components) much easier. However, projects like Primer React would need to depend on Primer CSS as a source of truth instead of Primer Primitives. Functionally they would be equivalent, and bundle sizes wouldn't be different, so it'd be more of an organization thing.

Another option would be some sort of monorepo for Primer CSS/Primer Primitives, but we've experimented with monorepos before and decided to move away from them.

/cc @primer/ds-core for 💭s

@simurai
Copy link
Contributor

simurai commented Oct 6, 2020

Added an "auto scale" e66591e. This allows using the existing scale variables (e.g. --color-scale-gray-3) without having to think about the inverted order. Only the new auto scale (e.g. --color-auto-gray-3) flips the order and we can remove it later when not needed anymore.

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

Successfully merging this pull request may close these issues.

7 participants