-
-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
First pass at update the contribution instructions #982
Conversation
See #756 (comment) We should actually ensure this works consistently for people before making this change.
Codecov Report
@@ Coverage Diff @@
## master #982 +/- ##
==========================================
+ Coverage 12.6% 12.61% +<.01%
==========================================
Files 192 192
Lines 4426 4447 +21
Branches 707 710 +3
==========================================
+ Hits 558 561 +3
- Misses 3243 3258 +15
- Partials 625 628 +3
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested and these instructions look good to me.
However I am not sure about the difference between test-cra
and cra-storybook
. Storybook works in both directories, but npm start
only works for me in cra-storybook
. @ndelangen any input here?
My error for
|
``` | ||
git clone https://github.com/storybooks/storybook.git | ||
cd storybook | ||
npm install |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At this moment when I npm install
it only installs root dependencies. In order to install all packages, I need to run npm run bootstrap
.
OS: Windows. So I can't tell is it common or only windows issue (but previosly npm install
started to install everything inside packages folder)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ndelangen where are we at with fixing this problem? Should we update these instructions or will it be sorted soon?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be changed so npm run bootstrap
is a required step.
I changed this because bootstrapping is not always needed when adding a root dependency for example, but it does take a huge amount of time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added it
☁️ Nx Cloud ReportCI is running/has finished running commands for commit 037453d. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this CI Pipeline Execution ✅ Successfully ran 1 targetSent with 💌 from NxCloud. |
See #756 (comment)
We should actually ensure this works consistently for people before making this change.