primer / primitives

Color, typography, and spacing primitives in json.
https://primer.style/primitives
MIT License
307 stars 43 forks source link

Cleanup / re-structuring of color tokens #362

Closed lukasoppermann closed 1 year ago

lukasoppermann commented 2 years ago

Hey, I want to discuss some opportunities of cleaning up the color tokens in this issue and see what we can remove or move.

Marketing tokens

Relates to https://github.com/github/primer/issues/1366

Do we want to have marketing tokens in primer/primitives or should they live in primer/brand? If we keep them in primer/primitives can we move them into a marketing-light.json file and maybe implement a common naming convention?

Currently we have the following variables

    --color-marketing-icon-primary: #218bff;
    --color-marketing-icon-secondary: #54aeff;
    --color-mktg-btn-bg: #1b1f23;
    --color-mktg-btn-shadow-outline: rgb(0 0 0 / 15%) 0 0 0 1px inset;
    --color-mktg-btn-shadow-focus: rgb(0 0 0 / 15%) 0 0 0 4px;
    --color-mktg-btn-shadow-hover: 0 3px 2px rgba(0, 0, 0, 0.07), 0 7px 5px rgba(0, 0, 0, 0.04), 0 12px 10px rgba(0, 0, 0, 0.03), 0 22px 18px rgba(0, 0, 0, 0.03), 0 42px 33px rgba(0, 0, 0, 0.02), 0 100px 80px rgba(0, 0, 0, 0.02);
    --color-mktg-btn-shadow-hover-muted: rgb(0 0 0 / 70%) 0 0 0 2px inset;

Syntax highlights

@simurai mentioned that we have variables for both prettylights and codemirror. The idea would be to somehow combine this, or remove one set completely, as there are some duplications. Maybe we can have a generic setsyntax-light.json and use the variables in both codemirror and prettylights?

ANSI

We currently provide two sets of ANSI colors (that differ). --color-ansi-[black/red/...] and --color-checks-ansi-[black/red/...]. Can we remove the checks set and use the default ANSI set instead?

Base color scales

As discussed with @vdepizzol and @langermank we are planning to remove the --base-color-scale-red-... variables and not ship any color scale, only functional colors.

Feedback wanted

Please mention:

/ CC: @rezrah @langermank @josepmartins @jonrohan @colebemis @tobiasahlin @simurai @vdepizzol

langermank commented 2 years ago

Do we want to have marketing tokens in primer/primitives or should they live in primer/brand?

primer/brand should cover all marketing needs, so its not necessarily that the existing marketing tokens will move to primer/brand but more that primer/brand will make sure that all of their tokens and CSS covers the needs of brand outside of a React context. This item relates to: https://github.com/github/primer/issues/1366

It might be that we move these existing tokens to dotcom temporarily as a stopgap.

Syntax highlights

I like the sound of consolidating! I'm curious how related to Primer these tokens are, though. We might want to start thinking about moving dotcom specific tokens into dotcom, but I'm not sure off hand how much infrastructure these kinds of tokens need (for example, if they need custom overrides for specific themes than it would need to stay in primer/primitives)

ANSI

Paging @simurai on this one.. I don't have a lot of context here.

Base color scales

I started looking into this in dotcom, and unfortunately many base scale colors are used that don't have a corresponding functional token. I think we'll need to include them in the bundle to start (so keep it as it currently is in the old build.)

simurai commented 2 years ago

Syntax highlights Maybe we can have a generic set syntax-light.json and use the variables in both codemirror and prettylights?

Yes, that sounds like a good plan.

ANSI

The reason why Checks needs their own ansi colors is because the background of the log is dark in light mode.

Screen Shot 2022-10-05 at 20 10 49

So we had to create custom tokens for them. To not have to do that, there are these two options:

  1. Convince Product Design to use a light background in light mode. Now that the header is light too, I think Checks is the only remaining outlier.
  2. Look into supporting something like nested themes.

Base color scales not ship any color scale, only functional colors.

The scale colors are currently still used for some edge cases. And also to create some variables that only live on doctom. To remove the scale colors from Primitives, I guess we would have to:

  1. Create new tokens in Primitives as replacements.
  2. Maybe redesign the UI/feature so we can use the existing functional tokens.
  3. For some colors that don't need to adapt to color modes and we don't want to move to Primitives, we might can also use the static colors instead.
langermank commented 1 year ago

@lukasoppermann should we close this?

lukasoppermann commented 1 year ago

I guess yes, we can keep this in mind anyway