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

Output new tokens to new directory. Output new and legacy namespaces #56

Closed
wants to merge 6 commits into from

Conversation

xdrdak
Copy link
Contributor

@xdrdak xdrdak commented Dec 3, 2019

Description

Generated CSS/Sass tokens/utilities was starting to get out of sync.

I've quickly updated the generation scripts to output the new values.

I've also taken the time to namespace the newly generated files.

So...

before:

@lightspeed/flame-tokens/partials/index.scss

now:

@lightspeed/flame-tokens/sass/${THEME_NAME_HERE}/partials/index.scss

How to test?

  • Checkout branch, run yarn dev
  • Open Storybook
  • Or check the deploy preview on Netlify (link available in comments)

Checklist

@netlify
Copy link

netlify bot commented Dec 3, 2019

Deploy preview for lightspeed-flame ready!

Built with commit 81b19e0

https://deploy-preview-56--lightspeed-flame.netlify.com

@xdrdak xdrdak marked this pull request as ready for review December 6, 2019 13:56
@xdrdak xdrdak requested a review from a team as a code owner December 6, 2019 13:56
@glambert glambert mentioned this pull request Dec 10, 2019
Copy link
Collaborator

@glambert glambert left a comment

Choose a reason for hiding this comment

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

Looks good to me. The only thing I would do is remove /partials in the namespace and put generated tokens directly in @lightspeed/flame-tokens/sass/${THEME_NAME_HERE}/. Would that cause a breaking change?

@glambert
Copy link
Collaborator

Also, do we need to add/update docs for the usage of these tokens?

Copy link
Collaborator

@glambert glambert left a comment

Choose a reason for hiding this comment

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

Alright this is 👍, LGTM 🎉

@@ -57,41 +57,45 @@ Note that this command will be run automatically when we publish to npm.
### Import SCSS variables

```scss
@import '@lightspeed/flame-tokens/index.scss';
@import '@lightspeed/flame-tokens/sass/index.scss';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't this supposed to be @import '@lightspeed/flame-tokens/sass/flame/index.scss'; based on your changes?

@xdrdak xdrdak closed this Aug 3, 2020
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