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.

Sign up for GitHub

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

Jump to bottom

feat: expose --output-references CLI arg for building tokens, registers filters, and updates CSS vars format #3203

Merged
merged 1 commit into from
Sep 3, 2024
Merged

feat: expose --output-references CLI arg for building tokens, registers filters, and updates CSS vars format #3203

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
.PHONY: build
build:
rm -rf ./dist
tsc --project tsconfig.build.json
Expand Down
10 changes: 10 additions & 0 deletions bin/paragon-scripts.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,11 +85,21 @@ const COMMANDS = {
description: 'Include only source design tokens in the build.',
defaultValue: false,
},
{
name: '--output-references',
description: 'Include references in the build output.',
defaultValue: true,
},
Comment on lines +88 to +92
Copy link
Member Author

Choose a reason for hiding this comment

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

[inform] Currently, running build-tokens with the CLI for a brand package (e.g., @edx/elm-theme), the CSS build output does not include the references, i.e. CSS variables, instead hardcoding the raw value.

I believe we want to default to always including references, even for brand packages, to mitigate risk that updating a reference for a style property, won't change the actual style due to the missing use of the CSS variable.

However, I think it's also reasonable to not want this in some cases so having it be an option seems reasonable.

Copy link
Contributor

Choose a reason for hiding this comment

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

100% agree. Good call/addition!

{
name: '-t, --themes',
description: 'Specify themes to include in the token build.',
defaultValue: 'light',
},
{
name: '-v, --verbose',
description: 'Enable verbose logging.',
defaultValue: false,
Copy link
Member Author

Choose a reason for hiding this comment

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

[inform] Improves the utility of warning/error messages resulting from style-dictionary during build-tokens.

},
],
},
'replace-variables': {
Expand Down
81 changes: 55 additions & 26 deletions lib/build-tokens.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
const path = require('path');
const minimist = require('minimist');
const { initializeStyleDictionary, createCustomCSSVariables, colorTransform } = require('../tokens/style-dictionary');
const {
initializeStyleDictionary,
colorTransform,
} = require('../tokens/style-dictionary');
const { createIndexCssFile } = require('../tokens/utils');

/**
Expand All @@ -13,24 +16,37 @@ const { createIndexCssFile } = require('../tokens/utils');
* @param {string|string[]} [commandArgs.themes=['light']] - The themes (variants) for which to build tokens.
*/
async function buildTokensCommand(commandArgs) {
const StyleDictionary = await initializeStyleDictionary();

const defaultParams = {
themes: ['light'],
'build-dir': './build/',
'source-tokens-only': false,
'output-references': true,
verbose: false,
};

const alias = {
'build-dir': 'b',
themes: 't',
verbose: '-v',
};

const {
'build-dir': buildDir,
source: tokensSource,
'source-tokens-only': hasSourceTokensOnly,
'output-references': outputReferences,
themes,
} = minimist(commandArgs, { alias, default: defaultParams, boolean: 'source-tokens-only' });
verbose,
} = minimist(
commandArgs,
{
alias,
default: defaultParams,
boolean: ['source-tokens-only', 'output-references', 'verbose'],
},
);

const StyleDictionary = await initializeStyleDictionary({ themes });

const coreConfig = {
include: [
Expand All @@ -43,33 +59,35 @@ async function buildTokensCommand(commandArgs) {
platforms: {
css: {
prefix: 'pgn',
transformGroup: 'css',
// NOTE: buildPath must end with a slash
buildPath: buildDir.slice(-1) === '/' ? buildDir : `${buildDir}/`,
options: {
fileHeader: 'customFileHeader',
},
files: [
{
format: 'css/custom-variables',
destination: 'core/variables.css',
filter: hasSourceTokensOnly ? 'isSource' : undefined,
options: {
outputReferences: !hasSourceTokensOnly,
outputReferences,
Copy link
Member Author

Choose a reason for hiding this comment

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

[inform] Now relies on the CLI option; outputReferences is now distinct from hasSourceTokensOnly.

},
},
{
format: 'css/custom-media-breakpoints',
destination: 'core/custom-media-breakpoints.css',
filter: hasSourceTokensOnly ? 'isSource' : undefined,
options: {
outputReferences: !hasSourceTokensOnly,
outputReferences,
},
},
],
transforms: StyleDictionary.hooks.transformGroups.css.filter(item => item !== 'size/rem').concat('color/sass-color-functions', 'str-replace'),
options: {
fileHeader: 'customFileHeader',
},
},
},
log: {
verbosity: verbose ? 'verbose' : 'default',
},
};

const getStyleDictionaryConfig = (themeVariant) => ({
Expand All @@ -91,45 +109,56 @@ async function buildTokensCommand(commandArgs) {
transform: (token) => colorTransform(token, themeVariant),
},
},
format: {
'css/custom-variables': formatterArgs => createCustomCSSVariables({
formatterArgs,
themeVariant,
}),
},
platforms: {
css: {
...coreConfig.platforms.css,
files: [
{
format: 'css/custom-variables',
destination: `themes/${themeVariant}/variables.css`,
filter: hasSourceTokensOnly ? 'isSource' : undefined,
filter: hasSourceTokensOnly
? `isSource.${themeVariant}`
: `isThemeVariant.${themeVariant}`,
Comment on lines +119 to +121
Copy link
Member Author

Choose a reason for hiding this comment

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

[inform] Currently, the registered format css/custom-variables performs filtering within its own logic (i.e., filtering on dictionary.allTokens), rather than treating the filter as a legit style-dictionary filter here. New filters are now registered to filter tokens by the appropriate themeVariant file path within the filter property here versus within the definition of css/custom-variables.

Note that other custom filters (e.g., isSource) can automatically opt into having sub-filters created specific to individual theme variants (e.g., isSource.light).

Copy link
Contributor

Choose a reason for hiding this comment

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

Just trying to wrap my head around this change a little bit. Before this change if hasSourceTokensOnly was false we'd have filter as undefined - does that mean it used to apply to all variants?

Copy link
Member Author

Choose a reason for hiding this comment

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

Previously, hasSourceTokensOnly: false would rely on filtering implemented within the format function, instead of as a true style-dictionary filter implemented with .registerFilter. That is, formatters should assume any token filtering happens before the format is applied, which already limits the contents of dictionary.allTokens we previously filtered on explicitly.

There's some more detailed rationale about this change provided on my review of Peter's PR here, notably:

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).

See the current implementation of createCustomCSSVariables (source).

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the added context! 100% agree that it's better to have

a true style-dictionary filter implemented with .registerFilter

than

filtering implemented within the format function

options: {
outputReferences: !hasSourceTokensOnly,
outputReferences,
},
},
{
format: 'css/utility-classes',
destination: `themes/${themeVariant}/utility-classes.css`,
filter: hasSourceTokensOnly ? 'isSource' : undefined,
options: {
outputReferences: !hasSourceTokensOnly,
outputReferences,
},
},
],
},
},
});

new StyleDictionary(coreConfig).buildAllPlatforms();
createIndexCssFile({ buildDir, isTheme: false });
// Create list of style-dictionary configurations to build (core + theme variants)
const configs = [
{ config: coreConfig },
...themes.map((themeVariant) => {
const config = getStyleDictionaryConfig(themeVariant);
return {
config,
themeVariant,
};
}),
];

themes.forEach(async (themeVariant) => {
const config = getStyleDictionaryConfig(themeVariant);
new StyleDictionary(config).buildAllPlatforms();
createIndexCssFile({ buildDir, isTheme: true, themeVariant });
});
// Build tokens for each configuration
await Promise.all(configs.map(async ({ config, isThemeVariant, themeVariant }) => {
const sd = new StyleDictionary(config);
await sd.cleanAllPlatforms();
await sd.buildAllPlatforms();
createIndexCssFile({
buildDir,
isThemeVariant: !!isThemeVariant,
themeVariant,
});
}));
}

module.exports = buildTokensCommand;
8 changes: 4 additions & 4 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 3 additions & 2 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,8 @@
"prepare": "husky || true",
"build-tokens": "./bin/paragon-scripts.js build-tokens --build-dir ./styles/css",
"replace-variables-usage-with-css": "./bin/paragon-scripts.js replace-variables -p src -t usage",
"replace-variables-definition-with-css": "./bin/paragon-scripts.js replace-variables -p src -t definition"
"replace-variables-definition-with-css": "./bin/paragon-scripts.js replace-variables -p src -t definition",
"cli:help": "./bin/paragon-scripts.js help"
Copy link
Member Author

Choose a reason for hiding this comment

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

[inform] Adds a way to run the CLI help command from Paragon root.

},
"dependencies": {
"@popperjs/core": "^2.11.4",
Expand Down Expand Up @@ -114,7 +115,7 @@
"@babel/preset-typescript": "^7.16.7",
"@edx/eslint-config": "^3.2.0",
"@edx/stylelint-config-edx": "^2.3.0",
"@edx/typescript-config": "^1.0.1",
"@edx/typescript-config": "^1.1.0",
adamstankiewicz marked this conversation as resolved.
Show resolved Hide resolved
"@formatjs/cli": "^5.0.2",
"@semantic-release/changelog": "^6.0.1",
"@semantic-release/git": "^10.0.1",
Expand Down
2 changes: 1 addition & 1 deletion src/Form/_FormText.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
.pgn__form-text {
font-size: var(--pgn-typography-font-size-small-base);
font-size: var(--pgn-typography-font-size-sm);
Copy link
Member Author

Choose a reason for hiding this comment

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

The token representing .small was refactored slightly to be named pgn-typography-font-size-sm.

Copy link
Contributor

Choose a reason for hiding this comment

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

var(--pgn-typography-font-size-sm)

We will also need to update the changed design tokens in the edx/elm-theme and ensure that in the consuming MFEs that have already switched to the design tokens in the test format, all tokens will be relevant.
Since we use only native CSS variables, consumers will not see an error/warning about a missing token. Such minor changes are unfortunately easy to miss.

I have described this issue before, maybe we should come back to it in the future. Please let me know your thoughts.

Copy link
Member Author

Choose a reason for hiding this comment

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

@PKulkoRaccoonGang I have a draft PR brewing to account for edx/elm-theme, based on the changes introduced in this PR and the broader v3 -> v4 upgrade (e.g., adopting the DTCG format).

Note: this linked PR also accounts for a few style bug fixes as well that I noticed while QA'ing elm-theme against the Paragon docs site locally.

I have described #3057 before, maybe we should come back to it in the future. Please let me know your thoughts.

100% agreed we should explore opportunities for linting and/or other validation of tokens, but as a future initiative.

display: flex;
align-items: center;

Expand Down
4 changes: 2 additions & 2 deletions src/PageBanner/index.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,12 @@
min-height: 36px;
display: flex;
flex-wrap: nowrap;
font-size: var(--pgn-typography-font-size-small-x);
font-size: var(--pgn-typography-font-size-xs);
Copy link
Member Author

Choose a reason for hiding this comment

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

The token representing .x-small was refactored slightly to be named pgn-typography-font-size-xs.

background-color: var(--pgn-page-baner-bg, inherit);
color: var(--pgn-page-baner-color, inherit);

@include media-breakpoint-up(md) {
font-size: var(--pgn-typography-font-size-small-base);
font-size: var(--pgn-typography-font-size-sm);
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/ProgressBar/index.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,6 @@
.pgn__progress-hint {
box-sizing: border-box;
padding: 0 var(--pgn-spacing-progress-bar-hint-annotation-gap);
font-size: var(--pgn-typography-font-size-small-base);
font-size: var(--pgn-typography-font-size-sm);
}
}
2 changes: 1 addition & 1 deletion src/Stepper/index.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@
}

.pgn__stepper-header-step-description {
font-size: var(--pgn-typography-font-size-small-x);
font-size: var(--pgn-typography-font-size-xs);
}

&.pgn__stepper-header-step-active ~ .pgn__stepper-header-step {
Expand Down
2 changes: 1 addition & 1 deletion src/Toast/index.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@
padding: 0;

p {
font-size: var(--pgn-typography-font-size-small-base);
font-size: var(--pgn-typography-font-size-sm);
margin: 0;
padding-right: .75rem;
}
Expand Down
9 changes: 4 additions & 5 deletions styles/css/core/custom-media-breakpoints.css
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@
/*
* IMPORTANT: This file is the result of assembling design tokens.
* Do not edit directly.
* Generated on Tue, 27 Aug 2024 17:14:44 GMT
/**
* Do not edit directly, this file was auto-generated. while transforming design tokens.
* See <root>/tokens/README.md for more details.
*/

@custom-media --pgn-size-breakpoint-min-width-xs (min-width: 0rem);
@custom-media --pgn-size-breakpoint-min-width-xs (min-width: 0);
@custom-media --pgn-size-breakpoint-max-width-xs (max-width: 576px);
@custom-media --pgn-size-breakpoint-min-width-sm (min-width: 576px);
@custom-media --pgn-size-breakpoint-max-width-sm (max-width: 768px);
Expand Down
Loading
Loading