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

Conversation

fredvisser
Copy link
Contributor

Pull Request

🤨 Rationale

We received feedback that it's difficult to find icons by name, and that it's particularly difficult to look up icons using common metaphors. This PR replaces the Storybook autodocs for the Icon story with a generated MDX file that includes icon, icon name, and icon synonyms.

👩‍💻 Implementation

Modified the generate-icon.js source to generate a Storybook docs icons.mdx file (in addition to the Nimble components and the all-icons barrel file).

image

Alternates considered

Initial put the icons in a IconGallery DocBlock, but it lacked flexibility with long icons names, and including synonyms made it look even worse. (We are still using the IconItem component to format the icon)

🧪 Testing

Manual Storybook review

✅ Checklist

  • I have updated the project documentation to reflect my changes or determined no changes are needed.

| Icon | Synonyms |
| ---- | -------- |
`;

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

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

@@ -70,10 +90,18 @@ export const ${tagName} = DesignSystem.tagFor(${className});
fileCount += 1;

allIconsFileContents = allIconsFileContents.concat(`export { ${className} } from './${fileName}';\n`);
mdxFileContents = mdxFileContents.concat(`${mdxImportStatements}\n`);
mdxTableFileContents = mdxTableFileContents.concat(`${mdxIconTableRow}\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}`;

*/
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)

@rajsite rajsite marked this pull request as draft June 14, 2023 23:24
@fredvisser fredvisser closed this Sep 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants