feat: add custom variables to utility classes #2022

andrey-canon · 2023-02-14T16:44:04Z

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 · 2023-02-14T16:44:10Z

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 · 2023-02-14T16:45:46Z

@adamstankiewicz

codecov · 2023-02-14T17:49:47Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.94%. Comparing base (387e3c4) to head (aac2cea).
Report is 983 commits behind head on alpha.

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

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

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 · 2023-02-15T14:59:08Z

tokens/src/utilities/color.json

+  ],
+  "color": {
+    "focus": {


[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?

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

@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-*)

@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

@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?

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

+1 sounds good here.

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 · 2023-02-15T21:52:30Z

tokens/css-utilities.js

  const parent = `.bg-${type}${item === 'base' ? '' : `-${item}`}`;
+  const action = token.name.replace(type, `utilities-action-default-${type}`);


[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?

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; } `; }

@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?

@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

@adamstankiewicz please check this commit

some experiments:

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" }

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 · 2023-02-23T16:43:19Z

tokens/css-utilities.js

 /**
 * Implements bg-variant mixin from bootstrap. Creates utility classes for background colors based on theme color.
 */
 function bgVariant(token) {
-  const { attributes: { type, item }, value } = token;
+  const { attributes: { type, item }, actions } = token;


Suggested change

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

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

brian-smith-tcril · 2023-02-23T16:43:54Z

tokens/css-utilities.js

  const parent = `.bg-${type}${item === 'base' ? '' : `-${item}`}`;
  return `${parent} {
-  background-color: ${value} !important;
+  background-color: ${`var(--${token.name})`} !important;


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 · 2023-02-23T16:47:08Z

tokens/css-utilities.js

  const parent = `.text-${type}${item === 'base' ? '' : `-${item}`}`;
  return `${parent} {
-  color: ${value} !important;
+  color: ${`var(--${token.name})`} !important;


same as previous comment about name from bgVariant

brian-smith-tcril · 2023-02-23T16:47:25Z

tokens/css-utilities.js

  const className = `.border-${type}${item === 'base' ? '' : `-${item}`}`;
  return `${className} {
-  border-color: ${value} !important;
+  border-color: ${`var(--${token.name})`} !important;


same as previous comment about name from bgVariant

andrey-canon · 2023-02-23T17:18:28Z

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 · 2023-02-24T15:08:45Z

@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 · 2023-03-01T20:14:54Z

@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 · 2023-03-10T20:03:23Z

@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 · 2023-03-23T21:09:56Z

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

mphilbrick211 · 2023-04-24T19:41:51Z

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

CC: @adamstankiewicz for review/merge.

viktorrusakov

LGTM!

adamstankiewicz · 2023-04-26T13:34:26Z

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

openedx-webhooks · 2023-04-26T13:34:39Z

@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 · 2023-04-26T13:50:15Z

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

The release is available on:

Your semantic-release bot 📦🚀

edx-semantic-release · 2023-09-01T12:56:35Z

🎉 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:52:34Z

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

The release is available on:

Your semantic-release bot 📦🚀

openedx-semantic-release-bot · 2024-12-12T02:38:53Z

🎉 This PR is included in version 23.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Feb 14, 2023

mphilbrick211 requested a review from adamstankiewicz February 14, 2023 18:58

adamstankiewicz reviewed Feb 15, 2023

View reviewed changes

andrey-canon force-pushed the and/utility-css-variables branch from d149386 to e1ea4bd Compare February 15, 2023 21:45

adamstankiewicz reviewed Feb 15, 2023

View reviewed changes

adamstankiewicz force-pushed the alpha branch 4 times, most recently from 4b127b4 to d6a761d Compare February 17, 2023 19:53

andrey-canon requested a review from adamstankiewicz February 20, 2023 17:45

adamstankiewicz force-pushed the alpha branch 3 times, most recently from 626f43e to 79f0a71 Compare February 20, 2023 18:42

andrey-canon force-pushed the and/utility-css-variables branch from 2a89430 to a756575 Compare February 21, 2023 15:41

adamstankiewicz force-pushed the alpha branch from 6254fba to 9184d80 Compare February 21, 2023 21:08

andrey-canon force-pushed the and/utility-css-variables branch from a756575 to 4a567b1 Compare February 23, 2023 00:19

brian-smith-tcril reviewed Feb 23, 2023

View reviewed changes

andrey-canon force-pushed the and/utility-css-variables branch from fa0910a to 5d7c354 Compare March 1, 2023 20:12

mphilbrick211 added the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Mar 10, 2023

e0d removed the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Mar 13, 2023

andrey-canon force-pushed the and/utility-css-variables branch from 5d7c354 to 6e47d3a Compare March 27, 2023 18:19

andrey-canon added 9 commits April 24, 2023 17:58

feat: add custom variables to utility classes

53f76f9

feat: define default action tokens instead of using focus

cb25b15

feat: add utilities namespace

ae32d9a

feat: move actions to global colors

c278959

feat: add actions field to colors

8bbef0d

fix: lint issues

98a53c1

refactor: destructuring name token

eabf07e

feat: get token by reference a use name instead of using path attribute

adcc7c4

feat: rebase and resolve conflicts

aac2cea

andrey-canon force-pushed the and/utility-css-variables branch from cd24bf1 to aac2cea Compare April 24, 2023 23:01

mphilbrick211 added the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Apr 25, 2023

viktorrusakov approved these changes Apr 26, 2023

View reviewed changes

adamstankiewicz approved these changes Apr 26, 2023

View reviewed changes

adamstankiewicz merged commit f5f4921 into openedx:alpha Apr 26, 2023

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

e0d removed the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Apr 27, 2023

viktorrusakov linked an issue Apr 27, 2023 that may be closed by this pull request

[discovery] Ensure generated CSS utility classes from style-dictionary use CSS variables instead of resolved values #2016

Closed

1 task

monteri pushed a commit to raccoongang/paragon that referenced this pull request Aug 17, 2023

feat: add custom variables to utility classes (openedx#2022)

2bb135f

PKulkoRaccoonGang pushed a commit to raccoongang/paragon that referenced this pull request Aug 18, 2023

feat: add custom variables to utility classes (openedx#2022)

a83f996

PKulkoRaccoonGang pushed a commit that referenced this pull request Aug 1, 2024

feat: add custom variables to utility classes (#2022)

04f4d55

PKulkoRaccoonGang pushed a commit that referenced this pull request Aug 4, 2024

feat: add custom variables to utility classes (#2022)

053d3bc

PKulkoRaccoonGang pushed a commit that referenced this pull request Aug 5, 2024

feat: add custom variables to utility classes (#2022)

1b02505

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

		const parent = `.bg-${type}${item === 'base' ? '' : `-${item}`}`;
		const action = token.name.replace(type, `utilities-action-default-${type}`);

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

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

feat: add custom variables to utility classes #2022

feat: add custom variables to utility classes #2022

Conversation

andrey-canon commented Feb 14, 2023

Description

Deploy Preview

Merge Checklist

Post-merge Checklist

openedx-webhooks commented Feb 14, 2023

andrey-canon commented Feb 14, 2023

codecov bot commented Feb 14, 2023 • edited Loading

Codecov Report

adamstankiewicz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamstankiewicz Feb 15, 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

adamstankiewicz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrey-canon Feb 15, 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

brian-smith-tcril left a 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

andrey-canon commented Feb 23, 2023

adamstankiewicz commented Feb 24, 2023 • edited Loading

andrey-canon commented Mar 1, 2023

e0d commented Mar 10, 2023

mphilbrick211 commented Mar 23, 2023

mphilbrick211 commented Apr 24, 2023

viktorrusakov left a comment

Choose a reason for hiding this comment

adamstankiewicz commented Apr 26, 2023

openedx-webhooks commented Apr 26, 2023

edx-semantic-release commented Apr 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

codecov bot commented Feb 14, 2023 •

edited

Loading

adamstankiewicz Feb 15, 2023 •

edited

Loading

andrey-canon Feb 15, 2023 •

edited

Loading

adamstankiewicz commented Feb 24, 2023 •

edited

Loading