-
Notifications
You must be signed in to change notification settings - Fork 567
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
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(); | ||
}); | ||
}); | ||
}); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
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 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
* 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 GitHub Actions / Verify changes (18, ubuntu-latest)
Check warning on line 15 in lib/common/templates/markdown/table.md.template.js GitHub Actions / Verify changes (18, windows-latest)
Check warning on line 15 in lib/common/templates/markdown/table.md.template.js GitHub Actions / Verify changes (21.x, ubuntu-latest)
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const hasDescription = options.showDescriptionColumn; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Something like: const hasDescription = allTokens.some((token) => token.$description !== undefined || token.comment !== undefined); There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
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:
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. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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'], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) |
||
}; |
There was a problem hiding this comment.
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 ofRecord<string, boolean>
, where folks can turn on/off any column they want, where we set a couple of columns totrue
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 🎉 .