feat: add `markdown` and `markdown/table` #1353

julien-deramond · 2024-09-25T16:39:37Z

Warning

This PR is heavily draft.
Feel free to:

close this PR if too heavily draft and that it bothers the repo to have it opened :)
directly modify it if needed

Description

This PR proposes to add a new markdown/table format that would render a table in Markdown format. This would be useful for documentation purposes, as it would allow for a more structured and readable output.

For instance, it would generate the following Markdown:

| Token | Description | Type | Value |
| --- | --- | --- | --- |
| aqua 0 |   | color | `#d9fcfb` |
| aqua 100 |   | color | `#c5f9f9` |
| aqua 200 |   | color | `#a5f2f2` |
| aqua 300 |   | color | `#76e5e2` |
| aqua 400 |   | color | `#33d6e2` |
| aqua 500 |   | color | `#17b8ce` |
| aqua 600 |   | color | `#0797ae` |
| aqua 700 |   | color | `#0b8599` |
| aqua 800 |   | color | `#0f6e84` |
| aqua 900 |   | color | `#035e73` |
| aqua 1000 |   | color | `#083d4f` |
| aqua 1100 |   | color | `#002838` |

that would be rendered this way:

Token	Type	Value
aqua 0	color	`#d9fcfb`
aqua 100	color	`#c5f9f9`
aqua 200	color	`#a5f2f2`
aqua 300	color	`#76e5e2`
aqua 400	color	`#33d6e2`
aqua 500	color	`#17b8ce`
aqua 600	color	`#0797ae`
aqua 700	color	`#0b8599`
aqua 800	color	`#0f6e84`
aqua 900	color	`#035e73`
aqua 1000	color	`#083d4f`
aqua 1100	color	`#002838`

In terms of usage, even if I'm not that familiar with all usages of Style Dictionary, I can imagine that it could be useful to also have a new markdown transform group that would allow rendering the tokens in Markdown format.

Features

`original` property

Mentioned in #1340 (comment):

"original (reads from token.original.value/token.original.$value) → can be left empty if it's the same as $value/value"

TODO: check how to implement it. Render a new dedicated column. If so, automatic or with a property named showOriginalColumn (or something like that).

`description` property

Mentioned in #1340 (comment):

"description (reads from token.$description or token.comment (legacy) property)"

This first version adds a showDescriptionColumn which makes it a voluntary gesture from the user. There could be another approach as described in #1353 (comment) where there's nothing to configure as a user, and the description is automatically displayed if there's at least one description.

We could add some specific unit tests too.

Reference chain (nice to have)

Mentioned in #1340 (comment):

(nice to have) "reference chain, shows the entire chain of references that lead to the resolved value"

TODO: Let's prototype! Maybe optional for this first version?

Value preview (nice to have)

Mentioned in #1340 (comment):

(nice to have) "value preview e.g. for colors putting a small color box div prefix, maybe some other token types can also show value previews"

Unfortunately, Markdown doesn't inherently support colored text (or areas). For example, it's not possible to include colored text in GitHub comments or descriptions, or even READMEs, even when using something like <span style="color:orange;">Word up</span>. However, some Markdown processors do allow for HTML.

However, as mentioned in #1340 (comment), GitHub and GitLab render colors in a specific way when the value is surrounded by backticks. This could be used to render a color preview in the table.

For instance: #ffdd00

Based on this, I've surrounded all the values (not necessarily colors) by backticks in the table. This would render the color values in a colored box in GitHub and GitLab, but not in other Markdown processors. And the other values will also have a code-like appearance. It would avoid complexity and specificities for each type of token.

If we'd like to go further, we might add a new option/property to provide the rendering as a parameter, or to enable a rich preview, where we could use a colored rounded <div> for instance. This would be useful for all Markdown preprocessors that don't display nicely the colors; for instance, the Markdown documentation of GitLab.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

aws-amplify-us-west-1 · 2024-09-25T16:43:24Z

This pull request is automatically being deployed by Amplify Hosting (learn more).

Access this pull request here: https://pr-1353.d16eby4ekpss5y.amplifyapp.com

julien-deramond · 2024-09-30T11:17:10Z

lib/common/templates/markdown/table.md.template.js

+ * }} opts
+ */
+export default ({ allTokens, formatProperty, options, header }) => {
+  const hasDescription = options.showDescriptionColumn;


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

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.

julien-deramond · 2024-09-30T11:24:54Z

Marking it as 'ready for review' to get initial feedback from the core team before proceeding further.

jorenbroekema

Sorry this took a while, I was pretty swamped with some other work so I only just now was able to really take a close look at this and invest time in properly reviewing.

I think we're definitely on the right track, but I do have quite a few suggestions :)

jorenbroekema · 2024-10-11T07:27:01Z

__integration__/__snapshots__/markdown.test.snap.js

+| green 800 | color | \u0060#008b46\u0060 |
+| green 900 | color | \u0060#006b40\u0060 |
+| info | color | \u0060{color.core.blue.0.value}\u0060 |
+| interactive _ | color | \u0060{color.brand.primary.value}\u0060 |


I'm wondering if it might be wise to put in place an escape characters regex for special markdown characters. _ for example is a special character in markdown and even though it doesn't cause issues in this particular instance, it definitely could. So the desired output would probably be \_ to be safe

jorenbroekema · 2024-10-11T07:29:01Z

__integration__/__snapshots__/markdown.test.snap.js

+
+| Token | Type | Value |
+| --- | --- | --- |
+| aqua 0 | color | \u0060#d9fcfb\u0060 |


Is there a particular reason for using the unicode variant of the ` character? I think you could argue using the unicode is a bit less ergonomic since you have to look up the unicode to understand its value.

jorenbroekema · 2024-10-11T07:40:33Z

docs/src/content/docs/reference/Hooks/Formats/predefined.md

+| ------------------------------- | ------------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
+| `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`                                                                                                                         |


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

jorenbroekema · 2024-10-11T07:55:53Z

docs/src/content/docs/reference/Hooks/Transform Groups/predefined.md

+
+Transforms:
+
+[attribute/cti](/reference/hooks/transforms/predefined#attributecti)


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.

jorenbroekema · 2024-10-11T08:01:48Z

lib/common/templates/markdown/table.md.template.js

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


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

jorenbroekema · 2024-10-11T08:03:09Z

lib/common/formats.js

+    const formatProperty = createPropertyFormatter({
+      outputReferences,
+      dictionary,
+      formatting,
+      usesDtcg,
+    });


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.

jorenbroekema · 2024-10-11T08:07:09Z

lib/common/templates/markdown/table.md.template.js

+ * }} opts
+ */
+export default ({ allTokens, formatProperty, options, header }) => {
+  const hasDescription = options.showDescriptionColumn;


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.

jorenbroekema · 2024-10-11T08:36:01Z

lib/common/templates/markdown/table.md.template.js

+  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')}
+`;


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.

jorenbroekema · 2024-10-11T08:38:59Z

lib/common/transformGroups.js

+   *
+   * @memberof TransformGroups
+   */
+  markdown: ['attribute/cti', 'name/human'],


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)

jorenbroekema · 2024-10-11T09:14:22Z

Another note btw, for color values with color preview, this only works for issues, PRs and discussions, https://docs.github.com/en/get-started/writing-on-github/getting-started-with-writing-and-formatting-on-github/basic-writing-and-formatting-syntax#supported-color-models

If we want color value previews that not only work in github markdown elsewhere but also for all markdown renderers, we may have to go with something more agnostic like this: https://stackoverflow.com/questions/11509830/how-to-add-color-to-githubs-readme-md-file (https://placehold.co/)

julien-deramond · 2024-10-11T11:10:34Z

Sorry this took a while, I was pretty swamped with some other work so I only just now was able to really take a close look at this and invest time in properly reviewing.
I think we're definitely on the right track, but I do have quite a few suggestions :)

Thanks a lot for your time and your suggestions. I'll look at them closely to make evolve this PR as soon as possible; also swamped with tome other work, but step-by-step we'll manage to land something :)

julien-deramond force-pushed the main-jd-add-markdown-table branch 3 times, most recently from 10b8d14 to 56216ad Compare September 30, 2024 11:16

julien-deramond commented Sep 30, 2024

View reviewed changes

julien-deramond marked this pull request as ready for review September 30, 2024 11:24

julien-deramond requested a review from a team as a code owner September 30, 2024 11:24

feat: add markdown and markdown/table

ad2b987

julien-deramond force-pushed the main-jd-add-markdown-table branch from 56216ad to ad2b987 Compare October 1, 2024 19:06

Merge branch 'main' into main-jd-add-markdown-table

1afdb4c

jorenbroekema requested changes Oct 11, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add `markdown` and `markdown/table` #1353

feat: add `markdown` and `markdown/table` #1353

julien-deramond commented Sep 25, 2024 •

edited

Loading

aws-amplify-us-west-1 bot commented Sep 25, 2024

julien-deramond Sep 30, 2024

jorenbroekema Oct 11, 2024

julien-deramond commented Sep 30, 2024 •

edited

Loading

jorenbroekema left a comment

jorenbroekema Oct 11, 2024

jorenbroekema Oct 11, 2024

jorenbroekema Oct 11, 2024

jorenbroekema Oct 11, 2024

jorenbroekema Oct 11, 2024

jorenbroekema Oct 11, 2024

jorenbroekema Oct 11, 2024

jorenbroekema Oct 11, 2024 •

edited

Loading

jorenbroekema Oct 11, 2024

jorenbroekema commented Oct 11, 2024

julien-deramond commented Oct 11, 2024


		Transforms:

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

feat: add markdown and markdown/table #1353

Are you sure you want to change the base?

feat: add markdown and markdown/table #1353

Conversation

julien-deramond commented Sep 25, 2024 • edited Loading

Description

Features

original property

description property

Reference chain (nice to have)

Value preview (nice to have)

aws-amplify-us-west-1 bot commented Sep 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julien-deramond commented Sep 30, 2024 • edited Loading

jorenbroekema 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorenbroekema Oct 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorenbroekema commented Oct 11, 2024

julien-deramond commented Oct 11, 2024

feat: add `markdown` and `markdown/table` #1353

feat: add `markdown` and `markdown/table` #1353

julien-deramond commented Sep 25, 2024 •

edited

Loading

`original` property

`description` property

julien-deramond commented Sep 30, 2024 •

edited

Loading

jorenbroekema Oct 11, 2024 •

edited

Loading