-
-
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
[ember] update ember ergonomics to not require any manual setup #4594
[ember] update ember ergonomics to not require any manual setup #4594
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4594 +/- ##
=======================================
Coverage 35.59% 35.59%
=======================================
Files 557 557
Lines 6732 6732
Branches 884 884
=======================================
Hits 2396 2396
Misses 3876 3876
Partials 460 460 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.
@gabrielcsapo smoke tests are still failing. mind looking into it? also there is a conflict in yarn.lock
3fc4b75
to
18b14ea
Compare
…build an application before we start storybook
@igor-dv @ndelangen would you be able to take a look at this? |
@gabrielcsapo What's $$ yarn storybook:dev
yarn run v1.10.1
$ yarn serve & start-storybook -p 9009 -s dist, public
error Command "serve" not found. |
@shilman it was supposed to be |
|
||
```json | ||
{ | ||
"scripts": { | ||
"storybook": "start-storybook -p 9001 -s dist" | ||
"build-storybook": "ember build & build-storybook -p 9001 -s 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.
This can be &&
?
examples/ember-cli/package.json
Outdated
@@ -6,7 +6,8 @@ | |||
"build": "ember build", | |||
"build-storybook": "yarn build && cp -r public/* dist && build-storybook -s dist", | |||
"dev": "ember serve", | |||
"storybook": "yarn build && start-storybook -p 9009 -s dist, public" | |||
"storybook": "yarn build && start-storybook -p 9009 -s dist, public", |
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 don't think dist, public
works (tested briefly on my machine).
Also I don't think it's necessary -- looks like ember copies everything from public
into dist
when it builds?
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 don't know if that is the cases, I know I needed to copy the files for build-storybook, these are needed
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.
When I added a new file logo1.png
into public
it automatically gets copied into dist
when I ember build
.
When I yarn storybook
and try to access it at the following URL, it succeeds:
http://localhost:9009/logo1.png
When I remove the file from dist
and try to access it at the following URL, it 404s:
http://localhost:9009/logo1.png
Therefore, I believe:
- The syntax "dist, public" doesn't work (maybe no comma is needed?), or possibly Storybook's ability to serve from multiple directories doesn't work.
- We don't even need to be serving from
public
becauseember build
does the copying even when I don't use the file anywhere.
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.
makes sense, tested this also and it works.
@shilman I think I resolved your feedback |
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.
LGTM!
[ember] update ember ergonomics to not require any manual setup
Issue: The previously submitted PR did not accommodate for dist not being around, changes were made in the ember-cli-storybook plugin to ensure it used the right build time hook to ensure dist was available.
What I did
Removes hacky solutions for setting up ember environment, now supports ember's live reload out of the box.