-
Notifications
You must be signed in to change notification settings - Fork 687
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Added storybook to venia concept for scaffolding #2355
Added storybook to venia concept for scaffolding #2355
Conversation
|
Thanks @fooman I'll try that :) |
…to 2352-storybook-venia-concept
Using the command @fooman suggested I can verify the changes also in a scaffolded app. :) |
Great work Niklas! |
|
It's taken us too long to get to this PR, but the value here makes sense to me and I think the approach is straightforward. We'll do a QA pass and make sure it works for us as well, but from my perspective I don't see any reason to hold this up. Thanks for your contribution and your patience. |
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.
@zetlen Sanity check, if you would, please. 👍
Performance Test Results The following fails have been reported by WebpageTest. These numbers indicates a possible performance issue with the PR which requires further manual testing to validate. https://pr-2355.pwa-venia.com : LH Performance Expected 0.85 Actual 0.52, LH Best Practices Expected 1 Actual 0.92 |
Would it be nice to provide info to user in console that
|
@dpatil-magento Nice catch! @niklaswolf Would you like to add that to this pull request? Those log messages come from https://github.com/magento/pwa-studio/blob/develop/packages/pwa-buildpack/lib/cli/create-project.js near the end of the file. |
- Move script commands out of venia-concept - Add post-install CLI message about storybook
storybook: 'start-storybook -p 9001 -c src/.storybook', | ||
'storybook:build': 'build-storybook -c src/.storybook -o storybook-dist' |
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.
@zetlen - I opted to move these scripts here from package.json
in venia-concept, since they weren't executable if attempting to run inside the mono (this assumes venia-ui
is in node_modules
). This set off some snapshot failures and I didn't feel confident in which method is best. Couple clarifying questions:
- Was it okay to have failing scripts in
venia-concept
? - Am I using
scriptsToInsert
as intended?
I ran the following verification steps:
|
Verified from sibling dir as well, looks good. ( Steps mentioned here - #2363 ) |
Description
Added storybook capabilities back to venia-concept as this is the template for the scaffolding command. Like this users of a new scaffolded app are able to see all stories from
venia-ui
and defined custom ones in their ownsrc
.Storybook than can be run using
yarn storybook
/npm run storybook
Related Issue
Closes #2352.
Acceptance
Verification Steps
I don't have any clue how to test an app scaffold locally, definetly need some help here.
I tested this by creating a new app using the scaffolding tool and applying the changes there. This worked and if I saw this correctly, the scaffolding copys every directory/file besides those ignored explicitly in the
create.js
.