feat: updating to Style Dictionary v4

[PKulkoRaccoonGang]

Description

This PR is an attempt to update the Style Dictionary v3 library to Style Dictionary v4 (including refactoring the design tokens to the DTCG format).

Merge Checklist

• If your update includes visual changes, have they been reviewed by a designer? Send them a link to the Netlify deploy preview, if applicable.
• Does your change adhere to the documented style conventions?
• Do any prop types have missing descriptions in the Props API tables in the documentation site (check deploy preview)?
• Consider whether this change needs to reviewed/QA'ed for accessibility (a11y). If so, please request an a11y review for the PR in the #wg-paragon Open edX Slack channel.
• Were your changes tested using all available themes (see theme switcher in the header of the deploy preview, under the "Settings" icon)?
• Were your changes tested in the example app?
• Is there adequate test coverage for your changes?

Post-merge Checklist

• Verify your changes were released to NPM at the expected version.
• If you'd like, share your contribution in #show-and-tell.
• 🎉 🙌 Celebrate! Thanks for your contribution.

[openedx-webhooks]

Thanks for the pull request, @PKulkoRaccoonGang!

What's next?

Please work through the following steps to get your changes ready for engineering review:

🔘 Get product approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

• If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
  • This process (including the steps you'll need to take) is documented here.
• If it doesn't, simply proceed with the next step.

🔘 Provide context

To help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:

• Dependencies

  This PR must be merged before / after / at the same time as ...

• Blockers

  This PR is waiting for OEP-1234 to be accepted.

• Timeline information

  This PR must be merged by XX date because ...

• Partner information

  This is for a course on edx.org.

• Supporting documentation
• Relevant Open edX discussion forum threads

🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

🔘 Let us know that your PR is ready for review:

Who will review my changes?

This repository is currently maintained by @openedx/paragon-working-group. Tag them in a comment and let them know that your changes are ready for review.

Where can I find more information?

If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:

• Overview of Review Process for Community Contributions
• Pull Request Status Guide
• Making changes to your pull request

When can I expect my changes to be merged?

Our goal is to get community contributions seen and reviewed as efficiently as possible.

However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:

• The size and impact of the changes that it introduces
• The need for product review
• Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

[netlify]

✅ Deploy Preview for paragon-openedx ready!

Name	Link
🔨 Latest commit	a0baaa6
🔍 Latest deploy log	https://app.netlify.com/sites/paragon-openedx/deploys/66e0778299f4630008191c09
😎 Deploy Preview	https://deploy-preview-3186--paragon-openedx.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[PKulkoRaccoonGang]

TODO

• correct display of the header of generated files
• checking the correctness of variable generation (for some variables you can notice an extra indication of the unit of measurement, for example 0rem)
  ...

ES6 modules
Since the updated Style Dictionary v4 API is based on ES6 modules, an open question is finding a solution for the correct operation of Style Dictionary v4 together with the existing CommonJS functionality of Gatsby. According to the Gatsby documentation, support for ES6 modules is implemented in more recent releases of the framework. Most likely, for the current version of Gatsby (4.23.1), a potential solution could be to use dynamically imports or update the Gatsby version.

[brian-smith-tcril]

Thank you so much for putting this together! Considering the task of "do some discovery around v4" this is truly above and beyond!

checking the correctness of variable generation (for some variables you can notice an extra indication of the unit of measurement, for example 0rem)

I think it's reasonable to update the .json files to include units where we don't yet have them as a solution here.

Since the updated Style Dictionary v4 API is based on ES6 modules, an open question is finding a solution for the correct operation of Style Dictionary v4 together with the existing CommonJS functionality of Gatsby. According to the Gatsby documentation, support for ES6 modules is implemented in more recent releases of the framework. Most likely, for the current version of Gatsby (4.23.1), a potential solution could be to use asynchronous imports or update the Gatsby version.

Could you elaborate on this a bit? I worry that moving from Gatsby v4 to v5 would be quite a heavy lift, and I wonder how that compares to some workarounds people have tried gatsbyjs/gatsby#31599 (comment)

[brian-smith-tcril]

Just adding a note based on the discussion in the Paragon WG meeting today:

The migration guidelines have a few suggestions for how to handle this - one that seemed to be a good candidate was

dynamically import Style Dictionary into your CommonJS files const StyleDictionary = (await import('style-dictionary')).default;

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.76%. Comparing base (0e16dbb) to head (a0baaa6).

Additional details and impacted files

@@            Coverage Diff             @@
##            alpha    #3186      +/-   ##
==========================================
- Coverage   93.76%   93.76%   -0.01%     
==========================================
  Files         229      229              
  Lines        3787     3784       -3     
  Branches      908      902       -6     
==========================================
- Hits         3551     3548       -3     
  Misses        229      229              
  Partials        7        7              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[adamstankiewicz]

[curious] Can you clarify these changes a bit? Why are they needed?

[PKulkoRaccoonGang]

I needed to ignore the pre-commit hook)
This is a temporary change that I will remove 💯

[adamstankiewicz]

[clarification] Is having the already quoted font names now wrapped with single quotes, too, intended/needed/work as expected?

[PKulkoRaccoonGang]

[inform] Implemented in follow-up PR: #3207

[adamstankiewicz]

[clarification] Will all tokens defined for ActionRow be of "$type": "dimension"? Should this be associated more directly to the dimension-specific tokens, even though these are the only tokens in this file right now?

For example, if/when a non-dimension token is added to this file, is there a risk this $type won't be semantically correct/accurate anymore?

Similar question like applies to other components' tokens, too.

[PKulkoRaccoonGang]

[inform] Implemented in follow-up PR: #3207

[adamstankiewicz]

[curious] Have we looked into whether we can avoid including CSS-specific syntax calc in the token itself, or how such tokens would be transformed outside of CSS variables?

For example, if there is arithmetic like subtraction as in this line, could the token $value be {size.card.border.radius.base} - {size.card.border.width} and have some sort of transform for the CSS variables output to wrap it with calc(...)?

That way, tokens like these would be more portable to other non-CSS based platforms like mobile.

[adamstankiewicz]

Dug into this a bit. It appears this still remains a complex problem within the style-dictionary community. There is support for a resolveMath transform, from tokens-studio/sd-transforms; however, when the token outputs references via outputReferences: true | fn, the transformed/resolved math is replaced with the original reference (i.e., a CSS variable with var(--foo: 5px) in this case).

I attempted to craft a solution for tokens-studio/sd-transforms#203, but came to the same realization recently described here:

The big issue is that outputting refs happens on the format lifecycle and wrapping values in calc() statements usually happens in the transform lifecycle that happens before. So even if you wrap the values in calc() in transform, the format part will just undo that work by outputting refs by using the original value.

Either the calc wrapping needs to happen on the format level or the outputting refs util needs to act on the transformed value somehow (keeping in mind the bugs that this used to cause in v3 and not regressing on this again)

There are a couple of open issues to rethink how references are resolved and how values with references in them are transformed for a future v5 version of Style Dictionary, but this topic is rather complex and this issue won't be fixed until we have solutions to that broader topic.

tl;dr; Handling the wrapping of math expressions with the CSS calc syntax remains an open question, and likely will not land in v4 of Style Dictionary per the above. Given this, it probably makes sense to keep the calc syntax for now, but when we begin transforming to non-CSS platforms (e.g., JavaScript, iOS, Android), having calc in the token value itself will present an issue. A possible workaround for if/when this becomes an issue might be to introduce custom transforms for the (future) non-CSS platforms that strips the wrapping calc(...) from the underlying math operations used in the token value.

[PKulkoRaccoonGang]

This is definitely a very interesting question that we haven't thought about yet. Thanks for your research, now we know more!

A possible workaround for if/when this becomes an issue might be to introduce custom transforms for the (future) non-CSS platforms that strips the wrapping calc(...) from the underlying math operations used in the token value.

It seemed to me earlier that when adding mobile platforms, Style Dictionary would generate the necessary variables taking into account the various CSS tools (calc) that we use for design tokens, converting them into alternatives acceptable for mobile platforms.

I also consulted with Android and iOS developers from Raccoon Gang, they confirmed that there is no alternative to calc CSS. There are tools to do this in the languages ​​of mobile platforms, but it is desirable that Paragon design tokens provide static (integer) values ​​for variables.

I agree with you, most likely in the future (if there are no new Style Dictionary updates) we will have to make additional modifiers for mobile platforms.

Android

<resources>
    <dimen name="size_card_border_radius_base">16dp</dimen>
    <dimen name="size_card_border_width">2dp</dimen>
</resources>

iOS

<plist version="1.0">
<dict>
    <key>size_card_border_radius_base</key>
    <string>16</string>
    <key>size_card_border_width</key>
    <string>2</string>
</dict>
</plist>

Resources

• https://blogs.halodoc.io/simplifying-android-apps-design-with-design-tokens
• https://source.android.com/docs/automotive/unbundled_apps/design-tokens

[adamstankiewicz]

[clarification] Similar to other comment about the dimension type defined more globally in a file, will all tokens in this file be related to text-decoration? It would seem perhaps the $type should be defined within the decoration sub-object?

[PKulkoRaccoonGang]

[inform] Implemented in follow-up PR: #3207

[adamstankiewicz]

[clarification] only the theme-interval token specifically in this file is a percentage, right?

[PKulkoRaccoonGang]

[inform] Implemented in follow-up PR: #3207

[adamstankiewicz]

[suggestion] could the "$type": "dimension", be moved up under size so it applies to all size permutations (e.g., base vs. lg vs. sm)?

[PKulkoRaccoonGang]

[inform] Implemented in follow-up PR: #3203

[adamstankiewicz]

I wonder if it'd make sense to abstract a helper util function to make these utils accessible, e.g.

/* eslint-disable import/no-unresolved */
const getStyleDictionaryUtils = async () => import('style-dictionary/utils');
/* eslint-enabled import/no-unresolved */

// createCustomCSSVariables
const { sortByReference, usesReferences, getReferences } = await getStyleDictionaryUtils();

getStyleDictionaryUtils could also be used within initializeStyleDictionary, e.g.:

const initializeStyleDictionary = async () => {
  // eslint-disable-next-line import/no-unresolved
  const StyleDictionary = (await import('style-dictionary')).default;
  const { getReferences } = await getStyleDictionaryUtils();
 
  // ...
}

[adamstankiewicz]

[inform] Implemented in follow-up PR: #3203

[adamstankiewicz]

[suggestion/consideration] FWIW, I don't think we get much value out of defining our own customFileHeader given the default fileHeader has an option to opt-in to including the generated timestamp.

IMHO, we don't necessarily need "IMPORTANT: This file is the result of assembling design tokens." explicitly , as this is largely assumed.

If we rely on the default fileHeader with the opt-in timestamp, it'd result in the following, which should be suitable and results in slightly simpler implementation / style-dictionary config:

/**
 * Do not edit directly, this file was auto-generated.
 * Generated on Sat, 31 Aug 2024 07:11:29 GMT
 */

// formatting opts in build-tokens.js

{
  format: 'css/custom-variables',
  destination: 'core/variables.css',
  filter: hasSourceTokensOnly ? 'isSource' : undefined,
  options: {
    outputReferences: !hasSourceTokensOnly,
    formatting: {
      fileHeaderTimestamp: true, // ensures timestamp is included in the `fileHeader` output for this format
    },
  },
},

// usage within `createCustomCSSVariables`

const { dictionary, options, file } = formatterArgs;
const { outputReferences, formatting } = options;
const { fileHeader } = await getStyleDictionaryUtils();
const header = await fileHeader({ file, formatting });

Even if we do register a custom file header, we would still want to rely on the fileHeader function here, since the provided function would render the custom file header on its own.

[adamstankiewicz]

[inform] Implemented in follow-up PR: #3203

[adamstankiewicz]

[curious/suggestion] I think it might be worth considering relying on the out-of-the-box formattedVariables format helper from style-dictionary/utils (docs) to remove the need to construct the CSS variables ourselves 🤔

It'd also have the benefit of automatically adding inline comments to the output files for each CSS variable that has an associated description in token definition, e.g.:

--pgn-transition-base: all .2s ease-in-out; /* Generic transition for any property change */

Something we'd have to be mindful of here, though, is that we're doing filtering of tokens within createCustomCSSVariables when creating the outputTokens variable:

dictionary.allTokens.filter(token => token.filePath.includes(themeVariant))

Because the format helper formattedVariables accepts a dictionary (containing: allTokens, tokens, unfilteredTokens, etc.), I'm not sure we could get away by filtering allTokens ourselves here.

IMHO, having unfilteredTokens in the dictionary here suggests a possible code smell that we might want to be doing such filtering as a legit custom filter registered with style-dictionary (similar to how we seem to define an isSource filter today via registerFilter).

Would it be possible to have real style-dictionary filters to do the above filtering on token.filePath.includes(themeVariant) such that this function is already pre-filtered and the entire dictionary is filtered, not just allTokens?

On an initial look through documentation, it doesn't seem style-dictionary can pass more than one custom filter function at a time within build-tokens, though so I'm not entirely sure yet what the best approach would be to combine a new theme variant filter with the existing isSource filter, when both should be applied.

E.g.,

// existing
StyleDictionary.registerFilter({
  name: 'isSource',
  filter: token => token.isSource,
});

// proposed within `initializeStyleDictionary`
StyleDictionary.registerFilter({
  name: 'isSource',
  filter: token => token.isSource,
});

themes.forEach((themeVariant) => {
  const capitalizedThemeVariant = `${themeVariant.charAt(0).toUpperCase()}${themeVariant.slice(1)}`;
  const themeVariantFilter = `isThemeVariant${capitalizedThemeVariant}`;
  const themeVariantFilterSourceOnly = `${themeVariantFilter}SourceOnly`;

  StyleDictionary.registerFilter({
    name: themeVariantFilter,
    filter: token => token.filePath.includes(themeVariant),
  });

  StyleDictionary.registerFilter({
    name: themeVariantFilterSourceOnly,
    filter: token => token.isSource && token.filePath.includes(themeVariant),
  });
});

// example usage within `build-tokens`'s `getStyleDictionaryConfig`

{
  platforms: {
    css: {
      files: [
        {
          format: 'css/custom-variables',
          destination: `themes/${themeVariant}/variables.css`,
          filter: hasSourceTokensOnly ? themeVariantFilterSourceOnly : themeVariantFilter,
          options: {
            outputReferences: !hasSourceTokensOnly,
          },
        },
      ],
    },
  },
}

[adamstankiewicz]

Related, upon further investigation, it looks we do rely on token-specific handling of outputReferences. For example, we opt-out of including references even when the formatter has them opted-in (e.g., for the case of always including HEX values in SVG url values).

That said, token-specific outputReferences is still supported with the out-of-the-box formattedVariables, by passing outputReferences a function instead of a boolean, e.g.:

const variables = formattedVariables({
  format: 'css',
  dictionary,
  outputReferences: (token) => {
    // Formatter options configured to never output references
    if (!outputReferences) {
      return false;
    }
    // Token has modifications (e.g., mix, darken, lighten); the computed value should be output instead of the base reference
    if (token.modify) {
      return false;
    }
    // Formatter options configured to show output references, but handle individual token might opt-out
    return token.outputReferences ?? true;
  },
  usesDtcg: true,
});

[adamstankiewicz]

[inform] Implemented in follow-up PR: #3203

[adamstankiewicz]

[clarification] Is the comma in "1,5" and other spacing tokens intended, or should it be a period (i.e., "1.5")?

[PKulkoRaccoonGang]

It doesn't matter for the final result. But I agree with you, I replaced the comma with a period (by analogy to the value in the description)

[inform] Implemented in follow-up PR: #3207

[adamstankiewicz]

Looks like we might have lost the transformGroup: 'css', which I think we want to keep so it does these default transforms provided by style-dictionary?

Looks like adding it back in and running build-tokens only results in a diff that adds the rem unit to 0 values in a handful of places, e.g. 0 -> 0rem.

[PKulkoRaccoonGang]

[inform] Implemented in follow-up PR: #3219

[adamstankiewicz]

D'oh, I think in my commit I lost the calc(...) that was previously wrapping this token's $value (this was the only token I experimented with). When previewing the docs site locally, the .lead utility class isn't working properly anymore, noticed first on the home page. Adding calc(...) back in will resolve.

[context] This was removed while exploring the sd-transforms transform for ts/resolveMath to see if we could abstract away calc, but as discussed in another comment, there is not currently a great solution for this, so we need to keep calc in here, at least for now :)

[PKulkoRaccoonGang]

[inform] Implemented in follow-up PR: #3222

[adamstankiewicz]

[suggestion] I believe this 87.5% is equivalent to the {typography.font.size.sm} if we wanted to treat this token an alias to a semantic token (i.e., if/when {typography.font.size.sm} changes, this token will also change).

[adamstankiewicz]

Comparing the computed typography values in the docs for this PR's local checkout vs. published docs site, it looks like we might actually want/need to keep the % scale these tokens originally had.

For example, x-small used to be 75% but we migrated it to .75rem, which seems to result in slightly different font sizes. Note the .small and .x-small examples specifically (left: local; right: prod):

[aside] Related, you might notice the line heights are also slightly off for some of them as well. This seems to be due to the switch to DataTable, where some of the cell content is inheriting DataTable's line-height styles, i.e. .pgn__data-table td (left: local; right: prod):

I suppose we could consider overriding DataTable's default CSS styles for typography example tables by unset'ing the line-height related definition(s), or maybe cross-check whether the CSS utility classes are defining explicit line-height style properties vs. inheriting from its parent.

[PKulkoRaccoonGang]

Yes, we will definitely need to do a full check of all tokens after the latest changes, as we can easily miss some small detail that can have a significant impact on both the components and the documentation site. Thanks! 👍

[adamstankiewicz]

On a local checkout of the docs site from this branch, it looks like the migrate of typography example tables to use DataTable is inheriting its overflow: hidden on table cells, cutting off the text of "Display 4" on smaller screen sizes. Probably not a big deal, but figured I'd flag it as it could be perceived as a style bug potentially.

[adamstankiewicz]

[inform, docs site bug] The "Links" section on the typography foundations page is incorrectly showing text-decoration: underline; on "standalone links" (left: local; right: prod):

It appears this is due to the migration of the example tables to DataTable, where the TextCell is wrapping the cell content with a p, which according to the table description, "For links inside a p...", the standalone links are actually getting treated as inline links even without the explicit .inline-link class.