[BD-46] feat: refactoring design tokens folders structure #2069

PKulkoRaccoonGang · 2023-02-21T22:51:20Z

Description

Ensure the global, alias, and component tokens defined in ./tokens/src/core/ are not specific to a particular theme variant (i.e., light or dark).
Ensure the global, alias, and component tokens defined in ./tokens/src/themes/light/ are only related to style properties relevant to a light theme variant (most likely color tokens).

Issue: #2017

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)?
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?
Consider whether this change needs to reviewed/QA'ed for accessibility (a11y). If so, please add wittjeff and adamstankiewicz as reviewers on this PR.

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 · 2023-02-21T22:51:30Z

Thanks for the pull request, @PKulkoRaccoonGang!

When this pull request is ready, tag your edX technical lead.

codecov · 2023-02-21T23:04:01Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.77%. Comparing base (5ea28ea) to head (6054ca0).
Report is 1122 commits behind head on alpha.

Additional details and impacted files

@@           Coverage Diff           @@
##            alpha    #2069   +/-   ##
=======================================
  Coverage   90.77%   90.77%           
=======================================
  Files         213      213           
  Lines        3501     3501           
  Branches      843      843           
=======================================
  Hits         3178     3178           
  Misses        315      315           
  Partials        8        8

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

adamstankiewicz

Looking good! I left a few comments; biggest feedback is that it looks like the new token files may be missing type attributes (and possibly other things?) that were in the original tokens.

adamstankiewicz · 2023-02-24T02:31:19Z

tokens/sass-helpers.js

@@ -14,7 +14,7 @@ const chroma = require('chroma-js');
 * @return chroma-js color instance (one of dark or light variants)
 */
 function colorYiq(color, light, dark, threshold) {
-  const defaultsFile = fs.readFileSync(path.resolve(__dirname, 'src', 'global', 'other.json'), 'utf8');
+  const defaultsFile = fs.readFileSync(path.resolve(__dirname, 'src', 'themes/light/global', 'other.json'), 'utf8');


[curious] Is everything in the other.json file specific to the light theme variant? Or are some of the properties, possibly like theme-interval, and yiq-contrasted-threshold.

yes, theme-interval and yiq-contrasted-threshold are not theme-specific, moved the to the core tokens

adamstankiewicz · 2023-02-24T02:32:48Z

tokens/sass-helpers.js

@@ -14,7 +14,7 @@ const chroma = require('chroma-js');
 * @return chroma-js color instance (one of dark or light variants)
 */
 function colorYiq(color, light, dark, threshold) {
-  const defaultsFile = fs.readFileSync(path.resolve(__dirname, 'src', 'global', 'other.json'), 'utf8');
+  const defaultsFile = fs.readFileSync(path.resolve(__dirname, 'src', 'themes/light/global', 'other.json'), 'utf8');


[question] If/when we have a dark theme variant, I wonder if there is a way we might be able to future proof this colorYiq helper function to support multiple theme variants later on (e.g., pulling from themes/dark/global/other.json to generate a dark.css output)?

The approach we're opting to take here is to have a light.css and dark.css output file, which would be generated by running the StyleDictionary.buildAllPlatforms() twice: once with the light theme variant tokens, and again with the dark theme variant tokens. If there's a way to pass in the current theme variant being used into the colorYiq that might be one approach to take here. Open to other ideas of course!

Makes sense, I've modified our functions to check whether token belongs to any theme and take theme's colors instead (defaults to light)

adamstankiewicz · 2023-02-24T02:42:52Z

tokens/src/themes/light/components/Annotation.json

+{
+  "elevation": {
+    "annotation": {
+      "box-shadow": {


It looks like the type attribute in these tokens might have gotten lost somehow (in the original, now-deleted Annotation.json, the type attributes are there. I wonder if it may be related to the issues we ran into trying to get alpha brought in sync with master that caused us to essentially rewrite the git history for the alpha branch... It may be worth a check to see if there might be anything else missing in addition to the type? It does look like this may be an issue in many of the files.

yeah, great catch! we were missing a lot of them... I've added all the missed types, I don't think we're missing anything else

…e in color-yiq function

adamstankiewicz · 2023-02-24T12:57:41Z

tokens/style-dictionary.js

+          // find whether token belongs to any theme based on its location
+          // split full path by '/', check if 'themes' directory is a part of the path, if it is - the next nested
+          // directory is the theme name, otherwise use 'light' theme
+          const pathParts = token.filePath.split('/');
+          const themePartIndex = pathParts.findIndex(item => item === 'themes');
+          const themeVariant = themePartIndex === -1 ? 'light' : pathParts[themePartIndex + 1];


This approach seems reasonable. I'm not sure of an alternative way to pass a theme variant into a custom style-dictionary transform to avoid needing to parse file paths.

For example, if build-tokens.js did something like...

const getStyleDictionaryConfig = (themeVariant) => { const { buildDir, source: tokensSource } = program.opts(); const source = tokensSource ? [tokensSource] : []; // Could we pass `themeVariant` arg into the config/transform somehow? const config = { include: [path.resolve(__dirname, 'src/**/*.json')], // ... }; return config; }; const THEME_VARIANTS = ['light']; THEME_VARIANTS.forEach((themeVariant) => { const config = getStyleDictionaryConfig(themeVariant); StyleDictionary.extend(config).buildAllPlatforms(); });

But, I think what you have works just as well for now :)

Hmm, I think we can do something like this, but it will be more complicated. Currently we register our color transform outside of build-tokens.js file and there is no way to let it know about the theme (style dictionary only passes a token to the transform func, so it does not know anything about style dictionary config).

We could try to dynamically change the transform function in build-tokens.js file, something like this

const THEME_VARIANTS = ['light']; THEME_VARIANTS.forEach((themeVariant) => { const config = getStyleDictionaryConfig(themeVariant); StyleDictionary.registerTransform({ name: 'color/sass-color-functions', transitive: true, type: 'value', matcher(token) { return token.attributes.category === 'color' || token.value?.toString().startsWith('#'); }, transformer: (token) => colorTransform(token, themeVariant) // pass themeVariant to transform function, }); StyleDictionary.extend(config).buildAllPlatforms(); });

Hopefully re-registering transform doesn't break anything 😅

actually, nevermind... I just saw that you could define transforms directly in the config object without using registerTransform method, so we could just use that

Ah, yeah, that should theoretically also work! I defer to you if we want to roll with what you have now in this PR to get this PR merged and treat that refactor as a fast-follow or if we want to try to get that change into this PR before merge.

We will tackle this as a fast follow in #2014

adamstankiewicz · 2023-02-24T13:00:47Z

tokens/sass-helpers.js

+ * @param {String} [themeVariant] - theme variant name that will be used to find default contrast colors
+ * @param {String} [light] - light color variant, defaults to 'yiq-text-light'
+ * from ./src/themes/{themeVariant}/other.json
+ * @param {String} [dark] - dark color variant, defaults to 'yiq-text-dark'
+ * from ./src/themes/{themeVariant}/other.json


nit: ./src/themes/{themeVariant}/global/other.json

adamstankiewicz · 2023-02-24T13:04:28Z

tokens/sass-helpers.js

+ * @param {String} [light] - light color variant, defaults to 'yiq-text-light'
+ * from ./src/themes/{themeVariant}/other.json
+ * @param {String} [dark] - dark color variant, defaults to 'yiq-text-dark'
+ * from ./src/themes/{themeVariant}/other.json


[clarification] I don't think the theme variants are necessarily defaulting to yiq-text-light or yiq-text-dark depending on the theme variant itself. That choice depends on the color being modified (yiq-ified? 😅).

Should the JSDoc comment here just reference that the light theme pulls both yiq light/dark values from the other.json file in light or dark folder, respectively, without an explicit mention of yiq-text-light or yiq-text-dark here?

openedx-webhooks · 2023-02-24T14:57:30Z

@PKulkoRaccoonGang 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

edx-semantic-release · 2023-02-26T16:48:39Z

🎉 This PR is included in version 21.0.0-alpha.15 🎉

The release is available on:

Your semantic-release bot 📦🚀

* feat: refactoring design tokens folders structure * fix: add missing "type" attribute to tokens and consider token's theme in color-yiq function * docs: update docstring for colorYiq function --------- Co-authored-by: Viktor Rusakov <[email protected]>

edx-semantic-release · 2023-09-01T12:58:05Z

🎉 This PR is included in version 22.0.0-alpha.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

openedx-semantic-release-bot · 2024-05-14T15:54:10Z

🎉 This PR is included in version 23.0.0-alpha.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

* feat: refactoring design tokens folders structure * fix: add missing "type" attribute to tokens and consider token's theme in color-yiq function * docs: update docstring for colorYiq function --------- Co-authored-by: Viktor Rusakov <[email protected]>

openedx-semantic-release-bot · 2024-12-12T02:40:29Z

🎉 This PR is included in version 23.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

feat: refactoring design tokens folders structure

b63e7e6

openedx-webhooks added the blended PR is managed through 2U's blended developmnt program label Feb 21, 2023

PKulkoRaccoonGang mentioned this pull request Feb 21, 2023

Refactor tokens file/folder structure to support additional theme variants #2017

Closed

2 tasks

PKulkoRaccoonGang requested a review from viktorrusakov February 21, 2023 23:04

adamstankiewicz linked an issue Feb 24, 2023 that may be closed by this pull request

Refactor tokens file/folder structure to support additional theme variants #2017

Closed

2 tasks

adamstankiewicz reviewed Feb 24, 2023

View reviewed changes

fix: add missing "type" attribute to tokens and consider token's them…

2be7656

…e in color-yiq function

adamstankiewicz reviewed Feb 24, 2023

View reviewed changes

docs: update docstring for colorYiq function

6054ca0

adamstankiewicz approved these changes Feb 24, 2023

View reviewed changes

adamstankiewicz merged commit a42ea75 into openedx:alpha Feb 24, 2023

adamstankiewicz mentioned this pull request Feb 24, 2023

feat: add custom variables to utility classes #2022

Merged

10 tasks

edx-semantic-release added the released on @alpha label Feb 26, 2023

openedx-semantic-release-bot added the released label Dec 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BD-46] feat: refactoring design tokens folders structure #2069

[BD-46] feat: refactoring design tokens folders structure #2069

PKulkoRaccoonGang commented Feb 21, 2023 •

edited

Loading

openedx-webhooks commented Feb 21, 2023

codecov bot commented Feb 21, 2023 •

edited

Loading

adamstankiewicz left a comment •

edited

Loading

adamstankiewicz Feb 24, 2023

viktorrusakov Feb 24, 2023

adamstankiewicz Feb 24, 2023

viktorrusakov Feb 24, 2023 •

edited

Loading

adamstankiewicz Feb 24, 2023

viktorrusakov Feb 24, 2023

adamstankiewicz Feb 24, 2023

viktorrusakov Feb 24, 2023

viktorrusakov Feb 24, 2023

adamstankiewicz Feb 24, 2023

adamstankiewicz Feb 24, 2023

adamstankiewicz Feb 24, 2023

adamstankiewicz Feb 24, 2023 •

edited

Loading

openedx-webhooks commented Feb 24, 2023

edx-semantic-release commented Feb 26, 2023

edx-semantic-release commented Sep 1, 2023

openedx-semantic-release-bot commented May 14, 2024

openedx-semantic-release-bot commented Dec 12, 2024

[BD-46] feat: refactoring design tokens folders structure #2069

[BD-46] feat: refactoring design tokens folders structure #2069

Conversation

PKulkoRaccoonGang commented Feb 21, 2023 • edited Loading

Description

Merge Checklist

Post-merge Checklist

openedx-webhooks commented Feb 21, 2023

codecov bot commented Feb 21, 2023 • edited Loading

Codecov Report

adamstankiewicz left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

viktorrusakov Feb 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamstankiewicz Feb 24, 2023 • edited Loading

Choose a reason for hiding this comment

openedx-webhooks commented Feb 24, 2023

edx-semantic-release commented Feb 26, 2023

edx-semantic-release commented Sep 1, 2023

openedx-semantic-release-bot commented May 14, 2024

openedx-semantic-release-bot commented Dec 12, 2024

PKulkoRaccoonGang commented Feb 21, 2023 •

edited

Loading

codecov bot commented Feb 21, 2023 •

edited

Loading

adamstankiewicz left a comment •

edited

Loading

viktorrusakov Feb 24, 2023 •

edited

Loading

adamstankiewicz Feb 24, 2023 •

edited

Loading