Skip to content

Conversation

@aaronccasanova
Copy link
Member

WHAT is this pull request doing?

This PR extracts the Polaris design tokens, previously implemented in TypeScript, to JSON files.

  • Converted TypeScript design tokens to JSON
  • Created JSON Schema's for all token groups
  • Added ajv-cli to validate JSON token files
  • Added a validate-tokens script and integrated in our CI

Follow up tasks

  • Update Polaris CustomProperties component to use these JSON tokens
  • Update Polaris custom-properties-allowed-list Stylelint plugin to use these JSON tokens

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

Copy-paste this code in playground/Playground.tsx:
import React from 'react';
import {Page} from '../src';

export function Playground() {
  return (
    <Page title="Playground">
      {/* Add the code you want to test in here */}
    </Page>
  );
}

🎩 checklist

package.json Outdated
"storybook": "start-storybook -p 6006 --quiet",
"storybook:build": "yarn run copy-polaris-tokens && build-storybook -o build-internal/storybook/static"
"storybook:build": "yarn run copy-polaris-tokens && build-storybook -o build-internal/storybook/static",
"validate-tokens": "ajv validate -s tokens/schema.token-group.json -d 'tokens/_!(color)*.json' && ajv validate -s tokens/schema.color.json -d 'tokens/_color.json'"
Copy link
Member

@alex-page alex-page Dec 8, 2021

Choose a reason for hiding this comment

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

Could this be simpler by breaking the color files into individual JSON files? color.light.json

Copy link
Member Author

Choose a reason for hiding this comment

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

Great idea 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

This will allow us to remove the color specific json schema 🙌

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay this commit:

  • Removed the _ prefix from all token files
  • Extracted color scheme tokens into individual files (e.g. color.light.json & color.dark.json)
  • Simplified the validate-tokens script

@github-actions
Copy link
Contributor

github-actions bot commented Dec 8, 2021

size-limit report

Path Size
cjs 156.99 KB (0%)
esm 90.01 KB (0%)
esnext 138.52 KB (0%)
css 35.9 KB (0%)

Copy link
Member

@alex-page alex-page left a comment

Choose a reason for hiding this comment

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

Love this. One thought, we separated the .ts files by adding the _. Now it's not as needed as the tokens are in .json files. We could just remove the _ now.

@aaronccasanova aaronccasanova added the 🤖Skip Changelog Causes CI to ignore changelog update check. label Dec 8, 2021
Comment on lines 1 to 8
{
"$schema": "http://json-schema.org/draft-07/schema#",
"title": "Token group schema",
"type": "object",
"additionalProperties": {
"type": "string"
}
}
Copy link
Member Author

Choose a reason for hiding this comment

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

If folks haven't worked with JSON Schema this is validating that our tokens are objects with a depth of 1 whose values are strings. Here is an example of the error thrown if we violate this constraint:
Screen Shot 2021-12-08 at 3 13 41 PM

@BPScott
Copy link
Member

BPScott commented Dec 9, 2021

I'd love to see a demo of how these files can get included by the source code

@aaronccasanova
Copy link
Member Author

I'd love to see a demo of how these files can get included by the source code

@BPScott Your in luck! I just submitted a PR to update the CustomProperty component: #4811

Copy link
Member

@kyledurand kyledurand left a comment

Choose a reason for hiding this comment

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

This looks good but we're back to px values for spacing and shape. Can we follow up to be sure we're converting these to rems?

@alex-page
Copy link
Member

alex-page commented Dec 9, 2021

This looks good but we're back to px values for spacing and shape. Can we follow up to be sure we're converting these to rems?

If possible I would prefer the values to be pixel based in JSON file and converted to rems in ts. Unsure how complex that would be though. Happy to ship rem values in JSON to speed this up for now.

@kyledurand
Copy link
Member

If possible I would prefer the values to be pixel based in JSON file and converted to rems in ts. Unsure how complex that would be though. Happy to ship rem values in JSON to speed this up for now.

Pretty much how our convo went in slack. Namely because rems are pretty hard to read, hard to convert from figma and that could lead to a misunderstanding of the values and increased friction in use / contribution.

I can't see it being too difficult to build but getting it to be performant could get tricky

@aaronccasanova
Copy link
Member Author

@kyledurand @alex-page Here is my first pass: 9856314. Thoughts?

@aaronccasanova
Copy link
Member Author

In an unfortunate git accident this PR was wiped and automatically closed by GitHub. Luckily all the commits we're preserved in the Update CustomProperty component PR# 4811 . There doesn't appear to be a way to restore this PR and thus the contents have been moved to the following PR: #4818

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🤖Skip Changelog Causes CI to ignore changelog update check.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants