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.

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

feat: add markdown and markdown/table #1353

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
8 changes: 8 additions & 0 deletions __integration__/__snapshots__/customFormats.test.snap.js
Original file line number Diff line number Diff line change
Expand Up @@ -717,6 +717,10 @@ snapshots["integration custom formats inline custom with new args should match s
"name/camel",
"color/css",
"size/object"
],
"markdown": [
"attribute/cti",
"name/human"
]
},
"transforms": {
Expand Down Expand Up @@ -1671,6 +1675,10 @@ snapshots["integration custom formats register custom format with new args shoul
"name/camel",
"color/css",
"size/object"
],
"markdown": [
"attribute/cti",
"name/human"
]
},
"transforms": {
Expand Down
349 changes: 349 additions & 0 deletions __integration__/__snapshots__/markdown.test.snap.js

Large diffs are not rendered by default.

70 changes: 70 additions & 0 deletions __integration__/markdown.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
/*
* Copyright Target Corporation. All Rights Reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License"). You may not use this file except in compliance with
* the License. A copy of the License is located at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* or in the "license" file accompanying this file. This file is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR
* CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions
* and limitations under the License.
*/
import { expect } from 'chai';
import StyleDictionary from 'style-dictionary';
import { fs } from 'style-dictionary/fs';
import { resolve } from '../lib/resolve.js';
import { buildPath } from './_constants.js';
import { clearOutput } from '../__tests__/__helpers.js';

describe('integration', async () => {
before(async () => {
const sd = new StyleDictionary({
source: [`__integration__/tokens/**/[!_]*.json?(c)`],
platforms: {
markdown: {
transformGroup: `markdown`,
buildPath,
files: [
{
destination: 'StyleDictionary.md',
format: 'markdown/table',
},
{
destination: 'StyleDictionaryWithReferences.md',
format: 'markdown/table',
options: {
outputReferences: true,
},
},
],
},
},
});
await sd.buildAllPlatforms();
});

afterEach(() => {
clearOutput(buildPath);
});

describe('markdown', async () => {
describe(`markdown/table`, async () => {
it(`should match snapshot`, async () => {
const output = fs.readFileSync(resolve(`${buildPath}StyleDictionary.md`), {
encoding: `UTF-8`,
});
await expect(output).to.matchSnapshot();
});

describe(`with references`, async () => {
it(`should match snapshot`, async () => {
const output = fs.readFileSync(resolve(`${buildPath}StyleDictionaryWithReferences.md`), {
encoding: `UTF-8`,
});
await expect(output).to.matchSnapshot();
});
});
});
});
});
25 changes: 25 additions & 0 deletions __tests__/formats/__snapshots__/all.test.snap.js
Original file line number Diff line number Diff line change
Expand Up @@ -1513,3 +1513,28 @@ class {
}`;
/* end snapshot formats all should match flutter/class.dart snapshot with fileHeaderTimestamp set */

snapshots["formats all should match markdown/table snapshot"] =
`
<!--
Do not edit directly, this file was auto-generated.
-->

| Token | Type | Value |
| --- | --- | --- |
| color_red | undefined | \u0060#FF0000\u0060 |
`;
/* end snapshot formats all should match markdown/table snapshot */

snapshots["formats all should match markdown/table snapshot with fileHeaderTimestamp set"] =
`
<!--
Do not edit directly, this file was auto-generated.
Generated on Sat, 01 Jan 2000 00:00:00 GMT
-->

| Token | Type | Value |
| --- | --- | --- |
| color_red | undefined | \u0060#FF0000\u0060 |
`;
/* end snapshot formats all should match markdown/table snapshot with fileHeaderTimestamp set */

1 change: 1 addition & 0 deletions docs/src/content/docs/reference/Hooks/Formats/index.md
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,7 @@ Not all formats use the `outputReferences` option because that file format might
- [compose/object](predefined/#composeobject)
- [ios-swift/class.swift](predefined/#ios-swiftclassswift)
- [flutter/class.dart](predefined/#flutterclassdart)
- [markdown/table](predefined/#markdowntable)

You can create custom formats that output references as well. See the [Custom format with output references](#custom-format-with-output-references) section.

Expand Down
21 changes: 21 additions & 0 deletions docs/src/content/docs/reference/Hooks/Formats/predefined.md
Original file line number Diff line number Diff line change
Expand Up @@ -911,3 +911,24 @@ class StyleDictionary {
```

---

### markdown/table

Creates a Markdown file containing a table with a row for each property.

| Param | Type | Description |
| ------------------------------- | ------------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
| `options` | `Object` | |
| `options.showFileHeader` | `boolean` | Whether or not to include a comment that has the build date. Defaults to `true` |
| `options.showDescriptionColumn` | `boolean` | Whether or not to include the description column in the table. Defaults to `false` |
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggestion, perhaps we should go with an option called columns which is an object of Record<string, boolean>, where folks can turn on/off any column they want, where we set a couple of columns to true by default.

Another suggestion for an option options.columns.order which is an array of string (with a default value) that defines how the columns are ordered from left to right.

I definitely approve of reusing showFileHeader/outputReferences btw, nice one. Especially outputReferences makes a lot of sense here, given that it doesn't suffer from the same caveats that it has in other output formats (like JS/CSS). I would add a note somewhere here in these docs pointing to this section https://styledictionary.com/reference/hooks/formats/#outputreferences-with-transitive-transforms and mention that this caveat fortunately doesn't really apply here 🎉 .

| `options.outputReferences` | `boolean \| OutputReferencesFunction` | Whether or not to keep [references](#references-in-output-files) (a -> b -> c) in the output. Defaults to `false`. Also allows passing a function to conditionally output references on a per token basis. |

Example:

```md title="colors.md"
| Token | Type | Value |
| ----- | ----- | ----------- |
| red 5 | color | `#fffaf3f2` |
```

---
Original file line number Diff line number Diff line change
Expand Up @@ -211,3 +211,12 @@ Transforms:
[name/camel](/reference/hooks/transforms/predefined#namecamel)
[size/object](/reference/hooks/transforms/predefined#sizeobject)
[color/css](/reference/hooks/transforms/predefined#colorcss)

---

### markdown

Transforms:

[attribute/cti](/reference/hooks/transforms/predefined#attributecti)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure we need a transformGroup if the only transforms are attribute/cti (imo not needed, we moved away from CTI as much as possible in v4) and a name transform (user can pick any).

I think what will end up happening for users formatting to markdown, is that they will pick the transformGroup that matches the output format they're going for besides markdown to document the tokens, since they want to match the token names and values that are in the actual code output.

[name/human](/reference/hooks/transforms/predefined#namehuman)
45 changes: 45 additions & 0 deletions lib/common/formats.js
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ import scssMapDeep from './templates/scss/map-deep.template.js';
import scssMapFlat from './templates/scss/map-flat.template.js';
import macrosTemplate from './templates/ios/macros.template.js';
import plistTemplate from './templates/ios/plist.template.js';
import markdownTable from './templates/markdown/table.md.template.js';

/**
* @typedef {import('../../types/Format.ts').Format} Format
Expand Down Expand Up @@ -1532,6 +1533,50 @@ declare const ${moduleName}: ${JSON.stringify(treeWalker(dictionary.tokens), nul
});
return flutterClassDart({ allTokens: sortedTokens, file, options, formatProperty, header });
},

// Markdown templates
/**
* Creates a Markdown file containing a table with a row for each property.
*
* @memberof Formats
* @kind member
* @typedef {Object} markdownTableOpts
* @property {boolean} [markdownTableOpts.showFileHeader=true] - Whether or not to include a comment that has the build date
* @property {boolean} [markdownTableOpts.showDescriptionColumn=false] - Whether or not to show the description column in the table
* @property {OutputReferences} [markdownTableOpts.outputReferences=false] - Whether or not to keep [references](/#/formats?id=references-in-output-files) (a -> b -> c) in the output.
* @param {FormatArgs & { options?: markdownTableOpts }} options
* @example
* ```md
* | Token | Type | Value |
* | --- | --- | --- |
* | red 5 | color | `#fffaf3f2` |
* ```
*/
'markdown/table': async function ({ dictionary, options, file }) {
const { allTokens, tokens, unfilteredTokens } = dictionary;
const { outputReferences, formatting, usesDtcg } = options;
const formatProperty = createPropertyFormatter({
outputReferences,
dictionary,
formatting,
usesDtcg,
});
Comment on lines +1558 to +1563
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this one is redundant here, the createPropertyFormatter is mostly useful as a utility to easily put the key/value pairs into a string property matching one of these formats: css, js, sass, styles, less etc.

In the case of the markdown template, there isn't much overlap between what that does and what this util does.


let sortedTokens;
if (outputReferences) {
sortedTokens = [...allTokens].sort(sortByReference(tokens, { unfilteredTokens }));
} else {
sortedTokens = [...allTokens].sort(sortByName);
}

const header = await fileHeader({
file,
commentStyle: 'xml',
formatting: getFormattingCloneWithoutPrefix(formatting),
options,
});
return markdownTable({ allTokens: sortedTokens, options, formatProperty, header });
},
};

// Mark which formats are nested
Expand Down
30 changes: 30 additions & 0 deletions lib/common/templates/markdown/table.md.template.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
/**
* @typedef {import('../../../../types/DesignToken.ts').TransformedToken} TransformedToken
* @typedef {import('../../../../types/Config.ts').Config} Config
* @typedef {import('../../../../types/Config.ts').LocalOptions} LocalOptions
*/

/**
* @param {{
* allTokens: TransformedToken[]
* formatProperty: (token: TransformedToken) => string
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this is needed here, see other comment about not using createPropertyFormatter

* options: Config & LocalOptions
* header: string
* }} opts
*/
export default ({ allTokens, formatProperty, options, header }) => {

Check warning on line 15 in lib/common/templates/markdown/table.md.template.js

View workflow job for this annotation

GitHub Actions / Verify changes (18, ubuntu-latest)

'formatProperty' is defined but never used

Check warning on line 15 in lib/common/templates/markdown/table.md.template.js

View workflow job for this annotation

GitHub Actions / Verify changes (18, windows-latest)

'formatProperty' is defined but never used

Check warning on line 15 in lib/common/templates/markdown/table.md.template.js

View workflow job for this annotation

GitHub Actions / Verify changes (21.x, ubuntu-latest)

'formatProperty' is defined but never used

Check warning on line 15 in lib/common/templates/markdown/table.md.template.js

View workflow job for this annotation

GitHub Actions / Verify changes (21.x, windows-latest)

'formatProperty' is defined but never used
const hasDescription = options.showDescriptionColumn;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or we could check if any token has at least a description, and drop the showDescriptionColumn option

Something like:

const hasDescription = allTokens.some((token) => token.$description !== undefined || token.comment !== undefined);

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah this thought crossed my mind as well but I think it makes more sense to have the user explicitly choose whether or not they want the column, they can make that decision based on whether any tokens are using a description or not, they'll know whether it's useful as a column or redundant. I am mostly worried that removing the column because there are no descriptions while the user has explicitly configured that there should be a description column may be unexpected/counter-intuitive behaviour for them because that's a bit of invisible magic that goes against their configured thing.


return `
${header}

| Token | ${hasDescription ? 'Description | ' : ''}Type | Value |
| --- | ${hasDescription ? '--- | ' : ''}--- | --- |
${allTokens
.map(
(token) =>
`| ${token.name.replace(/ $/, '')} | ${hasDescription ? (token.$description ? token.$description : token.comment ? token.comment : '') + ' | ' : ''}${token.original.type} | \u0060${options.usesDtcg ? JSON.stringify(token.original.$value) : token.original.value}\u0060 |`,
)
.join('\n')}
`;
Comment on lines +18 to +29
Copy link
Collaborator

@jorenbroekema jorenbroekema Oct 11, 2024

Choose a reason for hiding this comment

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

Disclaimer: the following code adjustment suggestion was written in Github PR WYSIWYG editor so I can't guarantee that it's valid 😅

Suggested change
return `
${header}
| Token | ${hasDescription ? 'Description | ' : ''}Type | Value |
| --- | ${hasDescription ? '--- | ' : ''}--- | --- |
${allTokens
.map(
(token) =>
`| ${token.name.replace(/ $/, '')} | ${hasDescription ? (token.$description ? token.$description : token.comment ? token.comment : '') + ' | ' : ''}${token.original.type} | \u0060${options.usesDtcg ? JSON.stringify(token.original.$value) : token.original.value}\u0060 |`,
)
.join('\n')}
`;
return `${header.length > 0 ? `${header}\n\n` : ''}| Token | ${hasDescription ? 'Description | ' : ''}Type | Value |
| --- | ${hasDescription ? '--- | ' : ''}--- | --- |
${allTokens
.map(
(token) =>
`| ${token.name} | ${hasDescription ?
(
token.$description ?
token.$description
: token.comment ?
token.comment
: ''
) + ' | '
: ''}${options.outputReferences ? : }${options.usesDtcg ? token.$type : token.type} | \`${JSON.stringify(options.outputReferences ?
(
options.usesDtcg ?
token.original.$value
: token.original.value
)
: (
options.usesDtcg ?
token.$value
: token.value
)}\` |`,
)
.join('\n')}
`;

If we were to allow the columns option and granular control over which columns and what order, this format becomes a little bit more complex, code-wise, my code suggestion here is:

  • we should not have leading newline
  • we should not have redundant newlines when there is no file header
  • we should support both DTCG and non DTCG formatted tokens
  • fixed JSON.stringify to apply to both $value and value
  • we should respect whatever token name convention the user configured rather than removing the first space (I'm not sure why it does that tbh)
  • I think we should take the "type" rather than "original.type" unless we know a specific use case where the type gets changed (e.g. by a preprocessor) and we have a good reason to not use the transformed type over the original type. To be honest, I'm not even sure if the original.type gets set before the preprocessors lifecycle 😅 in that case it shouldn't be possible to have a different value for original.type vs type.
  • we only output the original value and not the resolved value if outputReferences is false so I addressed this
  • we should format that nested ternary a bit to make it more readable, or abstract it into a little function, on that note I'm curious if Prettier already released --experimental-nested-ternaries https://prettier.io/blog/2023/11/13/curious-ternaries, maybe with this flag it would auto-format the nested ternary. Feel free to add this flag in this PR if it works :).

For the no fileheader (empty string) use-case, I think we should add a test so we can verify by snapshot no redundant newlines are present. A(n integration) test for DTCG formatted tokens also makes sense imo.

When looking at my code suggestion and the fact that we may make it more complicated with the "columns" option and the ordering, I think I would prefer if we split up the formatting per token in functions (formatToken, formatValue, formatDescription), at least for the value and description this seems important for maintainability/readability. I'd probably abstract the table head part into its own function as well while you're at it.

Lastly, I wrote a comment about an escape markdown special characters regex replace, so we'd probably need to add that here as well to apply to the generated string as a whole, ensuring we don't accidentally produce broken markdown because we don't escape certain characters.

};
10 changes: 10 additions & 0 deletions lib/common/transformGroups.js
Original file line number Diff line number Diff line change
Expand Up @@ -322,4 +322,14 @@ export default {
* @memberof TransformGroups
*/
'react-native': ['name/camel', 'color/css', 'size/object'],

/**
* Transforms:
*
* [attribute/cti](/reference/hooks/transforms/predefined#attributecti)
* [name/human](/reference/hooks/transforms/predefined#namehuman)
*
* @memberof TransformGroups
*/
markdown: ['attribute/cti', 'name/human'],
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as my other comment, I don't think we need a transformGroup, first one seems redundant and the second one makes more sense to me if it matches the user's transformGroup's name transform (e.g. if they output to CSS they'll likely prefer kebab case for their markdown format so it somewhat matches with the tokens in code, they may even put a custom name transform kebab that also adds "--" prefix to be fully matching)

};