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

Design tokens README usage instructions lead to "Missing script" errors #2086

Closed
Tracked by #246
brian-smith-tcril opened this issue Feb 23, 2023 · 7 comments
Closed
Tracked by #246

Comments

@brian-smith-tcril
Copy link
Contributor

When following the instructions here https://github.com/openedx/paragon/tree/alpha#usage, the last 2 commands lead to "Missing script" errors

npm run build-scss-to-css-map # creats scss-to-css-core.json and scss-to-css-components.json 
npm run replace-variables -- --path ../src # this will replace all scss variables in the project with the new css variables based on the "scss-to-css-core.json" and "scss-to-css-components.json" files

I have a PR out for the first one #2085, but the second one seems to be ambiguous, as there are 2 replace-variables-* scripts in the package.json

replace-variables-usage-with-css
replace-variables-definition-with-css

I'd love to get some clarification on what these steps should be, and what (if any) scripts should be added to the package.json

@adamstankiewicz adamstankiewicz added this to the Design tokens with style-dictionary milestone Feb 24, 2023
@adamstankiewicz adamstankiewicz linked a pull request Feb 24, 2023 that will close this issue
3 tasks
@adamstankiewicz
Copy link
Member

Ah, bringing the conversation about this issue back here instead of the PR itself 😄

Originally, the intent was that we'd be transforming tokens -> CSS variables and then replacing the existing SCSS variable's value with the new CSS variable as the new value instead. This would effectively mean any existing usages of SCSS variables (in Paragon and in consuming applications) would not really need to make any code changes (i.e, replace the SCSS variable to use the CSS variable directly).

However, upon further reflection, we realized that to really achieve the goal of this project to "build the theme once" instead of requiring each consuming applications to effectively build the theme separately, we really need to move away from consumers using Paragon's existing SCSS variables.

As such, we're no longer going to be associating the CSS variables as values for the existing SCSS variables. Instead, we're proposing that Paragon and consuming applications will need to use the new CSS variables directly instead. See #2018 and #2014 for more details

Because of that, we no longer really have a need for the CLI commands to replace the original SCSS variable value with a CSS variable instead, and neither will consumers. The docs in the README in alpha didn't get updated yet to reflect this decision.

@adamstankiewicz
Copy link
Member

adamstankiewicz commented Feb 24, 2023

Here's some updated pseudo-docs describing the intended future that might be helpful:

Compiling CSS from design tokens for Paragon contributions (in this repo)

  1. npm install. Install dependencies, including style-dictionary.
  2. Make changes to design token(s).
  3. npm run build-scss. Transforms the tokens to CSS variables and CSS utility classes, and generates core.css and light.css output files.
    • light.css. CSS variable definitions for colors in the light theme variant.
    • core.css. Contains the majority of Paragon/Bootstrap foundational styles for layout, components, etc. Consumes CSS variables defined by light.css.
  4. Test changes locally (e.g., running the documentation website, the example MFE app, etc.).
  5. Ensure changes to core.css and light.css are committed & released to NPM (which also "releases" them on versioned public CDNs for NPM packages).
  6. Consuming applications would inject the core.css and light.css theme files into their applications via a mechanism similar to feat: adds support for loading external theme CSS for MFEs frontend-platform#440 (ideally pulling from a public CDN for NPM packages, but falling back to locally installed copies, if needed).

Compiling CSS from design tokens for @edx/brand theme authors (in @edx/brand repos)

  1. npm install. Install dependencies, including @edx/paragon.
  2. Create tokens that will override Paragon's default tokens (matching same JSON schema).
  3. npm run build-scss. This @edx/brand repo will have a new NPM script that utilizes a new CLI exported by @edx/paragon that exposes the build-tokens.js script (or possibly another if we end up needing one for the brand packages to run specifically, TBD) for @edx/brand consumers.
    • The intent of running this command is to effectively deep merge the tokens defined in Paragon's default tokens with the override tokens defined by @edx/brand, generating its own core.css and light.css output files (exact output files still a TBD) containing CSS variable overrides based on the token overrides.
  4. Ensure any changes to the generated core.css and light.css files are committed & released to NPM (which also "releases" them on versioned public CDNs for NPM packages).
    • Note: It is a bit unclear still in the above linked implementation POC for @edx/frontend-platform how it would integrate with @edx/brand in this way. Open to suggestions/feedback/ideas here.

@adamstankiewicz
Copy link
Member

Additional clarification on the above, the aforementioned build-scss command isn't really exposed yet, though we have a similar existing command used by the docs site under www. Exposing this NPM script is part of one of tasks in-progress.

@brian-smith-tcril
Copy link
Contributor Author

@adamstankiewicz Those pseudo-docs look great! Based on your comment on #1905, would it make sense to close that one too?

I'm thinking since the alpha branch is an alpha, it would make sense to update the README to reflect the intended future user flow (while noting things that have not yet been implemented). Something as simple as moving the pseudo-docs from your comment into the README seems like a good idea to me!

@adamstankiewicz
Copy link
Member

I'm thinking since the alpha branch is an alpha, it would make sense to update the README to reflect the intended future user flow (while noting things that have not yet been implemented). Something as simple as moving the pseudo-docs from your comment into the README seems like a good idea to me!

@brian-smith-tcril Makes sense to me! I closed out the now-stale README docs PR in favor of this new one instead: #2088

@adamstankiewicz
Copy link
Member

Closing this issue as #2088 merged. Brian, feel free to re-open if you deem it necessary!

@brian-smith-tcril
Copy link
Contributor Author

No need to re-open! #2088 goes above and beyond resolving this!

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 a pull request may close this issue.

2 participants