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

[Feature] Adding storybook and MDX script #5

Merged
merged 40 commits into from
Apr 21, 2020
Merged

Conversation

MatteoPieroni
Copy link
Collaborator

@MatteoPieroni MatteoPieroni commented Mar 10, 2020

PR Description

  • Added storybook and configured to work with TS and MDX
  • Added to README generation in gen-package-readme/index a task to generate README.story.
  • Added validation to prompt when selecting packages
  • Added skip step function

Motivation and Context

We wanted a way to showcase the use of hooks in a component, this is the first step

Types of changes

  • New feature (non-breaking change which adds functionality)

@MatteoPieroni
Copy link
Collaborator Author

I also pushed the config for Netlify, hope you don't mind: https://deploy-preview-5--use-hooks.netlify.com/?path=/docs/useboolean--usage 😃

packages/boolean/README.story.mdx Outdated Show resolved Hide resolved
packages/boolean/StoryComponent.tsx Outdated Show resolved Hide resolved
@MatteoPieroni
Copy link
Collaborator Author

The only thing that doesn't really work as intended is the code preview:
Screenshot from 2020-03-16 22-23-07

@simmo simmo changed the base branch from master to develop March 17, 2020 22:30
@MatteoPieroni
Copy link
Collaborator Author

Any comments on this @simmo ?

@simmo
Copy link
Owner

simmo commented Apr 7, 2020

This is looking good. Is there a way we can hide canvas and just have it show the docs?
image

@MatteoPieroni
Copy link
Collaborator Author

This is looking good. Is there a way we can hide canvas and just have it show the docs?
image

Done, I added the --docs flag to the storybook scripts.

On another note, did we ditch Netlify?

@simmo
Copy link
Owner

simmo commented Apr 8, 2020

Its should still be working but looks like the check is missing from Github 🤔

packages/boolean/StoryComponent.tsx Outdated Show resolved Hide resolved
@MatteoPieroni
Copy link
Collaborator Author

@simmo I added a function called emptyPromptRetry to be able to read an empty selection for the prompt and re-ask the same question an x number of times.

I am skipping the story generation step if there is no StoryComponent file.

I also added the skip for the package gen if there are no packages selected after the 3 warnings, I wanted to add a message to this case but Listr automatically swallows all logs and it's useless to throw an error as we are not making the build fail.
Should we remove the exitOnError: false? I would argue it's better if we make it fail, even if it fails only for one package

@simmo
Copy link
Owner

simmo commented Apr 20, 2020

@simmo I added a function called emptyPromptRetry to be able to read an empty selection for the prompt and re-ask the same question an x number of times.

I might be missing this, but could the prompt just have a validate option that requires at least one to be selected?

I am skipping the story generation step if there is no StoryComponent file.

👍

I also added the skip for the package gen if there are no packages selected after the 3 warnings, I wanted to add a message to this case but Listr automatically swallows all logs and it's useless to throw an error as we are not making the build fail.

Does that mean it skips the entire script?

Should we remove the exitOnError: false? I would argue it's better if we make it fail, even if it fails only for one package

Yep I think thats ok, my only thought is that the packages before the error will have succeeded so therefore in one package will then stop any following from passing. What about keeping as is but cachet has the error?

README.md Outdated Show resolved Hide resolved
packages/boolean/StoryComponent.tsx Show resolved Hide resolved
scripts/tasks/gen-project-readme/index.js Outdated Show resolved Hide resolved
MatteoPieroni and others added 2 commits April 20, 2020 22:35
Co-Authored-By: Mike Simmonds <simmo@users.noreply.github.com>
Co-Authored-By: Mike Simmonds <simmo@users.noreply.github.com>
@simmo
Copy link
Owner

simmo commented Apr 20, 2020

Looks like Netlify checks are back. I re added the integration so... 🤞

@MatteoPieroni
Copy link
Collaborator Author

I might be missing this, but could the prompt just have a validate option that requires at least one to be selected?

It can yes, I didn't check the prompt object 😓 Changed it!

Does that mean it skips the entire script?

Yes, I removed it now, since prompt doesn't let you go ahead if validation doesn't pass

Yep I think thats ok, my only thought is that the packages before the error will have succeeded so therefore in one package will then stop any following from passing. What about keeping as is but cachet has the error?

Sorry, I didn't quite understood what you mean

scripts/utils/index.js Outdated Show resolved Hide resolved
@simmo
Copy link
Owner

simmo commented Apr 20, 2020

Yep I think thats ok, my only thought is that the packages before the error will have succeeded so therefore in one package will then stop any following from passing. What about keeping as is but cachet has the error?

Sorry, I didn't quite understood what you mean

If you have packages; A, B, C, D.

Currently, if the script fails at C, both A and B will have completed, C failed and the script will continue to run D. The outcome is either success or failed for each package.

If we change exitOnError to be a true, then a failure at C will mean D will never run. The outcome will then be either, success, failed or not run.

@MatteoPieroni
Copy link
Collaborator Author

That makes sense, the part I was confused about was this:

What about keeping as is but cachet has the error?

@simmo
Copy link
Owner

simmo commented Apr 21, 2020

That makes sense, the part I was confused about was this:

What about keeping as is but cachet has the error?

Ah ok, sorry bad type on my part there! Using the example in the previous comment, could we keep it as is, but catch the failure at C? That way A, B and D would still succeed and you'd only need to rerun C. We could even catch failures and then ask to rerun the failed ones?

@MatteoPieroni
Copy link
Collaborator Author

Ah ok, sorry bad type on my part there! Using the example in the previous comment, could we keep it as is, but catch the failure at C? That way A, B and D would still succeed and you'd only need to rerun C. We could even catch failures and then ask to rerun the failed ones?

Yeah, I think that would be good.

Should we do it in the next PR though, so we can get this merged? I'm going crazy with the dependabot PRs to be merged in the branch 😅

@simmo simmo added this to the 0.1.0 milestone Apr 21, 2020
@simmo
Copy link
Owner

simmo commented Apr 21, 2020

Yeah, I think that would be good.

Cool, I'll let you add a ticket for that 😉

Should we do it in the next PR though, so we can get this merged? I'm going crazy with the dependabot PRs to be merged in the branch 😅

Yep, love dependabot, but I've getting RSI from all the merging and PRing! 😂

Yes, merge when you're happy! 🚀
We can use #3 to add more docs once this base in merged.

@MatteoPieroni MatteoPieroni merged commit 771f091 into develop Apr 21, 2020
@MatteoPieroni MatteoPieroni deleted the feature/storybook branch April 21, 2020 11:07
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