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

8709 task: design tokens used to output styles and documentation (colours) #2

Open
wants to merge 12 commits into
base: develop
Choose a base branch
from

Conversation

creido-welly
Copy link
Contributor

@creido-welly creido-welly commented Jul 9, 2021

Relates to wellcometrust/corporate#8709

Context

Design tokens will be the core of the design system long-term. Initially however the aim is to automate the process of outputting CSS variables for styling and also documentation for Storybook from a single source (i.e. a JSON file src/tokens/colour/base.json). Amazon's Style Dictionary enables us to do this very easily and for the long-term could be used to output other style formats e.g. iOS, Android, Kotlin.

This PR is a proof of concept for flat variables. Further discovery needed to work out how to output CSS containing media queries

Design token build files (src/build/css/colours.css, src/build/css/colours.js and src/build/css/colours.d.ts) are included in this commit and not specifically ignored as they are dependencies for the CSS and Storybook output.

To test

To run locally:

  1. pull this branch
  2. run npm install
  3. run npm run storybook and check Globals > Colours
  4. optionally, to preview how design tokens are generated run npm run build:tokens to output css, js and d.ts to src/build
  5. run npm run build to output React and CSS for consumption in the dist folder

@creido-welly creido-welly requested a review from chriddell July 9, 2021 15:54
@@ -0,0 +1,39 @@
{
Copy link
Contributor Author

@creido-welly creido-welly Jul 9, 2021

Choose a reason for hiding this comment

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

Work in progress, not currently used to output CSS or docs

* initial work with using design tokens and style dictionary to output css variables for styling and js modules for documentation
* initial work with using design tokens and style dictionary to output css variables for styling and js modules for documentation
@creido-welly creido-welly force-pushed the task/8709-design-token-prototyping branch from 1a6e6c6 to c0e2ec6 Compare July 9, 2021 16:11
<tbody>
{tokenKeys.map((key) => {
const value = colourTokens[key];
const cssValue = /rgb/i.test(key) ? `rgb(${value})` : value;

Choose a reason for hiding this comment

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

This could fail if someone used the letters rgb in their variable name. For example, FlagColourGB. Doing a regex against the value might be safer than doing it against the key.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This line has been removed. Ultimately if we add RGB values we can handle that using a Style Dictionary transform.

"black": { "value": "#000" },
"black-rgb": { "value": "0, 0, 0" },
"white": { "value": "#fff" },
"white-rgb": { "value": "255, 255, 255" },

Choose a reason for hiding this comment

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

I'm wondering whether we couldn't auto-generate the rgb variants, rather than specify them manually ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like that idea. We could also auto output different formats such as HSL in future. It would also negate the issues around key names you highlighted

Copy link
Contributor Author

Choose a reason for hiding this comment

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

RGB values removed from the token file in favour of automatically inserting using a Style Dictionary transform.

src/utils/change-case.ts Outdated Show resolved Hide resolved
src/utils/change-case.ts Outdated Show resolved Hide resolved
@aliceh75
Copy link

I've left a few comments. As a general feedback, I'm wondering whether generated files (the ones in src/build) should be added to the repository, or should be auto-generated by the build system. There are pros and cons to either approach - we should make sure we weigh them for our use case. Maybe you've already done this thought process, in which case it would be useful to document it ?

Pros of including them in the repository:

  • Local devs don't need to re-generate the files when they make changes. This might be particularly important for designers who want to use them.
  • This can potentially be used without a build system (though I expect other requirements mean that wouldn't be an option)
  • For this PR here, it was useful to be able to look at the output

Pros of generating them on-demand:

  • The files don't show in commits and pull requests
  • There's no risk of someone making a commit where the source version and build version are inconsistent

For me that last point - the risk of committing something inconsistent - is quite a strong one. It could be mitigated by CircleCi build checking for consistency.

@creido-welly
Copy link
Contributor Author

@aliceh75 thanks for the feedback. I agree with you that committing build files to the repo seems to be the wrong approach given the reasons you mentioned above. As I mentioned in the PR blurb the only reason they are included is because the CSS and documentation output depend on them but I'm still not 100% happy with the approach.

If the build folder was ignored there is a risk of errors being raised when a user tries to build locally if the tokens haven't yet been generated. The obvious way to mitigate that would be to include npm run build:tokens at the beginning of every relevant npm script e.g.

"storybook": "npm run build:tokens && start-storybook -p 6006",

For me that last point - the risk of committing something inconsistent - is quite a strong one. It could be mitigated by CircleCi build checking for consistency.

I guess that's the alternative.

… task/8709-design-token-prototyping

# Conflicts:
#	.storybook/styles/storybook-app.scss
* added token build process to build and dev processes to ensure generated variables are always available
* removed rgb colour values from tokens list
@creido-welly creido-welly force-pushed the task/8709-design-token-prototyping branch from 10da041 to d436a4c Compare July 30, 2021 14:10
@creido-welly
Copy link
Contributor Author

@aliceh75 thanks for the feedback. I agree with you that committing build files to the repo seems to be the wrong approach given the reasons you mentioned above. As I mentioned in the PR blurb the only reason they are included is because the CSS and documentation output depend on them but I'm still not 100% happy with the approach.

If the build folder was ignored there is a risk of errors being raised when a user tries to build locally if the tokens haven't yet been generated. The obvious way to mitigate that would be to include npm run build:tokens at the beginning of every relevant npm script e.g.

"storybook": "npm run build:tokens && start-storybook -p 6006",

For me that last point - the risk of committing something inconsistent - is quite a strong one. It could be mitigated by CircleCi build checking for consistency.

I guess that's the alternative.

@aliceh75 further to this previous discussion, the npm script has been updated to ensure tokens are generated before dependent processes run. This is really a temporary stop-gap while we work out a more suitable architecture.

@creido-welly creido-welly requested a review from aliceh75 August 6, 2021 13:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants