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

[app/angular] Add ability to define component props binding in story. #2469

Closed
ralzinov opened this issue Dec 12, 2017 · 12 comments
Closed

[app/angular] Add ability to define component props binding in story. #2469

ralzinov opened this issue Dec 12, 2017 · 12 comments

Comments

@ralzinov
Copy link
Contributor

Issue details

Angular components can have 3 types of binding for props. Binding should be defined by template, like:

<component
  [label]="name"
  (onBlur)="callback($event)"
  [(ngModel)]="data"
></component

But currently story components are rendered dynamically and there is no way to define binding for props of story. I think something like this would be solution for that:

{
  component: SomeComponent,
  props: {
    label: 'some label'
    onBlur: () => {
       console.log('ping');
    },
    ngModel: {...},
    ngModelChange: () => {
      console.log('model change') 
   }
  },
  propsBinding: {
    label: 'input',
    onBlur: 'output',
    ngModel: 'input',
    ngModelChange: 'output'
  }
}
@alterx
Copy link
Member

alterx commented Dec 20, 2017

@ralzinov this is something we definitely want in @storybook/angular and I like this API.
I can't work on this right now but it's definitely in the top spot of my todo list.

Do you think you could take this proposal one step further and issue a PR? I'm more than willing to guide you if something doesn't make sense (although it looks like you're well versed in Angular :) )

@ralzinov
Copy link
Contributor Author

@alterx Thank you! I think i can do that. But that api seems to me little dirty solution, bindings rarely changing, what if do that with another decorator like this:

const decoratedComponent = withBindings(SomeComponent, {
    label: 'input',
    onBlur: 'output',
    ngModel: 'input',
    ngModelChange: 'output'
});

const someStory = {
   component: decoratedComponent,
   props: {
      ... 
   }
}

@alterx
Copy link
Member

alterx commented Dec 21, 2017

Hmmm, what would this bring to the table that the other approach (a simple object) wouldn't? 🤔
"Decorators" in Storybooks are actually add-ons that, for the most part, work across all the supported frameworks. I think it makes sense to declare all the things inside the story (the same way it makes sense to declare all the dependencies and things inside an NgModule when we create angular apps.

I'd lean towards something in the lines of the first example, like this:

{
  component: SomeComponent,
  props: {
    label: { value: 'some label', binding: 'input' },
    onBlur: { value: () => {
       console.log('ping');
    }, binding: 'output' },
    ...
  }
}

or even using 'tuples' (simple 2-item arrays that we can destructure) to avoid putting the name

{
  component: SomeComponent,
  props: {
    label: ['some label', 'input'],
    onBlur: [ () => {
       console.log('ping');
    }, 'output' ],
    ...
  }
}

this way we can also provide shorthands:

{
  component: SomeComponent,
  props: {
    label: 'some label',
    onBlur: () => {
       console.log('ping');
    },
    ...
  }
}

in the above example we could assume that a single string instead of an array means an @input prop or that a function instead of an array means an @output, for example. What do you think @ralzinov ?

@ralzinov
Copy link
Contributor Author

Mm its just less verbose and has better readability. I thought about decorators because you can decorate component once at top and use it in stories. You can also pass there other things ( f.e dependencies for ngModule) which don't change. It is more laconically than "propsBinding" property in every story and 1 and 2 methods.

Problem with 3 method can be if somebody want to pass a callback or class as input, or maybe JS objects like Date which also are functions.

What if functions would be recognized as outputs by default, but user can define bindings through decorator withMetadata which just set internal property to the component?
Later that decorator can be used to define component dependencies.

const decorated = withMetadata(component, {
   propsBindings: { ... },
   ngModuleMetadata: {
     imports: [ ... ],
     declarations [ ... ]
     ...
  }
})

I think nothing wrong if apps can have own decorators. What do you think? @alterx

@alterx
Copy link
Member

alterx commented Dec 21, 2017

I agree with you concerns with method # 3

Readability is somewhat subjective, what's readable for me it's not necessarily readable for others. And it just feels like the percentage of verbosity reduced isn't worth. However, decorating a component a single time and then using it in several stories does feel like a solid reason to lean towards this approach.
Also, I can see how adding this decorator would help separating the concerns between metadata, and the actual component and its data.

Ok, I think I've made my mind up. Let's give this approach a go and we can iterate over it once we have a working PR. Does that work for you, @ralzinov ?

@ralzinov
Copy link
Contributor Author

Ok, i will do that

@ndelangen
Copy link
Member

@ralzinov Could you join us on the storybook slack?
self invite link

@ndelangen ndelangen reopened this Dec 22, 2017
@ndelangen
Copy link
Member

WHOOPS didn't mean to close at all

@alterx
Copy link
Member

alterx commented Dec 27, 2017

Hey @ralzinov , what about a simpler API? Take a look at #269 (comment).

I think we could just allow users to provide a template. Given that we already have an NgModule it should be pretty straightforward 🤔
I'll create a PoC today after work

@ralzinov
Copy link
Contributor Author

ralzinov commented Dec 28, 2017

@alterx hmm i think its great idea, will experiment on weekend too. Found lib that can help us link. I think it can handle all dirty work with props, dependencies and bindings

@alterx
Copy link
Member

alterx commented Dec 28, 2017

This looks really promising @ralzinov , we could offload the tricky parts of dynamic component generation 🤔

@alterx alterx added the merged label Jan 11, 2018
@alterx
Copy link
Member

alterx commented Jan 11, 2018

The new template API should cover this requirement.
See #2690

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants