-
Notifications
You must be signed in to change notification settings - Fork 68
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 first draft (updated) #266
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: Ayesha Mazumdar <[email protected]> Co-authored-by: Adekunle Oduye <[email protected]> Co-authored-by: Kathleen McMahon <[email protected]>
…community-group into color-first-draft
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
✅ 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.
This is looking really good! Main highlight is just syncing this up with CSS Color Module 4 wherever possible in terms of specific number ranges and min/max values etc.
Re: supported colorspaces we’re open to supporting a subset of Color Module 4 if there are really clear, defined reasons! But as some of that spec’s editors highly encouraged us to match parity with that, and are expecting that, I’d like to just have “match Color Module 4” as the starting point and for every deviation we should make it clear why we can’t implement X, Y, Z, etc.
I'm late to the game, but let us consider that parsing color values in the generation of the design-token.json may not be the proper responsibility for design-token JSON files. Simply put, the spec does not need to declare a key/value for colorspace, components, or alpha to be of use. Because the CSS Color() spec is well defined and requires a few simple regex to properly lift the values and the color space/model, I suggest the transformer (Style Dictionary) is responsible for parsing any valid CSS Color() string given within the $value key. So long as the color is valid, it can be parsed into any other space/profile as desired for the target platform with no visual difference (except out-of-gamut, of course). Still, a fallback color space/model could be valuable where we do not want (or trust) the transformer to convert into another color space (reasons). If so, consider reuse of the CSS pattern for font-family, where we comma delim the values A proposed example...
The advantage is leveraging known patterns/standards established by CSS, maintaining separation of responsibility by allowing transformers to do what they are built to do, and simplifying the output in JSON. I'd further propose the choice of output space/model be embedded in Style Dictionary according to design/business decisions per platform and/or brand. All colors could be transformed to hex for pure CSS output, but SCSS may respect the first definition in comma delim array, (hex, hsl, oklab, oklch). For iOS, everything could be transformed to P3 or kept in Hex, or even a mix if possible. |
- update xyz-d65 to clarify use of d65 illuminant - make sure bounded scales match css color module 4 definitions - remove a few references to scss/sass naming - update headers to use h5s instead of bold
@caoimghgin Thanks for suggesting this! At this point we are conforming to CSS Color Module 4, per your suggestion. So everything you’d like to accomplish will work with our current version of the proposed syntax! But at this time we’re not open to considering “just accept any CSS string” since those are famously NOT easy to parse, and it is NOT a simple regex to get the values (they support arbitrary whitespace, numbers and percentages, in many functions are very lax when it comes to commas and other characters, etc.). It’s only trivially parseable if you defined a very limited subset of what CSS allows. But the good news is we’ve done that! We have a nearly 1:1 translation of CSS Color Module 4, just in JSON object notation form. No RegEx needed, and static JSON is much friendlier as a universal interface to every programming language. JS may make parsing, data types, and memory assignment easy, but many other languages do not and parsing CSS would be a significant technical barrier and beyond the scope of this 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.
This looks great! There are a few examples which need to be updated but overall this gets my stamp. Would love @kaelig to review as well though
|
||
| Color space | Value | | ||
| ------------ | ---------------- | | ||
| sRGB | `srgb`, `rgb` | |
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.
Would love @kaelig’s input on this but I know CSS Color Module 4 is really explicit about srgb
, and not conflating it with RGB which now describes many other colorspaces (Culori does, which IMO shouldn’t be followed). Even as an alias I’m worried it may cause confusion in the future (maybe; open to arguments if it’s helpful!)
| sRGB | `srgb`, `rgb` | | |
| sRGB | `srgb` | |
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 i was following the convention in CSS Color Module 4 where you can define an srgb color with either the srgb()
and the rgb()
function, where rgb is just an alias of srgb. I think that's why Culori allows either — it lets the user dictate which function should appear when outputting to CSS. By definition the color itself is identical no matter which one you use, so in this case I think it's a semantics/syntactics issue.
It's true that in the color()
function doesn't include rgb
— if we want to narrow our implementation surface a bit further than just the broad alignment to CSS Color Module 4, we can say up front that we're specifically aligning to the color()
function and modern syntax. I'm up for that if y'all think it would help us be more precise!
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.
Actually, a note here that may preclude us to sticking strictly to the color()
function; hsl, hwb, lab, lch, oklab, and oklch aren't supported there, since they're all transforms of other spaces. While in practice limiting scope to the color()
function doesn't limit what colors can be expressed, I can see authors wanting to use one of those transformations. So, we gotta figure out a way to make it logical which syntax (named functions vs. color()
function) we are preferring where.
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.
These are all great callouts, and we haven’t dug in! Probably worth a larger discussion for sure.
For the purposes of this PR / just updating everything, happy to let you “flip a coin” and decide something for the sake of proposal. But agreed we need to set boundaries on terminology here because just throwing around a link to that spec isn’t enough.
Thanks for replying. I have no doubt we are conforming to CSS Color Module 4. Having recently pulled code from https://colorjs.io/ and inserted an isValid function as a PR, I see the challenges of “just accept any Color() CSS string”, but it appears color.js solved to a satisfactory level. Advantage: No need to create another format using JSON object notation. I'm not certain I've encountered a case where other languages are unable to RegExp as easily as JS, but I haven't used every language. Indeed, I'm struggling to imagine what language aside from JS would be used. I suppose the communication is that with JSON object notation format, we've invented another standard and we're placing a lot of responsibility on the JS plugin to parse color data from Figma/Sketch/etc where I suspect that responsibility is better placed in the transformer. It may seem difficult to separate what is good practice from personal preference, but happy to have a forum to share. |
Parsing/lexing requires breaking down a very complex string into an abstract syntax tree (AST), many of which are JSON-serializable or a generic enough format to be universal. Taking a universal, JSON-serializable AST format and obfuscating it back into a DSL has no advantages and only downsides. Also, in regards to “no need to create another format,” that’s the entire purpose of the w3c design tokens spec—to outline and create another format. That is the overarching goal of this project. We are defining that format here. Further, color.js has NOT solved it to an adequate level. It is still alpha software, does not support the spec fully yet, and has performance problems. Not to say it can’t improve! But we also do not want to prop up this specification on the back of a single open-source alpha project.
Swift, Kotlin, and Java are three common consumers of the DTCG format, building iOS, Android, and macOS apps. Go and Rust are second-tier languages also within scope, being 2 languages that frequently overlap with JS/TS and web. It is directly the DTCG format’s concern to outline a non-web, non-JS centric universal format, to which CSS would be a significant technical barrier. One example worth looking into is Rust’s serde, which deals with serialization/deserialization between JSON and data structures in Rust, and can automatically handle memory assignment. Adding CSS parsing to, say, a Rust library’s plate for consuming DTCG is a step backwards. JSON is AST-compatible, universal, and there are myriad utilities in every programming language to deal with it as a universal language. Few other languages apart from JS have an adequate CSS parser… because they don’t need one (and personally I would go even further and argue JS itself doesn’t have a perfect CSS parser either!). Again, appreciate the input, but for this specific proposal CSS strings are out of the question at this stage. Please feel free to give specific feedback on the proposed JSON! Worth pointing out that this PR wasn’t meant to “slip in” changes quietly to the color proposal. It’s been in the works for a couple years and has gotten input from many talented individuals, even authors on the CSS Color Module 4 spec. But even after this PR merges, it’s not a “feedback closed; everything is finalized” situation. Please just see this as preparation for final input/review and we’ll tag you for more input, within the boundaries of the current structure/format. |
Interesting to see a use case where DTCG output from Figma/Sketch would be used 'raw' inside native apps rather than first transformed from generic JSON to platform-specific code. From that POV, a desire to pre-parse colors into JSON key/values and attach a profile/space seems reasonable. However...
If using the existing standard notation of CSS Color() becomes a possibility, I'd be thrilled to help. Given there have been years of conversations it seems this ship has sailed so I won't muddy the waters any further. |
@drwpow thanks for the latest round of comments! Should be ready for another review (sorry to keep missing things!). I ended up going a bit wild and formatting the allowed color spaces and values into a table because I got tired of jumping back and forth. (note to CSS Color Module 4 editors, feel free to copy my homework 😛 ) |
Formatting and updating changes:
Additional changes to discuss:
colorSpace
should be: all the modes with color functions in CSS Color Module 4, plus Display P3 (supported in FIgma and most platforms) , and XYZ65 (the foundation of all other color spaces, useful as a universal interchange space).Things I didn't change, but we should discuss: