feat: add custom variables to utility classes

[andrey-canon]

Description

Use ccs variables in utility-classes.css instead of resolved values.

Issue #2016

Deploy Preview

Include a direct link to your changes in this PR's deploy preview here (e.g., a specific component page).

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]

Thanks for the pull request, @andrey-canon! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

• supporting documentation
• Open edX discussion forum threads
• timeline information ("this must be merged by XX date", and why that is)
• partner information ("this is a course on edx.org")
• any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

[andrey-canon]

@adamstankiewicz

[codecov]

Codecov Report

Patch and project coverage have no change.

Comparison is base (387e3c4) 90.94% compared to head (aac2cea) 90.94%.

Additional details and impacted files

@@           Coverage Diff           @@
##            alpha    #2022   +/-   ##
=======================================
  Coverage   90.94%   90.94%           
=======================================
  Files         214      214           
  Lines        3523     3523           
  Branches      852      852           
=======================================
  Hits         3204     3204           
  Misses        311      311           
  Partials        8        8           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[adamstankiewicz]

@andrey-canon Thanks for the PR and for trying out the alpha release of design tokens in the learning MFE! Overall, I think this approach makes sense to explicitly define tokens for the hover/focus colors for the utilities so they remain theme-able. I left a question for your consideration for now.

[adamstankiewicz]

[curious] Hm, I wonder if it makes sense to have these new tokens explicitly named with focus given they're also being used for hover styles as well? Would it make sense to have a pretty much identical set of tokens for hover specifically? Or maybe there's a more generic name than focus here?

[andrey-canon]

IMO the name should be more generic than focus, however when I wrote that I didn't have more ideas,
probably action is better and when someone needs a specific action that could be added like this

{
  "color": {
    "action":  {
        "general": {...},
        "focus": {...},
        "hover": {...},
....
      }
  }
}

where general would be this case, that is used in focus and hover

[adamstankiewicz]

@andrey-canon I think your proposed approach above is reasonable!

nit: just personal preference, but perhaps default in favor of general? 🤔

Also, I'm wondering if it'd be worthwhile to "namespace" these tokens under a "utilities" property, e.g.:

{
  "color": {
      "utilities": {
          "action":  {
              "default": {...},
              "focus": {...},
              "hover": {...}
          }
      }
  }
}

such that the resulting CSS variables are perhaps more like:

var(--pgn-color-utilities-action-default-*)

[andrey-canon]

@adamstankiewicz I applied in this commit the first approach and this commit contains the utilities approach however I'm not sure about adding the namespace since that makes me feels those variables will only be used in the utility-classes file, right now is true, but probably, those will be used in other places in the future, so I think a general name will fit better

[adamstankiewicz]

@andrey-canon Totally a valid concern! That said, because these tokens are already defined specifically in the utilities folder, it would indicate that they'll only be used relevant to utility classes. If these new tokens are more general purpose than just the utility classes, then perhaps they should be created outside of the utilities folder in favor of tokens/global/color.json?

[andrey-canon]

@adamstankiewicz I agree with moving that to global folder, in that way, the variable scope is not limited

[adamstankiewicz]

+1 sounds good here.

[andrey-canon]

applied in this commit

[adamstankiewicz]

@andrey-canon Oh, worth noting, related to the new tokens you're adding... we recently merged in a PR that added a type attribute to all tokens based on the W3C spec for design tokens. Do you mind adding a type property to your new tokens as well? 😄

[adamstankiewicz]

[question/curious] If in the future we did have utilities-action-hover-* and/or utilities-action-focus-* in the future, how would the hardcoded default here in bgVariant and textEmphasisVariant pull in hover and/or focus instead?

[andrey-canon]

I think that will depends on the implementation, some examples

if a helper/util function is created, that method could replace that line

function getActionVariablebyToken(token, action ="default") {
  const { attributes: { type} } = token;
  return token.name.replace(type, `utilities-action-${action}-${type}`);
}

if current function is modified

function bgVariant(token) {
  const { attributes: { type, item } } = token;
  const parent = `.bg-${type}${item === 'base' ? '' : `-${item}`}`;
  const focus = token.name.replace(type, `utilities-action-focus-${type}`);
  const hover = token.name.replace(type, `utilities-action-hover-${type}`);
  return `${parent} {
  background-color: ${`var(--${token.name})`} !important;
}

a${parent}:hover,
a${parent}:focus,
button${parent}:hover{
  background-color: ${`var(--${hover})`}  !important;
}
button${parent}:focus {
  background-color: ${`var(--${focus})`}  !important;
}

`;
}

[adamstankiewicz]

@andrey-canon Thanks for the example here. I guess my bigger concern here is that we have to hardcode a string here to match the JSON schema exactly rather than being able to infer the token name for the hover/focus color instead. For example, what if the JSON schema decides to switch "action" ➡️ "actions", we'd have to remember to make the identical change here to account for that.

That said, I'm not sure finding a solution to that is worth the lift right now unless you see an obvious way around it; I'd be OK with deferring this particular issue. Thoughts?

[andrey-canon]

@adamstankiewicz I think a reference in the color will be necessary, something like

{
  "color": {
    "primary": {
      "base": { "value": "#0A3055", "type": "color", "source": "$primary" },
      "100": {
        "value": "{color.primary.base}",
        "type": "color",
        "source": "$primary-100",
        "modify": [{ "type": "mix", "otherColor": "white", "amount": "0.94" }]
        "actions": {
          "default": "{color.action.default.primary.base}"
        }
      }
  }
}

and then try to get the action from the token, however, right now, I didn't have the solution I will try to find it out

[andrey-canon]

@adamstankiewicz please check this commit

some experiments:

1. I tried to use the name instead of path, however the value returned was just the last part, example,

color.json
        "actions": {
          "default": "{color.action.default.secondary.100.name}"
        }

result
        "actions": {
          "default": "100"
        }

2. I tried to use a transformer instead of this line

   paragon/tokens/style-dictionary.js

   Line 146 in 2a89430

   token.actions = {"default": `var(--pgn-${token.actions.default.join('-')})`};

however the aliasing didn't work for colors with explicit values

config json

    "dark": {
      "base": {
        "value": "#273F2F",
        "type": "color",
        "source": "$dark",
        "actions": {
          "default": "{color.action.default.dark.base.path}"
        }
      },

transform

        "actions": {
          "default": "{color.action.default.dark.base.path}"
        }

register
        "actions": {
          "default": ["color", "action", "default", "dark", "base"]
        }

if I set value to something like {color.primary.base} the register and transform results are the same

[brian-smith-tcril]

Overall this is looking really great! I left a couple (somewhat nitpicky) comments, but nothing that involves functionality changes or that I'd consider a blocker.

[brian-smith-tcril]

Suggested change

	const { attributes: { type, item }, actions } = token;
	const { attributes: { type, item }, name, actions } = token;

[brian-smith-tcril]

combined with the previous suggestion we could then just have

Suggested change

	background-color: ${`var(--${token.name})`} !important;
	background-color: ${`var(--${name})`} !important;

[brian-smith-tcril]

same as previous comment about name from bgVariant

[brian-smith-tcril]

same as previous comment about name from bgVariant

[andrey-canon]

Overall this is looking really great! I left a couple (somewhat nitpicky) comments, but nothing that involves functionality changes or that I'd consider a blocker.

Thanks for the suggestion, it was already addressed in the last commit

[adamstankiewicz]

@andrey-canon [inform] We just merged #2069, which refactors the design tokens into core and themes/light folders to help future proof the JSON files to support additional theme variants in the future (e.g., dark mode). Unfortunately, these changes are likely to cause conflicts on this PR.

Also, I'm a bit concerned that we're relying on computed properties like .path in the token source files as opposed to just the token alias itself. I pulled down your branch and plan to poke around more in depth later this afternoon to try to see if there's any alternative ways of thinking about this approach.

[andrey-canon]

@adamstankiewicz I have already resolved all conflicts and also I have made some extra changes to avoid using path, please check last commit a let me know your thoughts about this approach

[e0d]

@adamstankiewicz and @brian-smith-tcril as @andrey-canon is a new contributor to this repository, actions must be approved for the tests to run. If you see the actions awaiting approval, and the code is safe, please approve to reduce wait time for feedback.

[mphilbrick211]

Hi @adamstankiewicz! Could you please review and merge if this is ready-to-go? Thanks!

[mphilbrick211]

Hi @andrey-canon! Some branch conflicts have popped up. Would you mind taking a look? Thanks!

CC: @adamstankiewicz for review/merge.

[viktorrusakov]

LGTM!

[adamstankiewicz]

@andrey-canon Thanks for your patience in getting this re-reviewed/merged! Appreciate your contribution 😄

[openedx-webhooks]

@andrey-canon 🎉 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]

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

The release is available on:

• npm package (@alpha dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[edx-semantic-release]

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

The release is available on:

• npm package (@alpha dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[openedx-semantic-release-bot]

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

The release is available on:

• npm package (@alpha dist-tag)
• GitHub release

Your semantic-release bot 📦🚀