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

Color Module - First Draft #147

Open
wants to merge 32 commits into
base: main
Choose a base branch
from
Open

Color Module - First Draft #147

wants to merge 32 commits into from

Conversation

adekunleoduye
Copy link

@adekunleoduye adekunleoduye commented Jun 21, 2022

Overview

This is the first draft of the Design Tokens Color Module.

@adekunleoduye adekunleoduye added the Do not merge Add this label to a PR to prevent it from accidentally being merged. label Jun 21, 2022
@netlify
Copy link

netlify bot commented Jun 21, 2022

Deploy Preview for dtcg-tr ready!

Name Link
🔨 Latest commit 1195a2f
🔍 Latest deploy log https://app.netlify.com/sites/dtcg-tr/deploys/6723ebd906327f0008c530c9
😎 Deploy Preview https://deploy-preview-147--dtcg-tr.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Comment on lines 25 to 34
{
"Majestic magenta": {
"value": "#ff00ff",
"type": "color"
},
"Simple sage": {
"value": "#abcabc",
"type": "color"
}
}
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update examples

Co-authored-by: Ayesha Mazumdar <[email protected]>
Co-authored-by: Adekunle Oduye <[email protected]>
Co-authored-by: Kathleen McMahon <[email protected]>
</td>
<td>
<ul>
<li>No fully supported (only safari)</li>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<li>No fully supported (only safari)</li>
<li>Not fully supported (only safari)</li>

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a nitpick. It would be nice if the Con sections mentioning the lack of browser support were consistent.

Something like

"Limited Browser Support (Safari)"

Consistent verbiage and browsers with support are listed.

@adekunleoduye adekunleoduye added To be reviewed by editors Issues that need to be reviewed in an upcoming meeting between editors. and removed Do not merge Add this label to a PR to prevent it from accidentally being merged. labels Dec 7, 2022
@adekunleoduye adekunleoduye marked this pull request as ready for review December 7, 2022 17:50
@adekunleoduye adekunleoduye changed the title [WIP] color: Init first draft Color Module - First Draft Dec 7, 2022
technical-reports/color/index.html Outdated Show resolved Hide resolved
technical-reports/color/index.html Show resolved Hide resolved
@@ -0,0 +1,7 @@
## Color module
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we may need to move this section directly into the technical-reports/color/index.html file like we do with the format:

<section id="introduction">
<h1>Introduction</h1>
<p>
<em>This section is non normative</em>
</p>
<p>
Design tokens are a methodology for expressing design decisions in a
platform-agnostic way so that they can be shared across different
disciplines, tools, and technologies. They help establish a common
vocabulary across organisations.
</p>

And we can probably remove the Color module title.


| Pros | Cons |
| ----------------------------------------------------- | ------------------------------------- |
| It is easy to understand and compare to other formats | Not supported in all browsers (IE 11) |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not supported in all browsers (IE 11)

Source?

| Pros | Cons |
| ------------------------------------------ | --------------------------------------------------------------------------- |
| Access to 50% more colors (P3 color space) | Colors more perceptually uniform, so it can be harder to distinguish values |
| | Limited browser support (Safari only) |
Copy link
Member

@kaelig kaelig Apr 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Limited browser support (Safari only)

Now supported in Chrome, Edge, Safari, and Firefox behind a flag.

https://caniuse.com/mdn-css_types_color_oklch

return (
<SubbrandContext.Provider value={subbrand}>
<ThemeContext.Provider value={theme}>
<div className={cx(subbrand, theme)} ref={forwardedRef}>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there another way to write this without the cx utility function?

ayeshakmaz and others added 16 commits May 30, 2023 14:23
Co-authored-by: Kaelig Deloumeau-Prigent <[email protected]>
Co-authored-by: Kaelig Deloumeau-Prigent <[email protected]>
Co-authored-by: Kaelig Deloumeau-Prigent <[email protected]>
Co-authored-by: Kaelig Deloumeau-Prigent <[email protected]>
Co-authored-by: Kaelig Deloumeau-Prigent <[email protected]>
Co-authored-by: Kaelig Deloumeau-Prigent <[email protected]>
Co-authored-by: Kaelig Deloumeau-Prigent <[email protected]>
Co-authored-by: Kaelig Deloumeau-Prigent <[email protected]>
Co-authored-by: Kaelig Deloumeau-Prigent <[email protected]>
Co-authored-by: Kaelig Deloumeau-Prigent <[email protected]>
Update examples to include colorspace object option, and rearrange files
Copy link
Member

@c1rrus c1rrus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry it's taken me so long to review this. Great work putting all of this together!

However, I think there's still work to be done to better align this with the format spec.

</p>

<p>Work in progress.</p>
<section
data-include="./overview.md"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The overview.md is missing in this PR. Is this an old link that needs to be removed, or does this file just need to be added to git?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • (This comment is still valid and needs addressing)

@@ -77,16 +79,29 @@
<h1>Introduction</h1>

<p>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dunno if this is something that's (going to be?) covered in the missing overview.md file (see my other review comment), but I think it would be helpful to provide a bit more context about this document's purpose here.

My reading is that the "Token naming" chapter is providing non-normative guidance and advice for how to name & organise colour tokens. However, the later sections are detailed specs for how colour tokens should be represented in DTCG files - so I presume they are intended to be normative. Furthermore, those specs conflict with the current format draft (e.g. the format doc currently states that the $value of a color token MUST be a string with a hex value. My understanding is that we want the info in this doc to supersede that though)

I therefore suggest we distinguish between the spec stuff and the guidance stuff more clearly. A few ways we might do this:

  • Move the color type and gradient chapters into the format spec, replacing/updating the corresponding chapters there as needed. Then the color report would only contain guidance and the intro could make that clear.
  • OR, remove the existing color and gradient type chapters from the format spec and replace them with links to this doc. Then this doc becomes the definitive spec for those types - and also complements that with some helpful guidance. In that case, I'd argue that this doc's primary purpose would become being the spec for color tokens in the DTCG format. So maybe we ought to then move the token naming chapter to the end - perhaps even as an appendix (bear in mind that format spec is intentionally agnostic about how folks name and organise their tokens, so putting advice about that at the start of a "spec" document might feel a little odd to readers).

Personally, I have a slight preference for the first option, since it keeps all the spec stuff in one place. But, I'd be OK with the other approach too.

Either way, this will also set a nice precedent for the animation module and any others we may have in the future to follow.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • (This comment still needs to be addressed—this info should all go into technical-reports/format/types.md)

technical-reports/color/token-naming.md Show resolved Hide resolved
technical-reports/color/token-naming.md Show resolved Hide resolved
technical-reports/color/token-naming.md Show resolved Hide resolved
technical-reports/color/color-type.md Show resolved Hide resolved
technical-reports/color/color-type.md Show resolved Hide resolved

## Hex - required support

For the color value, the required fallback format to represent color through design tokens is a hex or hex8 value. A hex triplet is a 6-digit, 24 bit, hexadecimal number that represents Red, Green, and Blue values as `#RRGGBB`. An eight-character hex will include the alpha value in the last 2 characters. If no alpha value is specified, it is assumed the color if fully opaque.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To align with the current spec draft and, as far as I know, how hex color values are interpreted in CSS and most design tools, should we say here that the sRGB color space is always implied for these fallback hex values?

Copy link
Contributor

@drwpow drwpow Aug 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. Hex values have started to be used for Display P3 in design tools, but the intent is limited to sRGB.

Copy link

@Shrinks99 Shrinks99 Aug 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
For the color value, the required fallback format to represent color through design tokens is a hex or hex8 value. A hex triplet is a 6-digit, 24 bit, hexadecimal number that represents Red, Green, and Blue values as `#RRGGBB`. An eight-character hex will include the alpha value in the last 2 characters. If no alpha value is specified, it is assumed the color if fully opaque.
For the color value, the required fallback format to represent color through design tokens is a hex or hex8 value. A hex triplet is a 6-character, 24 bit, hexadecimal number that represents Red, Green, and Blue values as `#RRGGBB`. An eight-character hex encoded value will include the alpha value in the last 2 characters. If no alpha value is specified, it is assumed the color if fully opaque. The sRGB colorspace MUST be used when interpreting fallback hex values.

technical-reports/color/color-type.md Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest splitting this out into a separate PR for now.

As it stands, this has got quite a few deviations from how the gradient type is currently defined in the format:

  • The format does not use $ prefixes for the value's inner properties and, as per the comments from @drwpow and myself on the updated color type, I think we should stick with that. (or, if we didn't, we'd need to change all the other composite types to be consistent)
  • The format does not include a style property, though several folks have suggested something along those lines in Gradient type feedback #101. I think that's probably a worthwhile addition, but there are other interesting proprosals in that thread too, which I think we ought to consider holistically.
  • The format does not have an alpha property on the stops and, given that the color values can have an alpha channel, it feels kinda redundant. Besides, what would be the expected behaviour if both have been specified but have conflicting values?

I presume the intent of this section is really just to show that the new, expanded color value syntax defined in the previous chapter can be used anywhere else that color values are permitted.

The value of a gradient stop is one case of that, but there are others too:

  • the color sub-value of a shadow
  • the color sub-value of a border

So, perhaps this section could be reworked into something that just states that the new object-style color values can be used throughout the spec and include an example of each of those things?

Colors can be represented through various formats. For color tokens, the `$type` property MUST be set to the string `color`. The `$value` property can then be used to specify the details of the color, including color space, alpha settings, and more. The `$value` object contains the following properties:

- `$hex` (required): the hex color to use as the default or a guaranteed fallback
- `$colorSpace` (optional): An object detailing an alternative color space to be used to interpret the color, if supported
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Update this list with current format. object keys with current feedback:

Note: The wording is just a suggestion and can be entirely rewritten (please change / improve / rewrite everything!)

Suggested change
- `$colorSpace` (optional): An object detailing an alternative color space to be used to interpret the color, if supported
- `colorSpace` (required): a string representing the predefined colorspace. This value MUST be a value from CSS Color Module Level 4’s [predefined colorspaces](https://www.w3.org/TR/css-color-4/#predefined).
- `channels` (required): an array representing the channel values of the colorspace, excluding alpha. The array’s values MUST be numbers, and MUST have a length of 3. The range of the values depend on `colorSpace`, and match the ranges defined in CSS Color Module Level 4’s [predefined colorspaces](https://www.w3.org/TR/css-color-4/#predefined).
- `alpha` (optional): the alpha channel. The value MUST be a number between `0` (0%) and `1` (100%). If omitted, `1` is assumed.
- `hex` (optional): a string representing an sRGB fallback for colors outside the sRGB gamut. The value MUST be a valid 6-digit hexadecimal code. `alpha`, if supplied, will also apply to the fallback.


</aside>

## Other color space options
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: remove this entire section, including all the colorspaces? (this can be found in the colorSpace table)

- `$hex` (required): the hex color to use as the default or a guaranteed fallback
- `$colorSpace` (optional): An object detailing an alternative color space to be used to interpret the color, if supported

The `$colorSpace` object has the following properties:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Make a subheading for colorSpace and channels together, since they are validated together:
Suggested change
The `$colorSpace` object has the following properties:
## `colorSpace` and `channels`

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: make a markdown table that restates the information from CSS Color Module Level 4 that contains:

  • All the accepted colorSpace values accepted
  • For each colorSpace value, the min/max values allowed per channel. This is important because it won’t always be [0, 1] (e.g. the “H” in HSL is [0, 360]).
    • Question: should we use mathematical syntax that uses square brackets for inclusive ranges?
  • Some sort of introduction to this table so people have more context
| colorSpace | channel[0]  | channel[1]  | channel[2]  |
|:-----------|:------------|:------------|:------------|
| `srgb`     | [0, 1]      | [0, 1]      | [0, 1]      |
…
| `lab`      | [0, 1]      | [-125, 125] | [-125, 125] |

Note: the column names may change to whatever is most readable

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: should we explain why it’s an array, and why the order matters (following CSS Color 4 spec)? Or is that too much information?

- `$components` (required): the non-alpha pieces of the color, listed as an array of floating-point numbers or integers
- `$alpha` (optional): the alpha component of the color, listed as a floating-point number integer in the range of 0 to 1. If omitted, color is assumed to be fully opaque

## Hex - required support
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Rename to ensure this is now optional
Suggested change
## Hex - required support
## `hex` fallback


## Hex - required support

For the color value, the required fallback format to represent color through design tokens is a [hex triplet/quartet](https://www.w3.org/TR/css-color-4/#typedef-hex-color) value. A hex triplet is a 6-digit, 24 bit, hexadecimal number that represents Red, Green, and Blue values as `#RRGGBB`. An eight-character hex will include the alpha value in the last 2 characters. If no alpha value is specified, it is assumed the color if fully opaque.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Rewrite these 2 paragraphs to explain why a person may or may not use the hex property
  • Remove “quartet,” and only allow 6-digit hexadecimal
  • Remove the “Pros / Cons” table?

@@ -77,16 +79,29 @@
<h1>Introduction</h1>

<p>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • (This comment still needs to be addressed—this info should all go into technical-reports/format/types.md)

@@ -79,7 +79,8 @@ Group keys without a dollar sign (`$`) prefix denote:
"Group of tokens": {
"$description": "This is an example of a group containing a single token",
"Token name": {
"$value": "#000000"
"$value": "#000000",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Here and blow, for $type: "color" tokens, update examples to match current updated format

@@ -12,7 +12,7 @@ If an explicit type is set, but the value does not match the expected syntax the

## Color

Represents a 24bit RGB or 24+8bit RGBA color in the sRGB color space. The `$type` property MUST be set to the string `color`. The value MUST be a string containing a hex triplet/quartet including the preceding `#` character. To support other color spaces, such as HSL, translation tools SHOULD convert color tokens to the equivalent value as needed.
Represents a 24bit RGB or 24+8bit RGBA color in the sRGB color space. The `$type` property MUST be set to the string `color`. The `$value` property MUST be a string containing a [hex triplet/quartet](https://www.w3.org/TR/css-color-4/#typedef-hex-color) including the preceding `#` character. To support other color spaces, such as HSL, translation tools SHOULD convert color tokens to the equivalent value as needed.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -65,7 +66,7 @@ <h1>Modules</h1>
<li>
<a href="./format/">Format</a>
</li>
<li>Color (coming soon)</li>
<li><a href="./color/">Color</a></li>
<li>Animations (coming soon)</li>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unknown how best to update this (related to @c1rrus’ comment), but perhaps we consolidate everything into format/types.md?

@@ -0,0 +1,389 @@
# Token naming
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Remove this document? Since we’re updating the format itself, and not providing recommendations (this starts to conflict with other upcoming proposals like modes), we could always split out ideas here into separate proposals or separate conversations to be discussed apart from the color changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
To be reviewed by editors Issues that need to be reviewed in an upcoming meeting between editors.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants