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

Angular: Improve how props are used in the story #12438

Closed
Marklb opened this issue Sep 10, 2020 · 16 comments
Closed

Angular: Improve how props are used in the story #12438

Marklb opened this issue Sep 10, 2020 · 16 comments

Comments

@Marklb
Copy link
Member

Marklb commented Sep 10, 2020

This ended up being a lot longer than I planned, so if you already know how the current Angular stories are written, you can probably just skim through the part before "Proposed Solutions".

This has been brought up multiple times, but I haven't seen any issue or discussion focused on it.

The way stories are written for Angular worked the way I would expect, until the introduction of auto-generated controls. Now, I have noticed that I often have maybe half the Controls in my ArgTables working. New users that aren't familiar with writing Angular stories before Controls were introduced or don't have an understanding of the framework's story implementation, seem to have trouble understanding why their controls aren't applied.

Before auto-generated Controls, I would only add the Knobs that were related to the story. So, it was easy to maintain the inputs/outputs that are expected to work for a story, but many of the inputs/outputs were ignored, because it was time consuming to maintain a Knob/Action for every input/output. Now Storybook can just add a Control/Action for each input/output automatically, but connecting them to the component isn't as simple.

In React its simple to add all the args to a component, because of the spread operator in JSX, but a property in Angular can do much more than just set an input/output, so a spread operator of properties doesn't really make sense. If some of the additional features, such as using a property in a Component/Directive selector or two-way-binding, are ignored, there may be a solution that is fairly close to spreading the args in JSX.

I'm sure there is a way to improve the way props are used, but I am not sure if it is a feature we can add to the framework or just a clean pattern we can recommend in the Storybook documentation.

Depending on the solution decided, this may provide a way to improve other frameworks, as well. I don't know much about Vue's syntax, but I think I have seen similar questions asked for it on Discord.

I will provide an example of the current solution first. Then I will list my ideas and their pros/cons, to see if anyone has a way to improve one of mine or has a different solution entirely.


Current Solution

The following assumes the project has configured Storybook to auto-generate a Control for inpOne and inpTwo.

@Component({ selector: 'story-example', template: '' })
class StoryExampleComponent {
	@Input() inpOne: number
	@Input() inpTwo: string
}

export default {
	title: 'Example',
	component: StoryExampleComponent,
	decorators: [
		moduleMetadata({
			declarations: [ StoryExampleComponent ]
		})
	]
} as Meta

// 1. Using `component`.
export const ExampleOne: Story = (args) => ({
	props: { ...args },
	component: StoryExampleComponent
})

// 2. Using `template`.
export const ExampleTwo: Story = (args) => ({
	props: { ...args },
	template: `<story-example [inpOne]="inpOne" [inpTwo]="inpTwo"></story-example>`
})
  1. Using component is fine for simple components and the props will be set on the component instance for you. I don't like writing my stories that way though, because majority of the time I use components in a template in my application. This way also doesn't provide a way to set ng-content that I can see.

  2. Using template is how I prefer to write my stories, because that is how I most likely use the component in my application. The downside is that I have to set the properties on my component in the template, because Storybook can't predict which component instance it should be setting the properties on.

    One confusion I see though is why the props aren't changing when <story-example></story-example> is used for the template. Sure, at first glance I may assume Storybook should be able to find that component instance and set the props for me. The way Storybook uses the template is by turning it into a component. Example: @Component({ template }) class DynamicComponent {}.

    To make it clear why Storybook can't just assume the child component, that is an instance of our story component, is where it should set the props. What if the template is <story-example></story-example><story-example></story-example>? Should both components get the same props or individual controls for each?
    I think it's clear why that wouldn't "just work", but one more example: (args) => ({ props: { list: [...new Array(Math.floor(Math.random() * 1000))], ...args }, template: `<story-example *ngFor="let x of list"></story-example>` }).


Proposed Solutions

1. Helper function to output component properties as string

I have never tried something like this in Angular, but I don't see any obvious reason this couldn't work.

import { toComponentProps } from '@storybook/angular'
(args) => ({
	props: { ...args },
	template: `<story-example ${toComponentProps(args)}></story-example>`
})

Pseudo code of toComponentProps:

function toComponentProps(args: { [key: string]: any }): string {
	const inputs = argsfilterInputs(args)// Use SB api to know what args are inputs.
	const outputs = argsfilterOutputs(args)// Use SB api to know which args are outputs.
	const twoWay = argsfilterTwoWayBound(args)// Use SB api to know which args are two-way-bound.
	const props: string[] = []
	Object.keys(inputs).forEach(k => props.push(`[${k}]="${k}"`))
	Object.keys(outputs).forEach(k => props.push(`(${k})="${k}"`))
	Object.keys(twoWay).forEach(k => props.push(`[(${k})]="${k}"`))
	return props.join(' ')
}

const props = { thing: 'one', enabled: () => action('enabled') }
const compProps = toComponentProps(props)
// compProps: '[thing]="thing" (enabled)="enabled"'

Pros:

  • Should result in code that matches the code a developer would type.
  • Doesn't rely on Angular for the implementation.

Cons:

  • May not actually be as simple as an independent helper function to access the right Storybook api calls needed to filter the prop types.
  • May not be easy to provide meaningful errors/warnings. One example could be unexpected duplicate props. Ex: () => ({ props: { thing: 'one' }, template: `<story-example ${toComponentProps({ thing: 'one' })} [thing]="thing"></story-example>` }) would result in the template <story-example [thing]="thing" [thing]="thing"></story-example>.
  • May not be straight forward to update individual props without a rerender.

2. Storybook Props Directive

  1. Simple (Not very flexible)
(args) => ({
	props: { ...args },
	template: `<story-example sbProps></story-example>`
})

Pseudo code of directive implementation:

@Directive({ selector: 'sbProps' })
class StorybookPropsDirective {
	constructor(
		@Self() private _view: ViewContainerRef,
		@Inject(STORY) private data: Observable<StoryFnAngularReturnType>
	) { }

	ngOnInit() { data.subscribe(v => this.setProps(this.componentInstance, v)) }

	get componentInstance() { return (<any>this._view)._element.component }

	setProps(componentInstance, props) { ... }
}
  1. Specify the props
(args) => ({
	props: { args },
	template: `<story-example [sbProps]="args"></story-example>`
})

Pseudo code of directive implementation:

@Directive({ selector: 'sbProps' })
class StorybookPropsDirective {
	@Input() set sbProps(value) { this.setProps(this.componentInstance, value) }

	constructor(@Self() private _view: ViewContainerRef) { }

	get componentInstance() { return (<any>this._view)._element.component }

	setProps(componentInstance, props) { ... }
}

Pros:

  • Single property to let Storybook know which component to set the props on.
  • Since the logic is in Angular, props could probably be individually updated without a story rerender.

Cons:

  • Source displayed in docs will not be code that could be copied into an app as a starting snippet.
  • May not be simple to match Angular's lifecycle and change detection triggering.

If something is decided, I should be able to help get it implemented.

@shilman
Copy link
Member

shilman commented Sep 10, 2020

@kroeder @gaetanmaisse can you please help out with this?

@giniedp
Copy link

giniedp commented Sep 11, 2020

I faced the same problem and and my solution is as follows:

Lets say we have this greeter module

// greeter.component.ts
@Component({
  selector: 'greeter',
  template: `Hello {{who}} `
})
class GreeterComponent {
  @Input()
  public who = "world"
}

// greeter.module.ts
@NgModule({
  imports: [CommonModule],
  declarations: [GreeterComponent],
  exports: [GreeterComponent],
})
export class GreeterModule {}

Now the stories

// greeter.stories.ts
// this is all common angluar DSL
@Component({
  selector: 'greet-basics-story',
  template: `
    <greeter></greeter>
  `
})
class GreetBasicsStory {

}

@Component({
  selector: 'greet-someone-story',
  template: `
    <input [(ngModule)]="buddy">
    <greeter who="buddy"></greeter>
  `
})
class GreetSomeoneStory {
  public buddy: string
}

@NgModule({
  imports: [GreeterModule],
  declarations: [GreetBasicsStory, GreetSomeoneStory],
  exports: [GreetBasicsStory, GreetSomeoneStory],
})
class GreeterStoriesModule {}

// this is all storybook interface
export default {
  title: 'Greeter Stories',
  decorators: [
    moduleMetadata({
      imports: [GreeterStoriesModule],
    }),
  ],
}

export const greeterBasics = () => ({
  component: GreeterBasicsStory,
})

export const greetSomeone = () => ({
  component: GreetSomeoneStory,
})

This obviously has a lot of Angular boilerplate. However, that is not necessarily a bad thing. For me it solves the following issues which i initially had when trying to squeeze angular into storybook.

  • Angular dependencies are clear to someone who "speaks Angular"
  • Source code samples make much more sense, as it is all Angular DSL
  • Type safety is given.
  • No more issues when Angular components have @input("withAlias")
  • Minimal contact point to storybook library.
  • Functional example components even without storybook (if i ever want to port them to another system)

For me, the interface into storybook could be a little less verbose. Here is some pseudocode

+ export default AngularStories('Greeter Stories', GreeterStoriesModule)
- export default {
-   title: 'Greeter Stories',
-   decorators: [
-     moduleMetadata({
-       imports: [GreeterStoriesModule],
-     }),
-   ],
- }

+ export const greeterBasics = AngularStory(GreeterBasicsStory)
- export const greeterBasics = () => ({
-   component: GreeterBasicsStory,
- })

+ export const greetSomeone = AngularStory(GreetSomeoneStory)
- export const greetSomeone = () => ({
-   component: GreetSomeoneStory,
- })

i'll probably write some utility functions to accomplish that.

The cons:
since the contact point is so little, storybook does know little about your components. So this cuts out other storybook features.

@Marklb
Copy link
Member Author

Marklb commented Sep 12, 2020

@giniedp I'm not sure if what you mentioned is the same as the problem this issue is focused on, so it may be better in it's own issue to avoid mixing the topics, unless I am just missing the connection. I think you are talking about Storybook's CSF format, right?

When you say Angular dependencies are clear to someone who "speaks Angular" are you referring to the additional @Component and @NgModule classes that you added?

As for the source code samples making more sense, I don't think I see what you mean. For the majority of my stories, I don't think the source needs anything more than a simple usage example, <my-component></my-component> or if it has inputs <my-component [inp1]="value" [inp2]="value2"></my-component>.

Types for stories is still fairly new and could be improved I'm sure, but I added Meta and Story to my snippet, which I had left off. If you mean Angular's types, I try to make my stories simple enough that I can avoid as much Angular code as possible.

If you are having problems with renamed input bindings (@Input("withAlias")), I would probably consider that I bug.

For the more complicated ones that need additional code to build an example, I do basically what you did, defining components in the stories file.

By making the code less verbose, by changing to something like your pseudocode AngularStories and AngularStory, I think it would end up making Story definitions more complicated in the end.
What I like about the way the stories are defined with CSF is how the code looks almost the same across all frameworks. The only difference is a few minor things, like the moduleMetadata decorator and a few properties like component, template, and styles in the Story type. Framework specific functions make the framework seem more important to the story definition than it actually is, in my opinion.

@giniedp
Copy link

giniedp commented Sep 12, 2020

I'm not sure if what you mentioned is the same as the problem this issue is focused on, so it may be better in it's own issue to avoid mixing the topics, unless I am just missing the connection. I think you are talking about Storybook's CSF format, right?

I think we are trying to solve the same problem. Sorry for not being clear as how my post relates to your proposals.

My concern is same as yours: how properties are passed to a angular components. I wanted to show that there is a need to improve the current system and how i go around that right now.

I am not happy with the outcome to 100% since the solution cuts out a lot from storybook features. But in general i prefer reading Angular DSL in a code sample instead of Storybook DSL.

Your proposals do minimize the Storybook DSL and i welcome that.

When you say Angular dependencies are clear to someone who "speaks Angular" are you referring to the additional @component and @NgModule classes that you added?

Yes, exactly. I mean, we see this block of DSL in the code sample anyway, because it is part of the story declaration

export default {
  title: 'Greeter Stories',
  decorators: [
    moduleMetadata({
      imports: [GreeterModule],
      declarations: [GreetBasicsStory, GreetSomeoneStory],
    }),
  ],
}

I prefer reading the angular module instead which is more natural to an angular dev.

@NgModule({
  imports: [GreeterModule],
  declarations: [GreetBasicsStory, GreetSomeoneStory],
})
class GreeterStoriesModule {}

As for the source code samples making more sense, I don't think I see what you mean. For the majority of my stories, I don't think the source needs anything more than a simple usage example

I partially agree. The main thing we want to see is the template code needed to render a component. The secondary thing is the typescript of the host component.

For example https://ng-bootstrap.github.io/ or https://material.angular.io/components or others do have html and typescript tabs in their code samples.

Types for stories is still fairly new and could be improved I'm sure, but I added Meta and Story to my snippet, which I had left off. If you mean Angular's types, I try to make my stories simple enough that I can avoid as much Angular code as possible.

When i wrote my first angular story it was like this

export const basicStory = () => ({
  component: MyComponent,
  props: {
    input: 'foo'
  }
})

Here props are not type checked against MyComponent. Not a big problem, it can be solved in different ways. for example

export const basicStory = ngStory({
  component: MyComponent,
  props: {
    input: 'foo'
  }
})

and ngStory being something like

function ngStory<T>(story: { type: Type<T>, props: PropertiesOf<T> }) {
  return () => story
}

However, i can assign props that are not decorated as @Input. And as i faced problems with aliased inputs i skipped the props completely and wrote my stories as angular components.

By making the code less verbose, by changing to something like your pseudocode AngularStories and AngularStory, I think it would end up making Story definitions more complicated in the end.

That depends on the final solution. The pseudocode is just to make my current solution less verbose.

What I like about the way the stories are defined with CSF is how the code looks almost the same across all frameworks

As a consumer of a library i am not interested in the storybook binding. I want to see relevant code to the framework i am working with.

Framework specific functions make the framework seem more important to the story definition than it actually is, in my opinion.

Agree. But same is for Storybook framework specific functions.

@stale
Copy link

stale bot commented Oct 4, 2020

Hi everyone! Seems like there hasn't been much going on in this issue lately. If there are still questions, comments, or bugs, please feel free to continue the discussion. Unfortunately, we don't have time to get to every issue. We are always open to contributions so please send us a pull request if you would like to help. Inactive issues will be closed after 30 days. Thanks!

@stale stale bot added the inactive label Oct 4, 2020
@shilman shilman added the PN label Oct 5, 2020
@stale stale bot removed the inactive label Oct 5, 2020
@Marklb
Copy link
Member Author

Marklb commented Oct 8, 2020

I haven't had a chance to really get into this, but the following is my very simple naive attempt. It isn't close to optimal yet, but does partially work.

First attempt

It adds inputs, if they have a control and args value, but they don't display in the source.

It identifies outputs, but I don't think it can add actions.

import { ArgTypes } from '@storybook/addons'

/**
 * This is an attempt at simplifying the use of auto-generated args in stories
 * defined with `template`, since Angular doesn't have a way to simply use a
 * spread operator syntax.
 *
 * @experimental
 */
export function argsToTpl(args: any, argTypes: ArgTypes) {
  // console.log({ args, argTypes })
  let s = ''

  Object.keys(argTypes).forEach(k => {
    // Inputs
    if (
      // Is in the inputs category
      argTypes[k].table.category === 'inputs' &&
      // Needs a control to be able to change from auto-generated args.
      argTypes[k]?.hasOwnProperty('control') &&
      // Assuming the arg might not be in props if there isn't an arg value.
      args.hasOwnProperty(k)
    ) {
      s += `[${k}]="${k}" `
    }

    // Outputs
    if (
      // Is in the outputs category
      argTypes[k]?.table?.category === 'outputs'
    ) {
      // Without access to props, I don't know if I can get an action into the
      // template context like this.
      // if (argTypes[k].table.category === 'inputs') {
      //   s += `(${k})="${k}" `
      // }
    }
  })

  return s
}

// Story
export const Example1 = (args, { argTypes }) => ({
  props: args,
  template: `<example-one ${argsToTpl(args, argTypes)}></example-one>`
})

Second attempt

It adds inputs, if they have a control and args value, but they don't display in the source.

It adds outputs as actions.

This one can do outputs, but the story is a little more complicated to write.

import { action, HandlerFunction } from '@storybook/addon-actions'
import { ArgTypes } from '@storybook/addons'

export interface ArgsTplParts {
  actions: { [prop: string]: HandlerFunction }
  tplfragment: string
}

/**
 * This is an attempt at simplifying the use of auto-generated args in stories
 * defined with `template`, since Angular doesn't have a way to simply use a
 * spread operator syntax.
 *
 * @experimental
 */
export function argsToTplParts(args: any, argTypes: ArgTypes): ArgsTplParts {
  // console.log({ args, argTypes })
  const parts: ArgsTplParts = {
    actions: {},
    tplfragment: ''
  }

  Object.keys(argTypes).forEach(k => {
    // Inputs
    if (
      // Is in the inputs category
      argTypes[k].table.category === 'inputs' &&
      // Needs a control to be able to change from auto-generated args.
      argTypes[k]?.hasOwnProperty('control') &&
      // Assuming the arg might not be in props if there isn't an arg value.
      args.hasOwnProperty(k)
    ) {
      parts.tplfragment += `[${k}]="${k}" `
    }

    // Outputs
    if (
      // Is in the outputs category
      argTypes[k]?.table?.category === 'outputs'
    ) {
      parts.tplfragment += `(${k})="${k}($event)" `
      parts.actions[k] = action(k)
    }
  })

  return parts
}

// Story
export const Example2 = (args, { argTypes }) => {
  const { actions, tplfragment } = argsToTplParts(args, argTypes)
  return {
    props: { ...args, ...actions },
    template: `<example-one ${tplfragment}></example-one>`
  }
}

@manuelmeister
Copy link
Contributor

I think both attempts have too much overhead. I agree with @giniedp on this matter. Storybook and React together feel so seamless, but Storybook with Angular is so inconsistent, has a lot of overhead and a lot of missing documentation.

@shilman
Copy link
Member

shilman commented Oct 27, 2020

Disclaimer: I'm not familiar enough with Angular to process the suggestions here, so I'll let everybody here and the Storybook Angular maintainers like @kroeder @Marklb come to consensus on the best path forward.

That said, I think there are a few different goals here that might merit different solutions:

  1. The story format should be efficient and Angular-friendly.
  2. The story format should be Storybook CSF or compile to it, otherwise Storybook for Angular will be hard to maintain and diverge from the rest of Storybook.
  3. The code samples should be useful for people consuming the components.

Goals 1 and 2 may present some tension and I think there's probably things we can do here to make it better like #8673 or some of the suggestions here.

Goal 3 is best solved by #10617, which is what we've already done for React #11332 and Vue #12812. The args-stories are now parameterized, which is great for making them interactive, for combinatorial testing, etc. but makes them not great for examples. However, if we render the examples based on the selected arg values, we can create concise useful examples regardless of the story format.

@stale
Copy link

stale bot commented Dec 25, 2020

Hi everyone! Seems like there hasn't been much going on in this issue lately. If there are still questions, comments, or bugs, please feel free to continue the discussion. Unfortunately, we don't have time to get to every issue. We are always open to contributions so please send us a pull request if you would like to help. Inactive issues will be closed after 30 days. Thanks!

@stale stale bot added the inactive label Dec 25, 2020
@manuelmeister
Copy link
Contributor

What is needed next for this issue, after the current changes to storybook and its angular integration (6.2)?

@Frikki
Copy link

Frikki commented Apr 26, 2021

Yea, what’s happening with the issue? How are we moving forward?

@shilman
Copy link
Member

shilman commented Apr 27, 2021

Dynamic snippet rendering now exists for angular thanks to @yngvebn which is a huge step forward. Now we don't need to be concerned as much with the readability of the story source code, since the source is generated on the fly: #13740

As for efficiently filtering out the props, we could do something like:

export const Template = (args, { argTypes }) => ({
  props: getProps(args, argTypes),
  ...
});

Where getProps might look something like:

Object.fromEntries(
  Object.entries(args).filter(([k, v]) => (
    argTypes[k]?.table?.category === 'properties'
  )
);

WDYT?

@Marklb
Copy link
Member Author

Marklb commented Jul 7, 2021

Most of what I was trying to achieve with this issue have been solved by Storybook's current Angular renderer or will be once their bugs have been fixed.

Since the only remaining scenarios I can think of are more unique than what Storybook should worry about maintaining, I will go on and close this issue.

@Marklb Marklb closed this as completed Jul 7, 2021
@stefan-schweiger
Copy link
Contributor

@Marklb for custom templates having some kind of helper function or a better way to render additional properties would still be a great addition. The fix for #14268 introduced the option to always show the template which is great, but if you don't set every property by hand they are missing from the template/source but can still be set via Controls and Table.

@JamesIves
Copy link

Dynamic snippet rendering now exists for angular thanks to @yngvebn which is a huge step forward. Now we don't need to be concerned as much with the readability of the story source code, since the source is generated on the fly: #13740

As for efficiently filtering out the props, we could do something like:

export const Template = (args, { argTypes }) => ({
  props: getProps(args, argTypes),
  ...
});

Where getProps might look something like:

Object.fromEntries(
  Object.entries(args).filter(([k, v]) => (
    argTypes[k]?.table?.category === 'properties'
  )
);

WDYT?

This thread is old, but I wanted to share that using this function in combination of the @open-wc/lit-helpers spread directive for web components has saved me so much boilerplate in a Lit configured Storybook.

@shilman
Copy link
Member

shilman commented Jan 12, 2024

@JamesIves this is an angular thread. can you elaborate on your point in a separate issue or discussion? If there's something we can do to improve @storybook/web-components or our documentation on best practices for using it, I'd love to get it into the product or docs. thanks you! 🙏

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

7 participants