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

Adding extra metadata to module/components #2526

Merged
merged 6 commits into from
Dec 22, 2017

Conversation

alterx
Copy link
Member

@alterx alterx commented Dec 20, 2017

Issue:

What I did

Added the ability to provide custom metadata to the story. This works, for example, when you need to include a service, a custom pipe or directive.

How to test

yarn bootstrap
cd examples/angular-cli
yarn storybook

@codecov
Copy link

codecov bot commented Dec 20, 2017

Codecov Report

Merging #2526 into release/3.3 will increase coverage by <.01%.
The diff coverage is 0%.

Impacted file tree graph

@@               Coverage Diff               @@
##           release/3.3    #2526      +/-   ##
===============================================
+ Coverage        19.45%   19.45%   +<.01%     
===============================================
  Files              387      387              
  Lines             8734     8733       -1     
  Branches           944      941       -3     
===============================================
  Hits              1699     1699              
- Misses            6310     6332      +22     
+ Partials           725      702      -23
Impacted Files Coverage Δ
addons/knobs/src/angular/helpers.js 0% <0%> (ø) ⬆️
app/vue/src/server/utils.js 0% <0%> (-53.58%) ⬇️
addons/actions/src/lib/retrocycle.js 34.09% <0%> (ø) ⬆️
app/react-native/src/preview/story_kind.js 0% <0%> (ø) ⬆️
lib/ui/src/libs/key_events.js 22.22% <0%> (ø) ⬆️
app/react/src/client/preview/element_check.js 41.17% <0%> (ø) ⬆️
addons/viewport/src/components/SelectViewport.js 15% <0%> (ø) ⬆️
lib/ui/src/modules/ui/components/shortcuts_help.js 12.85% <0%> (ø) ⬆️
addons/actions/src/lib/types/undefined/index.js 38.46% <0%> (ø) ⬆️
addons/info/src/components/types/ObjectType.js 14.28% <0%> (ø) ⬆️
... and 61 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 5966ae3...1f13274. Read the comment docs.

Signed-off-by: Carlos Vega <clmvega@gmail.com>
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.

  1. Is there some documentation that needs to be edited about how to use new features?
  2. As one that is not familiar with most of the angular things (now better, but still =) ), I think it's very important to add descriptive and intuitive names of just everything. I would like to create some followup PR with a bit refactoring/renaming/separating things, will it be OK?

@@ -13,7 +13,8 @@ import {
OnDestroy,
EventEmitter
} from "@angular/core";
import { STORY, Data } from "../app.token";
import { STORY } from "../app.token";
import { Data } from "../types";

@Component({
selector: "my-app",
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can set a better selector name for the app component? "main-angular-app", "app-root" ... idk ...

Copy link
Member Author

Choose a reason for hiding this comment

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

We want these to be the same since they all hook into the same tag in the template.
I'm going to go with app-root

@@ -1,5 +1,6 @@
import { Component, Inject } from "@angular/core";
import { STORY, Data } from "../app.token";
import { STORY } from "../app.token";
import { Data } from "../types";

@Component({
selector: "my-app",
Copy link
Member

Choose a reason for hiding this comment

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

also here. maybe "error-root"

Copy link
Member Author

Choose a reason for hiding this comment

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

We want these to be the same since they all hook into the same tag in the template.
I'm going to go with app-root

moduleMetadata: NgModuleMetadata
}

export type Data = {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe you can give this one an intuitive name as well ? And can't we actually use the NgStory type ?

@@ -20,28 +20,11 @@ import { Welcome, Button } from '@storybook/angular/demo';
import { SimpleKnobsComponent } from './knobs.component';
Copy link
Member

@igor-dv igor-dv Dec 21, 2017

Choose a reason for hiding this comment

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

I think we need to separate these examples into different stories like it's done in the react example. Surely not in this PR =)

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed. Back when we started with these the react example was this way, we surely need to update these too

name: 'Static name'
},
moduleMetadata: {
imports: [],
Copy link
Member

Choose a reason for hiding this comment

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

Did you put here all the props in order to visually show all the options?

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 kinda documentation, but maybe add it to a real documentation too?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, they're just there to show you can actually add all of them

import { DummyService } from './dummy.service';

@Component({
selector: 'simple-knobs-component',
Copy link
Member

Choose a reason for hiding this comment

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

simple-service-component ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup

@@ -2,7 +2,8 @@ import {
enableProdMode,
NgModule,
Component,
CUSTOM_ELEMENTS_SCHEMA
NgModuleRef,
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need them, if they are not in use?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, you're right, I probably screwed the rebase :P

@@ -34,7 +36,9 @@ const debounce = (func, wait = 100, immediate = false) => {
};
};

const getComponentMetadata = ({ component, props = {}, propsMeta = {}, pipes = [] }) => {
const getComponentMetadata = (
{ component, props = {}, propsMeta = {}, moduleMetadata }: NgStory
Copy link
Member

@igor-dv igor-dv Dec 21, 2017

Choose a reason for hiding this comment

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

moduleMetadata = {} and you won't need to check it further?

ndelangen and others added 4 commits December 21, 2017 11:32
- Changing main tag to a more meaningful name
- Removing redundant type Data
- Validating moduleMetadata param
- Removing unused references

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

alterx commented Dec 21, 2017

@igor-dv after this one gets merged feel free to submit a PR to rename some stuff, these names were chosen on the go and now that we have a more feature complete product I think there's a lot of room for improvement in naming :)
Also, the types here are just scratching the surface and I added them mainly to keep some sanity while coding. There's a lot of room for improvement there, too.

@ndelangen @igor-dv how do we want to procede with documentation?

@felipe-norato
Copy link

I will try to help you, above all in docs.
I will use my personal profile @norato

@igor-dv
Copy link
Member

igor-dv commented Dec 21, 2017

There is an outdated demo gif
image
And it's used in all the apps (besides RN)... 😫

@norato
Copy link

norato commented Dec 21, 2017

@igor-dv I can improve this gif

@igor-dv
Copy link
Member

igor-dv commented Dec 21, 2017

@norato it will be very helpful

@alterx
Copy link
Member Author

alterx commented Dec 21, 2017

Perfect, guys. Is any of these things happening in this PR? 🤔 @norato @igor-dv

@igor-dv
Copy link
Member

igor-dv commented Dec 21, 2017

Nope. Just merge 👍

@ndelangen ndelangen merged commit c97eaf9 into release/3.3 Dec 22, 2017
@ndelangen ndelangen deleted the angular-module-metadata branch December 22, 2017 00:19
@shilman shilman mentioned this pull request Dec 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants