-
Notifications
You must be signed in to change notification settings - Fork 35
Conversation
Not sure why linting is failing in CI. It does not fail for me locally |
Random thought should we call these “variables” instead of “variants”? |
I don't feel like that change improves understanding when compared to "variants". |
My thinking was these are actually variables as opposed to our current colors which are constants. The value varies depending on dark, light, or theme (for web). Variants also indicates a sibling relationship when this is more like a parent/child relationship. The role variables belong to their given role because they are derived from the role’s base color. |
Updated the base branch to |
Okay I’m happy with where this is at for a first iteration and think it is ready for review. Additional features like adding in metadata, etc I'd like to leave for future beta releases. As this stands, it should support both Polaris React and Polaris Rails. |
const {saturationAdjustmentFn, hueRotationFn} = require('../utils'); | ||
|
||
const base = { | ||
surface: [ |
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’d like to leave the scheme as-is for now, get a working beta released, then iterate on adding metadata which will be part of the generated files. Specifically the immediate plan for metadata is to add info that can be used to filter colors based on various dimensions.
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'm good with this great first step going out as a beta
Thank you for bringing me in this process along the way!
A tiny bit of docs would be appreciated but they can come later.
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.
Reviewed in person and all looking amazing!
Let's add a d.ts. file for the color-factory then get this merged, and then we can do follow-up stuff like getting TS integrated and pondering about that theo two step transform thing later.
gulp | ||
.src('tokens/themes/*.yml') | ||
.pipe( | ||
$.theo({ |
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.
Useing theo to transform a theme into a palette, which is then passed into theo again feels a little odd to me. I wonder if we could do this with plain old JS, rather than having to register a theo transform.
That can be a problem for later though, no need to block this PR
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.
Leaving this comment unresolved for posterity and future PRs
Co-authored-by: Dan Rosenthal <dan.rosenthal@shopify.com>
32ad306
to
46526a8
Compare
* Added surface disabled variant and updated other variant configs ([#101](#101)) * Added TypeScript build for modern tokens, and shifted directory structures to differentiate between legacy and modern tokens ([#97](#97)) * Updated variant names and descriptions ([#96](#96)) * Added decorative icon variants ([#94](#94)) * Built changes from previous release, and added textOnInteractive variant ([#93](#93)) * Fixed an issue where legacy themes caused the color factory to throw ([#92](#92)) * Update color variants for consistency with changes in Polaris React ([#91](#91)) * Marked the config as optional and the colors as partial ([dd3d8fc](https://github.com/Shopify/polaris-tokens/commit/ * Actually exporting the types would be helpful ([4998856](4998856)) * Added the Color Factory. Long live the Color Factory! ([#89](#89)) Co-authored-by: Dan Rosenthal <dan.rosenthal@shopify.com> Co-authored-by: Ben Scott <227292+BPScott@users.noreply.github.com> Co-authored-by: Kyle Durand <kyle.durand@shopify.com> Co-authored-by: Sara Hill <sara.hill@shopify.com>
Addresses #71
The PR is the first of a few to move the color factory from Polaris React to Polaris Tokens. Here is a high level diagram of the color system, including the color factory:
Todo