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

Replace Storybook icon docs with icon table (including metadata) #1260

Closed
wants to merge 7 commits into from
Closed
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be a good topic for our next call with our Chromatic rep. If you agree, it'd be good to save off some links/screenshots/chromatic builds/notes to show why we didn't end up using the icon gallery doc block.

"type": "none",
"comment": "Replace icon docs with generated icon table",
"packageName": "@ni/nimble-components",
"email": "1458528+fredvisser@users.noreply.github.com",
"dependentChangeType": "none"
}
35 changes: 33 additions & 2 deletions packages/nimble-components/build/generate-icons/source/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,12 @@
* Build script for generating Nimble components for icons.
*
* Iterates through icons provided by nimble-tokens, and generates a Nimble component for each in
* src/icons. Also generates an all-icons barrel file.
* src/icons. Also generates an all-icons barrel file and the icons.mdx Storybook file.
*/
import { pascalCase, spinalCase } from '@microsoft/fast-web-utilities';
import * as icons from '@ni/nimble-tokens/dist/icons/js';
// eslint-disable-next-line import/no-extraneous-dependencies
import { iconMetadata } from '@ni/nimble-components/dist/esm/icon-base/icon-metadata';
Copy link
Member

Choose a reason for hiding this comment

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

There is a chicken and egg problem here. The components build needs the icons to be generated before typescript runs to create the dist/esm output. The icons need the dist/esm output to generate the story.

Some options:

  • Have a separate script that runs after components build but before storybook build to generate the icon story
  • Can we just make an icons story in mdx that loops over all the available icons? I'd expect we can do something like that in the React mdx syntax

Copy link
Member

Choose a reason for hiding this comment

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

Maybe something like this comment could work? storybookjs/storybook#8392 (comment)


const fs = require('fs');
const path = require('path');
Expand All @@ -18,6 +20,18 @@ const trimSizeFromName = text => {
const generatedFilePrefix = `// AUTO-GENERATED FILE - DO NOT EDIT DIRECTLY
// See generation source in nimble-components/build/generate-icons\n`;

const mdxFilePrefix = `import { Meta, Title, IconItem } from '@storybook/blocks';
import * as iconStories from '../../icon-base/tests/icons.stories';

<Meta of={iconStories} />
<Title of={iconStories} />`;

let mdxTable = `| Icon | Synonyms |
| ---- | -------- |
`;

let mdxImportStatements = '';

const iconsDirectory = path.resolve(__dirname, '../../../src/icons');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I put the icons.mdx file in the top-level icons directory, instead of a tests directory. Since the whole folder is autogenerated, maybe that's fine?

Copy link
Member

Choose a reason for hiding this comment

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

The files generated should be treated as source code. I.e. could turn off the generation and check them in. So it's fine to be in icons but should go under icons/tests.

Reason behind the reason is files in any folder named tests/ get excluded from the final package automatically, so lean into that convention.

Copy link
Member

@rajsite rajsite May 22, 2023

Choose a reason for hiding this comment

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

Can see Malcolm's script for setting up the directory structure:

const destAbsoluteDir = path.resolve(__dirname, '../dist/');


if (fs.existsSync(iconsDirectory)) {
Expand All @@ -27,11 +41,13 @@ if (fs.existsSync(iconsDirectory)) {
}
console.log(`Creating icons directory "${iconsDirectory}"`);
fs.mkdirSync(iconsDirectory);
fs.mkdirSync(path.resolve(iconsDirectory, 'tests'));
console.log('Finished creating icons directory');

console.log('Writing icon component files');
let allIconsFileContents = `${generatedFilePrefix}\n`;
let fileCount = 0;

for (const key of Object.keys(icons)) {
const svgName = key; // e.g. "arrowExpanderLeft16X16"
const iconName = trimSizeFromName(key); // e.g. "arrowExpanderLeft"
Expand All @@ -41,6 +57,8 @@ for (const key of Object.keys(icons)) {
const className = `Icon${pascalCase(iconName)}`; // e.g. "IconArrowExpanderLeft"
const tagName = `icon${pascalCase(iconName)}Tag`; // e.g. "iconArrowExpanderLeftTag"

const iconSynonyms = iconMetadata[className];

const componentFileContents = `${generatedFilePrefix}
import { ${svgName} } from '@ni/nimble-tokens/dist/icons/js';
import { DesignSystem } from '@microsoft/fast-foundation';
Expand Down Expand Up @@ -69,11 +87,24 @@ export const ${tagName} = DesignSystem.tagFor(${className});
fs.writeFileSync(filePath, componentFileContents, { encoding: 'utf-8' });
fileCount += 1;

allIconsFileContents = allIconsFileContents.concat(`export { ${className} } from './${fileName}';\n`);
allIconsFileContents += `export { ${className} } from './${fileName}';\n`;

mdxImportStatements += `import { ${tagName} } from '../${fileName}';\n`;
mdxTable += `| <IconItem name="${fileName}"><${elementName} /></IconItem> | ${iconSynonyms?.tags.join(', ')} |\n`;
}
console.log(`Finshed writing ${fileCount} icon component files`);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be easier to follow the output document structure if we kept all the MDX stuff together inside the loop and concatenated the parts of the output file all at once after the loop. So something more like this:

let mdxImportStatements = '';
let mdxTable = mdxIconTablePrefix;
for (const key of Object.keys(icons)) {
    // ...
    mdxImportStatements += `import { ${tagName} } from './${fileName}';\n`;
    mdxTable += `| <IconItem name="${fileName}"><${elementName} /></IconItem> | ${iconSynonyms?.tags.join(', ')} |\n`;
    // ...
}

const mdxFileContents = `
${mdxFilePrefix}
${mdxImportStatements}
${mdxTable}`;


const allIconsFilePath = path.resolve(iconsDirectory, 'all-icons.ts');
console.log('Writing all-icons file');
fs.writeFileSync(allIconsFilePath, allIconsFileContents, { encoding: 'utf-8' });
console.log('Finished writing all-icons file');

const mdxFileContents = `
${mdxFilePrefix}\n
${mdxImportStatements}
${mdxTable}`;

const iconsMDXFilePath = path.resolve(iconsDirectory, 'tests/icons.mdx');
console.log('Writing icons.mdx file');
fs.writeFileSync(iconsMDXFilePath, mdxFileContents, { encoding: 'utf-8' });
console.log('Finished writing icons.mdx file');
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,7 @@ interface IconArgs {
}

const metadata: Meta<IconArgs> = {
title: 'Icons',
tags: ['autodocs']
title: 'Icons'
};

export default metadata;
Expand Down
1 change: 1 addition & 0 deletions packages/site/landing/index.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import '@ni/nimble-components/dist/esm/theme-provider/design-tokens';
import '@ni/nimble-components/dist/esm/icon-base/icon-metadata';

import { encircle } from './circle';

Expand Down