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

Added step to deployment to build the static storybook site. #1412

Merged
merged 4 commits into from Jan 2, 2019
Merged

Added step to deployment to build the static storybook site. #1412

merged 4 commits into from Jan 2, 2019

Conversation

nickytonline
Copy link
Contributor

What type of PR is this? (check all applicable)

  • Refactor
  • Feature
  • Bug Fix
  • Documentation Update

Description

Implements the step that builds the static storybook. If it fails, the build fails. This will prevent issues that occurred with #1407.

Still todo, deploy it to netlify.

Related Tickets & Documents

#338

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

Added to documentation?

  • docs.dev.to
  • readme
  • no documentation needed

[optional] What gif best describes this PR or how it makes you feel?

Build Step

@pr-triage pr-triage bot added the PR: unreviewed bot applied label for PR's with no review label Dec 27, 2018
@nickytonline
Copy link
Contributor Author

All that's required is deploying the static-storybook folder to netlify. @benhalpern, @maestromac, should I follow how this is done for the docs by adding a redirect to the folder and update the netlify.toml to redirect the folder to e.g. https://storybook.dev.to?

@maestromac
Copy link
Member

maestromac commented Dec 27, 2018

It was a bit tricky to get right. We had to add additional layer configuration so that Netlify wouldn't installed all Ruby & JS dependecies when only 1 JS dependency is needed. With our docs, the only command needed were installing gitdoc & building it.

You are free to give it a whirl and we'll probably alter it before the last push.

cc @Zhao-Andy

@@ -40,6 +40,7 @@ script:
- './cc-test-reporter sum-coverage coverage/codeclimate.*.json'
- './cc-test-reporter upload-coverage'
- 'bundle exec bundle-audit check --update'
- yarn build-storybook
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We just want to ensure that this builds. If it doesn't something is really broken in the UI or stories were not updated.

netlify.toml Outdated
@@ -35,3 +36,7 @@
from = "https://devto.netlify.com/*"
to = "https://docs.dev.to/:splat"
status = 301
[[redirects]]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just an idea as I don't know how to deploy multiple directories to Netlify. As well the storybook.dev.to domain would need to be set up in Netlify.

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe there's a way, but I'll have to dig into their docs and figure it out. We can set up storybook.dev.to in Netlify as well.

In general, TOML/YAML have always felt pretty cryptic to me. :|

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 can dig in as I imagine you have a few things on your plate 😉

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure if you'd like. Would certainly be appreciated!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I guess it depends how you slice it. Storybook is its own static site, so it probably makes more sense to have a new site in Netlify just for it. We could overwrite the netlify.toml file with one that is say called storybook.netlify.toml that gets renamed to netlify.toml. and then deploy.

I think that makes more sense than what is currently in this PR. Thoughts? @Zhao-Andy?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm I think that makes more sense, too. Like you said, I think it would have to be a new Netlify site, but we might be able to share the same netlify.toml file. Not sure the exact configuration so I'll have to look into it more. I'm okay with whatever you think is best.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool beans. I'll pop up a change shortly and maybe discuss it with the rest of the team.

netlify.toml Outdated Show resolved Hide resolved
@nickytonline
Copy link
Contributor Author

@Zhao-Andy, @maestromac I think for now, let's just go with the step that builds storybook. I need to dig into why I'm having issues with the netlify.toml and also, need to see about having multiple netlify.toml files or how to use one to deploy multiple sites. I'm going to test it with a branch on my own netlify account.

Building storybook as part of the build will at least guarantee that storybook dependency updates, if not valid, will fail the build, i.e. #1407, or if someone changes a component and it breaks a storybook story, the build will also fail.

Once I get the deployment part figured out, I'll create a separate PR.

@Zhao-Andy
Copy link
Contributor

@nickytonline that sounds great. Thanks for the clear communication and for researching.

Copy link
Member

@maestromac maestromac left a comment

Choose a reason for hiding this comment

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

LGTM!

@pr-triage pr-triage bot added PR: reviewed-approved bot applied label for PR's where reviewer approves changes and removed PR: unreviewed bot applied label for PR's with no review labels Jan 2, 2019
@maestromac maestromac merged commit b48e573 into forem:master Jan 2, 2019
@pr-triage pr-triage bot added PR: merged bot applied label for PR's that are merged and removed PR: reviewed-approved bot applied label for PR's where reviewer approves changes labels Jan 2, 2019
@nickytonline nickytonline deleted the feature/338-deploy-storybook branch April 9, 2019 23:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: merged bot applied label for PR's that are merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants