-
Notifications
You must be signed in to change notification settings - Fork 63
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 format adjustments #257
base: color-first-draft
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for dtcg-tr ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for updating this! Added some comments around updating the examples to match the format we last talked about in meetings. Note that small matters of channel
vs components
we can vote on / decide later! But I think it would be helpful to at least have all the examples be internally-consistent, and matching with the CSS Color Module 4 style of just using colorSpace
to refer to space + profile + gamut.
@@ -29,7 +29,7 @@ For example, initially color tokens may be defined as such: | |||
|
|||
```json | |||
{ | |||
"Majestic magenta": { | |||
"Hot pink": { | |||
"$type": "color", | |||
"$value": { | |||
"$hex": "#ff00ff" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this file, we should just define it in the srgb
space, and we don’t need the hex
fallback:
"$hex": "#ff00ff" | |
"colorSpace": "srgb", | |
"channels": [1, 0, 1] |
Here and below they should follow the same format
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the feedback! No changes have been added to this file yet. Once we get alignment on the rest, we can make adjustments throughout here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a suggestion: it is easier to review if this PR made everything consistent in all files, since that has to be done one way or another. Otherwise it ends up in a state of confusion not being able to tell where the proposal starts and where it ends. Would it be OK to just make everything a consistent format so it’s clear to reviewers?
"$type": "color", | ||
"$value": { | ||
"$hex": "#c44587", | ||
"$hex": "#ff00ff", | ||
"$colorSpace": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here, and below, these should align with the current spec:
"$colorSpace": { | |
"colorSpace": "srgb", | |
"channels": [1, 0, 1], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above comment
@@ -423,7 +423,7 @@ Describes a gradient that is solid yellow for the first 2/3 and then fades to re | |||
{ | |||
"brand-primary": { | |||
"$type": "color", | |||
"$value": "#99ff66" | |||
"$value": "#00ff66" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"$value": "#00ff66" | |
"$value": { | |
"colorSpace": "srgb", | |
"channels": [0, 1, 0.4] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can make this update in the interim. The color format editors have a different proposal for color gradient, and we should sync with format to align/make recommendations for community feedback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK! Yes breaking out changes to gradient as a separate PR would be easier to review! This PR shouldn’t introduce any changes to gradient. Because, yes, gradients do use colors, that doesn’t mean gradients change necessarily. Then we can review those unique changes easier
technical-reports/format/types.md
Outdated
| Key | Type | Required | Description | | ||
| :------------- | :------------: | :------: | :--------------------------------------------------------------------------------------------------------------------------------------- | | ||
| `colorSpace` | `string` | Y | An string representing the color space. | | ||
| `colorProfile` | `string` | | String representing url of color profile. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we are not using colorProfile, and just aligning with CSS Color Module 4? colorSpace
is a shorthand for space + profile + gamut, also borrowing from the same spec
| `colorProfile` | `string` | | String representing url of color profile. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In CSS Color Module 4 color terminology section, here's the definition of color space:
A color space is an organization of colors with respect to an underlying colorimetric model, such that there is a clear, objectively-measurable meaning for any color in that color space. This also means that the same color can be expressed in multiple color spaces, or transformed from one color space to another, while still looking the same.
Color profile is different, it's about using a particular ICC color profile for device specific implementations (print, for example). For example, you can use the CMYK color space and choose more than one color profile to implement those colors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You’re 100% right, and yes I do see us needing this. As we talked about earlier today, perhaps we limit this initial PR to CSS Color Module 4 where:
- only
colorSpace
is used colorSpace
is limited to the allowed values in CSS Color Module 4 (srgb
,srgb-linear
,display-p3
, etc.)
Then, we open a separate PR to expand the format to include CSS Color Module 5’s concept of @color-profile
. That way we can get feedback on that separately. Not being prescriptive, but if we copy CSS Color Module 5’s mental model, I could see it introducing a new $type: 'colorProfile'
token type that maps 1:1 to @color-profile
(perhaps; maybe we think of another pattern!)
technical-reports/format/types.md
Outdated
| :------------- | :------------: | :------: | :--------------------------------------------------------------------------------------------------------------------------------------- | | ||
| `colorSpace` | `string` | Y | An string representing the color space. | | ||
| `colorProfile` | `string` | | String representing url of color profile. | | ||
| `coordinates` | `number array` | Y | An array of numeric values representing individual color coordinates. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we were leaning toward “channels” being the term. Also just in the interest of being really specific, “an array” doesn’t specify the length of the list. Calling it a “tuple of 3 values” means that the values must always be 3—no more, no less. Just like any color in CSS, I think we should enforce that the length is always 3, no matter what.
| `coordinates` | `number array` | Y | An array of numeric values representing individual color coordinates. | | |
| `channels` | `[number, number, number]` | Y | Tuple of 3 numbers representing the 3 components of the color space. The allowed ranges depend on the `colorSpace` specified. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we were leaning towards that!
The terms component
and channel
were used in different capacities in the W3C CSS Color Level docs, sometimes interchangeably. I agree that we should standardize terminology and clarify what this means.
For example in the CSS Color Level 4 color type definition it reads:
Colors in CSS are represented as a list of color components, also sometimes called “channels”, representing axises in the color space. Each channel has a minimum and maximum value, and can take any value between those two. Additionally, every color is accompanied by an alpha component, indicating how transparent it is, and thus how much of the backdrop one can see through the color.
The CSS Color Level 5 spec builds upon Level 4, yet the terminology is still interchangeable.
In ColorJS, the term "coordinates" could be shorthand for component coordinate or channel coordinate if I assume correctly (need to confirm).
Happy to move back to channels
for now.
Also, I suggest we adjust wording to include none
along with numbers in the allowable channel value types, to account for 4.4“Missing” Color Components and the none Keyword support.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I suggest we adjust wording to include
none
along with numbers in the allowable channel value types, to account for 4.4“Missing” Color Components and the none Keyword support.
Oh smart! I like the idea of aligning with all that great thinking as much as possible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, there have been interest in this support in the legacy color thread!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh also I think as I said in another comment, maybe we like components
better and that’s fine with me! I don’t have any strong opinions. We could decide on the exact words at the last minute without it changing the proposal too much.
technical-reports/format/types.md
Outdated
| `colorSpace` | `string` | Y | An string representing the color space. | | ||
| `colorProfile` | `string` | | String representing url of color profile. | | ||
| `coordinates` | `number array` | Y | An array of numeric values representing individual color coordinates. | | ||
| `alpha` | `number` | | An integer or floating-point value representing the numeric value. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should specify this MUST be in the range of (0, 1)
(inclusive). 0
= 0%, 1
= 100%.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can do. Where should this sentence occur?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anywhere in the alpha
description
"$type": "color", | ||
"$value": { | ||
"colorSpace": "display-p3", | ||
"colorProfile": "http://example.org/display-p3.icc", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here and below, the format should match the updated syntax of colorSpace
+ channels
+ alpha
(optional) + hex
(optional)
"colorProfile": "http://example.org/display-p3.icc", | |
"channels": [0.92, 0.2, 0.97], | |
"hex": "#ff00ff" // HEX 6 supported only |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this suggestion! I should clarify why the optional colorProfile
is listed here. I was thinking of the cases for print where a specific color profile should be associated with the color space for conversion. Perhaps we do a separate callout for this and use CMYK as the color space example.
Summary
This adds new
$colorSpace
,colorProfile
,coordinates
, andfallback
properties for the color type$value
object as suggested by multiple discussions.Reasoning
This PR takes into account feedback from followup comments with respect to loosening up the color type and gracefully handling legacy color scenarios.
Pros
Cons
$color
in the future would be a breaking change, yet the release of the color format module will already be a breaking change.Alternatives
$components
/$channels
) was modified to align with both CSS Color Module Level 5 and ColorJS naming conventions for consistency.color
module will provide expanded guidance on this format. Or in areas where they potentially overlap, differences will be complementary rather than conflicting.Notes