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

Cli framework choice #4184

Merged
merged 14 commits into from
Oct 13, 2018
Merged

Cli framework choice #4184

merged 14 commits into from
Oct 13, 2018

Conversation

Keraito
Copy link
Contributor

@Keraito Keraito commented Sep 16, 2018

Issue: Part of #1108

Added a --type -t flag to the storybook init command so users can manually choose which framework/template of Storybook to install. It takes a type parameter which (for now) needs to be an entry in lib/cli/lib/project_types.js. It also opens an inquirer when nothing is detected so that the user can choose one anyways if they like.

Example of providing type flag

storybook-type-install

Example of inquirer when undetected SB

storybook-undetected-install

All the options in the inquirer and the accepted types are extracted from lib/cli/lib/project_types.js. This way, when ppl add new frameworks to SB they don't have to make changes in the CLI either (f.e. the confusion in #4161) and it will be auto included.

Some issues/remarks:

  • Do people agree with adding this as a flag in such a way?
  • The keys/values in the project_types.js aren't really that descriptive. It's most likely that this will only be used on projects that are not compatible with SB, so most projects will be auto-detected anyways. But it might be vague what the difference is f.e. between REACT_PROJECT, REACT, and REACT_SCRIPTS.
  • Probably nice to add an option to the inquirer list that their project framework/type is not included, and then provide information like SB GH issues page if they really want it for their project?
  • Not sure how to efficiently test this.
  • The CLI folder is kinda getting cluttered, probably needs some structure soon 😅 .

@@ -176,17 +157,83 @@ export default function(options, pkg) {
"Please make sure you are running the `storybook init` command in your project's root directory."
);
paddedLog(
'You can also install storybook for plain HTML snippets with `storybook init --html` or follow some of the slow start guides: https://storybook.js.org/basics/slow-start-guide/'
'You can also install storybook for plain HTML snippets with `storybook init --type html` or follow some of the slow start guides: https://storybook.js.org/basics/slow-start-guide/'
Copy link
Member

Choose a reason for hiding this comment

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

I think those lines should be changed to something like "you can specify the project type explixitly via ..."

@libetl
Copy link
Member

libetl commented Sep 17, 2018

It looks good to me, but do can you write an unit test ?

Copy link
Member

@libetl libetl left a comment

Choose a reason for hiding this comment

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

But I would advise to write an unit test

@igor-dv
Copy link
Member

igor-dv commented Sep 18, 2018

@Keraito

I've found one more thing that should be changed:

image

@ndelangen
Copy link
Member

ndelangen commented Sep 18, 2018

💙💙💙💙💙💙💙💙💙💙💙💙💙💙💙
💙💛💙💙💙💛💙💛💙💛💙💛💛💛💙
💙💛💙💙💙💛💙💛💙💛💙💛💙💙💙
💙💛💙💙💙💛💙⁣💛💛💙💙💛💛💙💙
💙💛💙💙💙💛💙💛💙💛💙💛💙💙💙
💙💛💛💛💙💛💙💛💙💛💙💛💛💛💙
💙💙💙💙💙💙💙💙💙💙💙💙💙💙💙

this is cool @Keraito !

@Keraito
Copy link
Contributor Author

Keraito commented Sep 18, 2018

@libetl Will see what I can do. As stated in the first comment, I'm not quite sure how to write a meaningful (unit) and efficient test for this. But I will see what I can do.

@igor-dv Thanks! Missed it somehow.

@ndelangen That actually took me ages to read correctly 🤔 More to come!

@tomByrer
Copy link
Contributor

I like the 'inquirer'/ framework chooser.

@codecov
Copy link

codecov bot commented Sep 20, 2018

Codecov Report

Merging #4184 into master will increase coverage by 0.14%.
The diff coverage is 15.78%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4184      +/-   ##
==========================================
+ Coverage   40.04%   40.19%   +0.14%     
==========================================
  Files         503      504       +1     
  Lines        5903     5916      +13     
  Branches      795      804       +9     
==========================================
+ Hits         2364     2378      +14     
+ Misses       3156     3151       -5     
- Partials      383      387       +4
Impacted Files Coverage Δ
lib/cli/bin/generate.js 0% <ø> (ø) ⬆️
lib/cli/lib/detect.js 13.46% <ø> (+13.46%) ⬆️
lib/cli/lib/initiate.js 0% <0%> (ø) ⬆️
lib/cli/lib/project_types.js 100% <100%> (ø)
lib/cli/lib/latest_version.js 0% <0%> (ø) ⬆️
lib/cli/lib/helpers.js 1.21% <0%> (+1.21%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c91d502...cba68c4. Read the comment docs.

@Keraito
Copy link
Contributor Author

Keraito commented Sep 20, 2018

Updated with some unit tests.

@ndelangen
Copy link
Member

Super awesome @Keraito 💪

@igor-dv
Copy link
Member

igor-dv commented Sep 27, 2018

@Keraito, I think you can fix the conflict and merge (don't forget to address my comment in some PR, though).

@Keraito
Copy link
Contributor Author

Keraito commented Sep 27, 2018

Sounds good @igor-dv , do you have any idea who or how that screenshot was made so I can make a similar one?

@ndelangen
Copy link
Member

Which screenshot?

@Keraito
Copy link
Contributor Author

Keraito commented Sep 28, 2018

@igor-dv
Copy link
Member

igor-dv commented Oct 2, 2018

Doesn't git history show something? I think you can print-screen it from whatever tool you want =)

@pksunkara
Copy link
Member

@Keraito https://github.com/marionebl/svg-term. I used it for https://github.com/storybooks/vue-cli-plugin-storybook

Copy link
Member

@wuweiweiwu wuweiweiwu left a comment

Choose a reason for hiding this comment

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

LGTM! NICE WORK 💯

@Keraito
Copy link
Contributor Author

Keraito commented Oct 10, 2018

Resolved the merge conflicts

@Keraito Keraito mentioned this pull request Oct 10, 2018
Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

Super improvement!!! 🎖

'polymer',
'mithril',
'riot',
];
Copy link
Member

Choose a reason for hiding this comment

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

ember
marko
meteor?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm I just extracted this array to another file. Ember support wasn't present yet when I made this PR, but marko en meteor def should have been. I don't see them in the removed code either tho 😱 will add them I'm a few hours

@shilman
Copy link
Member

shilman commented Oct 11, 2018

@Keraito felt guilty about creating more merge conflicts so I resolved them for yuo (hopefully correctly)!

@libetl
Copy link
Member

libetl commented Oct 11, 2018

Good job, I think everything is ready, don't you ?

@ndelangen
Copy link
Member

@shilman I assume you tested manually? If so go ahead and merge

@ndelangen
Copy link
Member

This is great

@Keraito
Copy link
Contributor Author

Keraito commented Oct 11, 2018

@shilman it wasn't that big of a deal, but thank you very much! 🙇‍♂️

@Hypnosphi
Copy link
Member

Looks like this is the only thing left: https://github.com/storybooks/storybook/pull/4184/files#r224264956

@shilman shilman merged commit 7f7e549 into storybookjs:master Oct 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants