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

Fix Storybook #707

Merged
merged 24 commits into from
Mar 7, 2019
Merged

Fix Storybook #707

merged 24 commits into from
Mar 7, 2019

Conversation

shawnbot
Copy link
Contributor

@shawnbot shawnbot commented Mar 6, 2019

TL;DR: This gets Storybook working again and closes #690, which I introduced while reorganizing the repo for v12.

  • I upgraded Storybook to 5.0.0 and installed the viewport addon with Primer breakpoints (small, medium, large, and x-large). You can change the viewport with the last icon along the top edge of the preview panel:

    image

  • Instead of importing code-blocks and handling all the markdown parsing in the bundle, the logic that reads markdown and generates stories is now a webpack "loader". This shrinks the JS bundle quite a bit, and should speed up hot module reloading too.

  • I tidied up the Storybook publishing logic and added a clause to script/postpublish that runs npm run publish-storybook if we're on the master branch in Actions. I tested it out and it worked, but we won't know for sure whether it works in Actions until we merge this PR to master and it succeeds.

@shawnbot shawnbot requested a review from emplums March 6, 2019 19:48
@shawnbot shawnbot marked this pull request as ready for review March 6, 2019 19:48
```

This is how we find all of the Markdown files in the bundle directory and generate stories from their code blocks. Storybook sections are labeled by the first argument to `storiesOf()` (in the above example, "Module name"), and individual stories get their titles from either the previous Markdown heading or the `title` attribute in the fenced code block. See the [`code-blocks` docs](https://npmjs.com/package/code-blocks) and the [`storiesFromMarkdown()` source](./.storybook/lib/storiesFromMarkdown.js) for more info.
Note: At this time, we do not load any stories from `src/**/stories.js`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Where are they loaded from? I'm confused by this comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's in the previous graf:

All html fenced code blocks in src/**/*.md will be rendered as stories and listed under the relevant module's name in the left-hand nav. File changes should trigger a live reload automatically (after a brief delay).

😁

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any reason why we can'd delete src/**/stories.js?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as I can tell, those didn't make it in the #666 migration.

}
})
#!/bin/bash
remote=${STORYBOOK_GIT_URL:-https://github.com/primer/storybook}
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we mean deploy here when we say publish?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, the "publish" terminology is from when we first introduced Storybook and just wasn't ever updated to match how we use "publish" and "deploy". In this case, though, I decided that putting it in script/postpublish was necessary to ensure that it would only be run when we published a new version.

🤔 I guess that's not exactly what we want, because that means it won't run if we don't publish a new version, which is what would happen if we merge this branch to master.

Copy link
Contributor

@emplums emplums Mar 7, 2019

Choose a reason for hiding this comment

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

ah, I think that's ok - we can manually run npm run publish-storybook when we'd like to publish outside of releases right?

@shawnbot shawnbot changed the base branch from master to release-12.1.1 March 6, 2019 23:55
@shawnbot shawnbot merged commit fadc008 into release-12.1.1 Mar 7, 2019
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.

2 participants