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

[framework] add ember support #4237

Merged
merged 4 commits into from
Oct 3, 2018

Conversation

gabrielcsapo
Copy link
Contributor

@gabrielcsapo gabrielcsapo commented Sep 27, 2018

Issue: #2877

What I did

Added ember framework support

ember-example

How to test

Is this testable with Jest or Chromatic screenshots?
Does this need a new example in the kitchen sink apps? yes
Does this need an update to the documentation? yes

@ndelangen
Copy link
Member

This is super exciting!

@storybook-safe-bot
Copy link
Contributor

Fails
🚫

PR is not labeled with one of: ["cleanup","BREAKING CHANGE","feature request","bug","documentation","maintenance","dependencies:update","dependencies","other"]

Generated by 🚫 dangerJS

lib/cli/bin/generate.js Outdated Show resolved Hide resolved

let appInstance;

function render(options, el) {
Copy link

Choose a reason for hiding this comment

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

This is essentially the same as how component tests are setup and ran. I'd like to extract a primitive library that can be shared between the main @ember/test-helpers and this project so that we can more easily support broad ranges of Ember applications (and reduce specific coupling in storybook itself).

Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I totally think this would be something we could decouple. Something like @ember/setup that would just handle setting up and exposing a booted application

app/ember/src/client/preview/render.js Outdated Show resolved Hide resolved
app/ember/src/client/preview/utils.js Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Sep 27, 2018

Codecov Report

Merging #4237 into master will decrease coverage by 0.49%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #4237     +/-   ##
=========================================
- Coverage   40.64%   40.15%   -0.5%     
=========================================
  Files         498      507      +9     
  Lines        5875     5942     +67     
  Branches      785      791      +6     
=========================================
- Hits         2388     2386      -2     
- Misses       3109     3171     +62     
- Partials      378      385      +7
Impacted Files Coverage Δ
app/ember/src/server/build.js 0% <0%> (ø)
app/ember/src/client/index.js 0% <0%> (ø)
addons/centered/ember.js 0% <0%> (ø)
app/ember/src/server/index.js 0% <0%> (ø)
app/ember/bin/index.js 0% <0%> (ø)
addons/centered/src/ember.js 0% <0%> (ø)
app/ember/src/client/preview/globals.js 0% <0%> (ø)
lib/cli/lib/detect.js 0% <0%> (ø) ⬆️
app/ember/src/server/options.js 0% <0%> (ø)
app/ember/src/client/preview/render.js 0% <0%> (ø)
... and 23 more

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 e8bd830...7af2fc4. Read the comment docs.

@gabrielcsapo
Copy link
Contributor Author

gabrielcsapo commented Sep 28, 2018

@Keraito @ndelangen thoughts on the current state?

@gabrielcsapo gabrielcsapo force-pushed the storybook-emberjs branch 7 times, most recently from 9aaa4f4 to 9badf6e Compare September 28, 2018 05:23
app/ember/README.md Outdated Show resolved Hide resolved
app/ember/package.json Outdated Show resolved Hide resolved
examples/ember-cli/.storybook/addons.js Show resolved Hide resolved
examples/ember-cli/package.json Outdated Show resolved Hide resolved
examples/ember-cli/package.json Outdated Show resolved Hide resolved
app/ember/package.json Outdated Show resolved Hide resolved
.eslintignore Outdated Show resolved Hide resolved
@gabrielcsapo gabrielcsapo force-pushed the storybook-emberjs branch 2 times, most recently from 2955dc9 to afea517 Compare October 1, 2018 07:32
@gabrielcsapo
Copy link
Contributor Author

I updated the example gif and made the stories and intro make more sense in its context. (It used to say go to button and edit, now makes more sense for ember and the welcome-banner example)

@Hypnosphi
Copy link
Member

@Hypnosphi
Copy link
Member

Hypnosphi commented Oct 1, 2018

Can you please add separate commits instead of squashing them each time? Right now it's impossible to review the new diff

@gabrielcsapo
Copy link
Contributor Author

@Hypnosphi yes I can do that in the future

Copy link
Member

@igor-dv igor-dv left a comment

Choose a reason for hiding this comment

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

"build": "ember build",
"build-storybook": "yarn build && cp -r public/* dist; build-storybook -s dist",
"dev": "ember serve",
"storybook": "yarn build && cp -r public/* dist; start-storybook -p 9009 -s dist"
Copy link
Member

Choose a reason for hiding this comment

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

why should it be built before using with dev mode?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In order to build a storybook you need an ember app to introspect on. It is because of the dynamic nature of ember where as react has an implicit import structure, you can build a component standalone, ember needs to register all components beforehand. Which is why we have the build before dev necessity.

@gabrielcsapo
Copy link
Contributor Author

@igor-dv added the needed links to #supported-frameworks and live-examples

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.

8 participants