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

Add angular support: Storybook for Angular #1474

Merged
merged 80 commits into from
Oct 16, 2017
Merged

Conversation

alterx
Copy link
Member

@alterx alterx commented Jul 14, 2017

Issue: #269

What I did

This is the first attempt at adding support for Angular based, mostly, on the vue changes.

What works:

  • Added angular sample code to the examples folder
  • Rendering stories and creating stories out of existing angular components
  • Supports components with separate templates/css as well as single-file components.
  • Add-ons:
    • Notes
    • Actions
    • Links
    • Knobs
    • HMR

How to test

yarn bootstrap --reset --core
cd examples/angular-cli
yarn storybook
open http://localhost:9009

@alterx alterx changed the base branch from master to alterx-release/3.3 July 14, 2017 21:59
@alterx alterx changed the base branch from alterx-release/3.3 to release/3.3 July 14, 2017 21:59
@codecov
Copy link

codecov bot commented Jul 14, 2017

Codecov Report

Merging #1474 into release/3.3 will decrease coverage by 0.76%.
The diff coverage is 12.38%.

Impacted file tree graph

@@               Coverage Diff               @@
##           release/3.3    #1474      +/-   ##
===============================================
- Coverage         22.3%   21.53%   -0.77%     
===============================================
  Files              326      364      +38     
  Lines             6537     7081     +544     
  Branches           810      908      +98     
===============================================
+ Hits              1458     1525      +67     
- Misses            4476     4875     +399     
- Partials           603      681      +78
Impacted Files Coverage Δ
lib/cli/lib/project_types.js 0% <ø> (ø) ⬆️
app/angular/bin/build.js 0% <ø> (ø)
app/angular/bin/index.js 0% <ø> (ø)
lib/cli/test/snapshots/angular-cli/karma.conf.js 0% <0%> (ø)
app/angular/src/client/preview/index.js 0% <0%> (ø)
app/angular/src/client/manager/provider.js 0% <0%> (ø)
app/angular/src/client/preview/config_api.js 0% <0%> (ø)
app/angular/src/server/config/babel.prod.js 0% <0%> (ø)
lib/cli/lib/detect.js 0% <0%> (ø) ⬆️
addons/knobs/angular.js 0% <0%> (ø)
... and 106 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 5c3fc6c...5f3fa62. Read the comment docs.

@shilman
Copy link
Member

shilman commented Jul 14, 2017

Awesome possum!! Super excited @alterx and welcome to Team Storybook! 🍻

@ndelangen ndelangen requested review from shilman and ndelangen July 18, 2017 07:38
@alterx alterx mentioned this pull request Jul 19, 2017
@ndelangen
Copy link
Member

I rebased 3.2 with master,
rebased 3.3 with 3.2
and rebased this branch with 3.3

@ndelangen ndelangen changed the title 269 angular support Add angular support: Storybook for Angular Jul 20, 2017
@ndelangen ndelangen added this to the v3.3.0 milestone Jul 23, 2017
@ndelangen ndelangen force-pushed the 269-angular-support branch from 257041d to 10ab08e Compare July 23, 2017 08:56
@ndelangen ndelangen force-pushed the 269-angular-support branch from 71730a5 to 7c3da0e Compare July 31, 2017 16:06
@@ -37,7 +37,7 @@ Now, write your stories with knobs.

```js
import { storiesOf } from '@storybook/react';
import { withKnobs, text, boolean, number } from '@storybook/addon-knobs';
import { addonKnobs, text, boolean, number } from '@storybook/addon-knobs';
Copy link
Member

Choose a reason for hiding this comment

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

This is wrong now..

// Knobs as dynamic variables.
stories.add('as dynamic variables', () => {
stories.add('as dynamic variables', addonKnobs(options)(() => {
Copy link
Member

Choose a reason for hiding this comment

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

Must have been 'reverted' when rebasing

Copy link
Member

@ndelangen ndelangen left a comment

Choose a reason for hiding this comment

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

Overall looks good! Will test on my machine..

}
};
})
// .add('All knobs', () => {
Copy link
Member

Choose a reason for hiding this comment

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

Do these not work currently? or is there another reason why these are commented-out?

@Component({
selector: 'knobs-component',
template: `
<div>
Copy link
Member

Choose a reason for hiding this comment

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

An empty div? Is this uncompleted / in progress or is this the way it should be?

@alterx
Copy link
Member Author

alterx commented Aug 2, 2017 via email

@ndelangen ndelangen force-pushed the 269-angular-support branch from e4420a8 to a4f665e Compare August 18, 2017 16:16
alterx and others added 9 commits August 18, 2017 18:16
This is the first attempt at adding support for Angular based, mostly, on the vue changes.

What works:
- Added angular sample code to the `examples` folder
- Rendering stories and creating stories out of existing angular components
- For the time being, this only supports one-page angular components

What's missing:
- Addons support
- Support for components that have a separate template

Signed-off-by: Carlos Vega <clmvega@gmail.com>
Signed-off-by: Carlos Vega <clmvega@gmail.com>
- `start-storybook` / `build-storybook` should be run out of
`node_modules/.bin`
- update lerna config to not publish angular-test example
Signed-off-by: Carlos Vega <clmvega@gmail.com>
Signed-off-by: Carlos Vega <clmvega@gmail.com>
We had TypeScript errors related to conflicting @types included in the root @storybook repo and the angular-cli sample. Also, we had some issues with TS not recognizing `describe`, `it` and other global functions exposed by jasmine.

Signed-off-by: Carlos Vega <clmvega@gmail.com>
- Links
- Actions
- Notes

Signed-off-by: Carlos Vega <clmvega@gmail.com>
Signed-off-by: Carlos Vega <clmvega@gmail.com>
@alterx
Copy link
Member Author

alterx commented Oct 10, 2017

Ok guys, I spent a fair amount of time digging into these webpack errors and found several things:

  1. Some of the issues are related to the fact the the Angular CLI, prior to 1.0 had a different structure (it didn't have a tsconfig.json in the base folder). This caused most of the errors as the ts-loader was looking for this file on every parent directory, and it tried to transpile a lot of files that didn't even need to be transpiled. This first issue can be fixed by simply adding a tsconfig.json file to the root of the angular-cli project.

  2. We had some issues with the multiple versions of TypeScript, I'm not a yarn/lerna master but it looks like we had some conflicting versions that were causing 2.0.10 to get installed. This is a fairly old version that causes some issues. Updating the version to a more recent one fixed some of the errors.

  3. Last but not least, there was some errors caused by a missing jasmine import and an old Protractor version.

After fixing these issues I was able to get it to compile using Angular 2.3.1 so we can say that it wasn't really an angular issue. It's just that a some of the modules angular uses are coupled to specific versions of TypeScript. I've committed those changes to the branch @Hypnosphi created just to demonstrate that we can fix it, here's the commit b604123. With that being said I'd recommend to use the @angular/cli@1.0.0 as our baseline (this installs angular@4.0.0) and just add something to let users know they can install it in previous versions of Angular but will need to play a bit with TypeScript and dependencies to make it work.

Thoughts on this?

ps. That branch was also using addonNotes instead of withNotes

@ndelangen
Copy link
Member

@alterx I agree, Sounds like you fixed it? Let's get this merged so @shilman can spend his weekend refactoring the common bits from the apps. I know he's looking forward to it! 😉

Signed-off-by: Carlos Vega <clmvega@gmail.com>
@alterx
Copy link
Member Author

alterx commented Oct 14, 2017

Guys, there's a problem with the CLI test, the latest-version package cannot find @storybook/angular (of course, it hasn't been published yet)

 getstorybook - the simplest way to add a storybook to your project. 

 • Detecting project type. ✓
 • Adding storybook support to your "Angular" app
     Package `@storybook/angular` doesn't exist
npm info lifecycle @storybook/cli@3.3.0-alpha.2~test: Failed to exec test script
npm ERR! Test failed.  See above for more details.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
Exited with code 1

It looks like this is the last issue, the build issues are gone 🎉
@ndelangen @Hypnosphi

Signed-off-by: Carlos Vega <clmvega@gmail.com>
Signed-off-by: Carlos Vega <clmvega@gmail.com>
Signed-off-by: Carlos Vega <clmvega@gmail.com>
@alterx
Copy link
Member Author

alterx commented Oct 14, 2017

I've added more changes to fix these tests (changes include mocking the @storybook/angular version only for this release. Now, the issue happens when attempting to install the @storybook/angular package.

@Hypnosphi
Copy link
Member

That's weird, it should link a local version. I'll take a look

@alterx
Copy link
Member Author

alterx commented Oct 14, 2017

It is weird, I even had to create this 9becf40 commit to mock the version (it tries to fetch the version from the npm registry using https://www.npmjs.com/package/latest-version

@Hypnosphi
Copy link
Member

But it is 3.3.0-alpha.0, according to its package.json

@alterx
Copy link
Member Author

alterx commented Oct 14, 2017

Hmm, I thought I bumped them all up here 64b7d3d

@Hypnosphi
Copy link
Member

Well, the version field left the same (actually you're not supposed to do this by hand, so it's ok)

@alterx
Copy link
Member Author

alterx commented Oct 14, 2017

Oh, ok, good to know that :)

@ndelangen
Copy link
Member

We're good to go, I think.
@shilman can you update your review?

@ndelangen ndelangen requested review from Hypnosphi and removed request for usulpro October 16, 2017 06:22
alterx and others added 4 commits October 16, 2017 16:20
Signed-off-by: Carlos Vega <clmvega@gmail.com>
Signed-off-by: Carlos Vega <clmvega@gmail.com>
Signed-off-by: Carlos Vega <clmvega@gmail.com>
@shilman shilman merged commit 77deab5 into release/3.3 Oct 16, 2017
@shilman shilman deleted the 269-angular-support branch October 16, 2017 23:16
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.

5 participants