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

Implement EDS Theming tooling #1738

Merged
merged 7 commits into from
Sep 7, 2023
Merged

Implement EDS Theming tooling #1738

merged 7 commits into from
Sep 7, 2023

Conversation

booc0mtaco
Copy link
Contributor

@booc0mtaco booc0mtaco commented Aug 31, 2023

Summary:

This adds some utility scripts to EDS to support setting up, applying, and updating token definitions, using style-dictionary. Further enhancements will be to support reading outputs from figma into a format EDS understands, and flagging theming errors where possible. Final phase will include support for diffing against EDS updates, so that newly-added tokens are available, or deprecated tokens are flagged.

This will also include usage instructions in-repo, as this is developer tooling.

  • init script to set up theming in a project
  • apply script to convert JSON into CSS tokens to read in code
  • updated config to package the correct files (bin/, theme files, etc.)
  • instructions on usage (how to set up config, options, gotchas, etc.)
  • FAQ location in README
  • Remove TODOs, and test integrations (config to theme EDS, etc.)

Test Plan:

  • Wrote automated tests
  • CI tests / new tests are not applicable
  • Manually tested my changes, but I want to keep the details secret
  • Manually tested my changes, and here are the details:

@booc0mtaco booc0mtaco force-pushed the aholloway/eds-themnig branch from 1b6508d to e00137b Compare August 31, 2023 21:59
@github-actions
Copy link

github-actions bot commented Aug 31, 2023

size-limit report 📦

Path Size
components 95.29 KB (0%)
styles 32.77 KB (0%)

@codecov
Copy link

codecov bot commented Aug 31, 2023

Codecov Report

Merging #1738 (cf176f1) into next (2b0855e) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             next    #1738   +/-   ##
=======================================
  Coverage   92.28%   92.28%           
=======================================
  Files         146      146           
  Lines        2580     2580           
  Branches      665      665           
=======================================
  Hits         2381     2381           
  Misses        183      183           
  Partials       16       16           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

📢 Have feedback on the report? Share it here.

@booc0mtaco booc0mtaco force-pushed the aholloway/eds-themnig branch from e00137b to ffb888f Compare September 1, 2023 14:01
@booc0mtaco
Copy link
Contributor Author

yarn.lock Outdated Show resolved Hide resolved
@booc0mtaco booc0mtaco force-pushed the aholloway/eds-themnig branch 8 times, most recently from 6c19d71 to 334ad8f Compare September 5, 2023 23:15
@booc0mtaco
Copy link
Contributor Author

booc0mtaco commented Sep 6, 2023

Created alpha version to test out on a few projects chanzuckerberg/[email protected]

@booc0mtaco
Copy link
Contributor Author

Two tools generate expected outputs! will add in some additional checks for reading config, then post for review

Copy link
Contributor

@timzchang timzchang left a comment

Choose a reason for hiding this comment

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

This makes sense to me! A couple questions and a nit in the readme only

} catch (e) {
console.error('EDS package not installed. Using local path...');
packageRootPath =
path.dirname(require.main.path) + '/src/tokens-dist/json/';
Copy link
Contributor

Choose a reason for hiding this comment

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

what do we expect to be at this directory if eds is not installed?

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 little bit is essentially for testing WITHIN EDS. I tested most of the functionality by dog-fooding this in the repo. I can update the error message to be info or something, and make this say something closer to that

if (config) {
try {
fs.copyFileSync(
packageRootPath + 'theme-base.json',
Copy link
Contributor

Choose a reason for hiding this comment

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

where is theme-base created?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

theme-base.json is generated along side the other style-dictionary output files (e.g., variables.css, et al), and gets shipped with EDS. For consumers it will live somewhere in node_modules/@chanzuckerberg/eds, and this line will match that location

Each released version of EDS will include an updated version of this file for two uses:

  • new projects will start off with a fresh copy that includes all the available tokens
  • existing projects can do a diff against their copy and this one to add/remove tokens that changes from EDS version X to Y

That second thing is another reason we have people editing JSON, as it is MUCH easier to build tooling around diffing JSON object as opposed to CSS files or some other content.


`json` determines where the core theme file will be stored, and `css` determines where the resulting css token file will be stored.

### eds-init-theme
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: can we name the files generated by eds-init-theme and specify which one to modify?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍🏾! added in file names in appropriate spots and better sequencing in the doc.s

@booc0mtaco booc0mtaco marked this pull request as ready for review September 7, 2023 16:24
@booc0mtaco booc0mtaco requested a review from a team September 7, 2023 16:24
@booc0mtaco
Copy link
Contributor Author

booc0mtaco commented Sep 7, 2023

FYI going to merge this one as a merge commit instead of a squash to preserve the tag and commit for the alpha I tested with. We will have 1-2 follow up alpha releases to test TS interop and make sure rollup still provides something that works with consumer projects

EDIT: i forgot, rebased, and now the hashes changed, so that tag is in a detached HEAD state. which is fine. I'll just do a squash and merge like normal :)

Tickets: [EDS-996, EDS-746, EDS-747, EDS-749]
- handle initializing token override file from base
- handle configuration for local paths to use
- handle comparison to base in package for checksum
- handle updates to themes when base has changed
- update naming for consistency
- add in better ordering of commands and exception handling
- sync changes to style-dictionary
- add documentation for using EDS commands
- update README to link to new documentation
@booc0mtaco booc0mtaco force-pushed the aholloway/eds-themnig branch from 663a8ab to cf176f1 Compare September 7, 2023 18:29
@booc0mtaco booc0mtaco merged commit 91497bf into next Sep 7, 2023
@booc0mtaco booc0mtaco deleted the aholloway/eds-themnig branch September 7, 2023 18:56
@booc0mtaco booc0mtaco mentioned this pull request Sep 7, 2023
booc0mtaco added a commit that referenced this pull request Sep 7, 2023
## [13.2.0](v13.1.1...v13.2.0) (2023-09-07)


### Features

* **Avatar:** add in expanded size range ([#1734](#1734)) ([7af6e5e](7af6e5e))
* **tokens:** add tooling for EDS theming ([#1738](#1738)) ([91497bf](91497bf))


### Bug Fixes

* opt our rollup CJS build into TS's module interop behavior ([#1747](#1747)) ([2b0855e](2b0855e))
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