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

8690 task: Added colours to storybook #7

Merged
merged 4 commits into from
Aug 3, 2021

Conversation

creido-welly
Copy link
Contributor

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

Initial commit for adding colour documentation to Storybook based on existing Sketch designs https://app.zeplin.io/project/5b4e06b48ae4580d4871178a/screen/5ea86a03c8963abb60b549cd

To test

  1. Checkout this branch
  2. npm run install
  3. npm run storybook

Screenshot 2021-08-03 at 12 17 05

@@ -0,0 +1,82 @@
export default {
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 file is taken from the token build process from this work in progress PR
#2

Copy link
Member

Choose a reason for hiding this comment

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

Would an aim be to eventually remove this file and rely on design tokens from an external source?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes this file is a stop-gap. The idea is to remove this and any similar build files from the codebase in favour of an npm process which uses tokens in JSON format to generate all outputs needed.

For more context, this file was generated by Style Dictionary using config and colour/base.json from the afore mentioned PR.

Are you specifically talking about an external source from a product theming standpoint?

Copy link
Member

Choose a reason for hiding this comment

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

Yep. Was concerned that we're defining tokens within the npm package itself (design-system), which seems like duplication (was imagining a single source of truth somewhere, probably a JSON file, which could be edited by designers to update values, which then permeate through the many apps which consume the design tokens).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the end result will be the single source of truth you speak of

Copy link
Member

@chriddell chriddell left a comment

Choose a reason for hiding this comment

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

I must admit I struggle to see the argument for using "colour" over "color"... I find when I'm writing code that "color" is the first spelling I opt for, before realising that the variables don't use that spelling.

I would prefer if we used "color" for consistency (with CSS property), unless there is a reason for using the UK spelling?

@creido-welly
Copy link
Contributor Author

I must admit I struggle to see the argument for using "colour" over "color"... I find when I'm writing code that "color" is the first spelling I opt for, before realising that the variables don't use that spelling.

I would prefer if we used "color" for consistency (with CSS property), unless there is a reason for using the UK spelling?

It's a fair point. This has been carried across from CC and I think the reason we used "colour" was because that was what the original designs used. I'll add a comment to the ticket for discussion.

@creido-welly creido-welly requested a review from chriddell August 3, 2021 11:07
@creido-welly
Copy link
Contributor Author

All instances of colour which are not documentation based (I include Storybook file names in that) have been changed to color.

Copy link
Member

@chriddell chriddell left a comment

Choose a reason for hiding this comment

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

Was it a conscious decision not to change filenames to US spelling (colors)?

@creido-welly
Copy link
Contributor Author

creido-welly commented Aug 3, 2021

Was it a conscious decision not to change filenames to US spelling (colors)?

For Colours.stories.tsx and Colours.stories.mdx yes. I was on the point of changing them but in the file system they almost read as the headings in Storybook.

Screenshot 2021-08-03 at 12 32 57

I'll change them if you like?

@chriddell
Copy link
Member

No suggestion from me either way, just checking whether it was an oversight!

@creido-welly creido-welly merged commit f9c0063 into develop Aug 3, 2021
@creido-welly creido-welly deleted the task/8690-storybook-colours branch August 3, 2021 12:26
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.

2 participants