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

Default examples #144

Merged
merged 11 commits into from
May 20, 2016
Merged

Default examples #144

merged 11 commits into from
May 20, 2016

Conversation

tizmagik
Copy link
Collaborator

@tizmagik tizmagik commented May 18, 2016

TL;DR Want to provide example usage automatically. This PR isn't ready to be merged, just wanted some early guidance.

I wanted a way to simply create default example usages in an existing codebase with many 'pure' components. For the most part, I found that just having something like <COMPONENT /> as example usage was sufficient, so I wanted to provide this functionality. It's clearly not very robust, but I think it's useful enough where it may be a valuable option to have for folks. In cases where it can't render the component based on the default template, just seeing an error in the styleguide was more helpful than not having any example codemirror block at all as it indicated to the developer when manually created examples would be useful.

Before taking the implementation too far, however, I wanted to sense-check the approach with you, @sapegin . Please have a look and let me know if this is completely the wrong direction (very possible!) as I just spent a couple hours on this and didn't dig too deeply into the inner workings of react-styleguidist. So any direction you could provide would be very helpful before I get too far long. For example, I'm not entirely sure if it makes sense to have to pass in the componentName as a query param to the example-loader, but it was the least invasive approach I could think of.

Still to do:

  • A little more robust-ness where it can maybe detect components that have some required props? Especially for child component requirements. For another PR
  • Tests
  • Config validation
  • Add info to README, CHANGELOG?, etc.

Thank you!

@sapegin
Copy link
Member

sapegin commented May 19, 2016

Thanks! That might be useful but I think it should be disabled by default.

Not sure what to do with components with required props. I’d rather skip them or make another option to enable them.

Add info to README, CHANGELOG?, etc.

Readme — yep, changelog — nope.

@tizmagik
Copy link
Collaborator Author

Great! Agreed, probably better if it's off by default, but I did want the ability to specify the markdown file to use for the default examples.

So we could either have 2-flags, which I think is unnecessarily verbose or have a flag that can be boolean|string and false by default. When true it'll use DefaultExamples.md in the templates directory, when it's a string it'll use the user-specified markdown file. I'll go with that for now unless you have another approach in mind?

As for required prop detection, maybe I'll save that for another PR :)

Thanks!

@sapegin
Copy link
Member

sapegin commented May 19, 2016

I’m for a single flag ;-)

@tizmagik
Copy link
Collaborator Author

@sapegin please have a look. I think this PR is ready to be merged. Let me know what you think!

Thanks!

@@ -20,7 +20,7 @@ function getPackagePath(packageName) {
function validateWebpackConfig(webpackConfig) {
webpackConfig.module.loaders.forEach(function(loader) {
if (!loader.include && !loader.exclude) {
throw Error('Styleguidist: "include" option is missing for ' + loader.test + ' Webpack loader.');
throw Error('Styleguidist: "include" (or "exclude") option is missing for ' + loader.test + ' Webpack loader.');
Copy link
Member

Choose a reason for hiding this comment

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

It’s out of the scope of the PR and wouldn’t recommend using exclude, that’s why it’s not included in the warning.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm, but it satisfies the conditional you're checking. What's wrong with using exclude?

Copy link
Member

Choose a reason for hiding this comment

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

Because exclude also works in most cases. But I believe explicit is always better than implicit. So it’s fine to use it but I don’t want to “advertise” the way I think is not the best.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fair enough I'll remove it from this PR

* master:
  Tweak title.
  Add examples and showcase.
  Move similar projects list to Wiki.
  Fix link.
  Reorganize readme.
  Add React Storybook and React Cards to similar projects.

# Conflicts:
#	Readme.md
@tizmagik
Copy link
Collaborator Author

@sapegin :shipit:

@sapegin sapegin merged commit fc2a51a into styleguidist:master May 20, 2016
@sapegin
Copy link
Member

sapegin commented May 20, 2016

Thanks! 👍

@tizmagik
Copy link
Collaborator Author

Thank you! 😄

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.

None yet

2 participants