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

Improve Intro to Storybook. Get Started chapter #297

Closed
wants to merge 2 commits into from

Conversation

orelmax
Copy link

@orelmax orelmax commented Mar 21, 2020

Added additional paragraph regarding serving static files to Add assets section

Added additional paragraph regarding serving static file
@@ -67,7 +67,8 @@ If you want to modify the styling, the source LESS files are provided in the Git
To match the intended design, you'll need to download both the font and icon directories and place its contents inside your `public` folder.

<div class="aside">
<p>We’ve used <code>svn</code> (Subversion) to easily download a folder of files from GitHub. If you don’t have subversion installed or want to just do it manually, you can grab the folders directly <a href="https://github.com/chromaui/learnstorybook-code/tree/master/public">here</a>.</p></div>
<p>We’ve used <code>svn</code> (Subversion) to easily download a folder of files from GitHub. If you don’t have subversion installed or want to just do it manually, you can grab the folders directly <a href="https://github.com/chromaui/learnstorybook-code/tree/master/public">here</a>.</p>
<p>To configure Storybook to serve static content refer to <a href="https://storybook.js.org/docs/configurations/serving-static-files">serving static content</a>.</p></div>
Copy link
Collaborator

Choose a reason for hiding this comment

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

@orelmax thank you for this small but rather on topic contribution. But i think we can do better. Probably go with something like:

<p>If you want to learn more about serving static content in your Storybook, you can read more about it in <a href="https://storybook.js.org/docs/configurations/serving-static-files">here</a>.</p>

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, this sounds better. I've finished the whole tutorial but didn't found any mentions of serving statics configuration and thought it'd improve people experience.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@orelmax that's beyond of the scope of the tutorial, as it's focus is on informing you in this case how to set it up with a practical example and throughout it mention the core functionalities at your disposal. If you don't mind i'm ok with merging this and i'll do it tomorrow (sunday) if you don't mind.

Slightly rephrase of serving static content note
@domyen
Copy link
Member

domyen commented Mar 22, 2020

@orelmax Thanks for the contribution. I'm a bit confused, when you were going through the tutorial did you put the assets in the /public folder? Or did you set up a Storybook to serve static files?

iirc We chose the /public folder because it gets generated with CRA. That means everyone will get it automatically when they run the taskbox setup commands.

Serving static assets via SB is typically a more advanced use case. I'm concerned people will go off track if we mention static asset hosting this early in the tutorial.

@jonniebigodes
Copy link
Collaborator

@domyen i've thinking a bit more about this and why not move this to it's own, at the lack of a better word recipe. I mean that we could grab this case and other cases like for instance how to use graphql and storybook and add them to the visual testing handbook. Turning it into a Storybook "cookbook" (pardon the bad pun) for quick reference. Making use of that content and complementing the official docs.

Let me know what do you think

@domyen
Copy link
Member

domyen commented Mar 24, 2020

That's an excellent suggestion @jonniebigodes. We have an open issue to create another guide with this type of content. I'll make note of it there! :)

I don't think we should merge this PR because it's an advanced technique and I'd prefer the guide be focused on the beginner experience. (Obviously, still very appreciative for @orelmax for the PR 🙇‍♂️)

@jonniebigodes
Copy link
Collaborator

@domyen i'm going to close this then per your feedback.

@orelmax to not let your work go to waste so to speak, towards the end of the week i'm going to draft a pr with a work in progress regarding the issue mentioned and i would like for you to continue your work there. I have a draft on paper on how the content could be outlined and with that we can address the issue. Sounds good?

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