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 docs "Show code" chooses generated template over user-defined template #14268

Closed
dexster opened this issue Mar 18, 2021 · 21 comments
Closed

Comments

@dexster
Copy link
Contributor

dexster commented Mar 18, 2021

Describe the bug
"Show code" in the Docs tab prioritises showing generated template over a template specified using the "template" property

To Reproduce
Steps to reproduce the behavior:

Create a story with a template

export const MySelector: Story = (args) => ({
  props: {
    label: foo
  },
  template: '<my-selector randomattribute [label]="label"></my-selector>'
});

Go to Docs and click Show code. The randomattribute will not be visible.

Expected behavior
My thinking is that anything defined by a user should be prioritised over a generated template as the user may want to include additional code in the template.

Also, if a component has multiple selectors, the generated template chooses the first selector (if this PR is accepted - #14230) whereas a template may specify a second or n selector which we would want to show in "Show code"

"Show code" should always show the user-defined template code when available

Code snippets
The sourceDecorator function in addons > frameworks > angular has this line. If the template and source are swapped then it fixes this
channel.emit(SNIPPET_RENDERED, context.id, prettyUp(source || template));

System
Using a fork of 6.2@beta14

@shilman
Copy link
Member

shilman commented Mar 18, 2021

@yngvebn is this something you can look into?

@yngvebn
Copy link
Contributor

yngvebn commented Mar 18, 2021

@shilman I'll take a look!

@dexster
Copy link
Contributor Author

dexster commented Mar 18, 2021

I have the code and am happy to submit a PR but just want to check whether my thinking is correct.

@yngvebn
Copy link
Contributor

yngvebn commented Mar 25, 2021

I have the code and am happy to submit a PR but just want to check whether my thinking is correct.

Please submit a PR. I'll be happy to take a look. Haven't had time to look at it myself 😊

@lacolaco
Copy link

#14343 (comment)

For now it will simply be easier to manually add the code for the View code

@dexster Could you share your workaround on how to add the source for the view? Thanks

@dexster
Copy link
Contributor Author

dexster commented Jun 28, 2021

@lacolaco Here you go Adding source code

@lacolaco
Copy link

@dexster Thanks a lot!

@stefan-schweiger
Copy link
Contributor

@dexster @shilman has there been any update on this?
Setting docs.source.type = 'code' just leads to "no code available" and the other options just generate the same incomplete code.

I could set docs.source.code manually, just having static code isn't worth the effort of adding it for every story.

@shilman
Copy link
Member

shilman commented Jul 28, 2021

@stefan-schweiger If setting the type to 'code' results in "no code available" then I think it's a problem with your setup. Do you have a reproduction? See how to create a repro

@stefan-schweiger
Copy link
Contributor

@shilman I've tried to make a repro, but yes there seems to be something up with my setup. But just showing the story source is only marginally more useful, because this was always an option with the storysource addon.

Is there anything in particular blocking us from implementing an additional option to just render out the template string with the args and showing it? I would be fine if this would not be the default behaviour but I think for more complex components this would be a useful feature.

@shilman
Copy link
Member

shilman commented Jul 29, 2021

@stefan-schweiger looks like @dexster tried to add it here #14343 but there was a bunch more work to do before it could be merged. if you can sync with him and @yngvebn and pick it up, i think that's a PR we'd merge.

@dexster
Copy link
Contributor Author

dexster commented Jul 29, 2021

This became more complex the more I looked at it and unfortunately, I don't have time to continue with this right now. Hopefully someday soon 😃

@stefan-schweiger
Copy link
Contributor

@shilman would introducing a new option docs.source.type = 'template' be too broad? I'm guessind this probably only applies to Angular, so I don't know if this is a bad idea because this would be a global change. But with something like this it would be pretty straight forward to implement.

@stefan-schweiger
Copy link
Contributor

@yngvebn @ThibaudAV maybe any thoughts on this since you were involved in the original PR by @dexster?

@shilman
Copy link
Member

shilman commented Aug 2, 2021

@stefan-schweiger there are other frameworks that also support a template, so it's not an outrageous idea. can you paste an example of a story using a template, some arg values, and the resulting string you'd like to show? 🙏

@stefan-schweiger
Copy link
Contributor

stefan-schweiger commented Aug 2, 2021

@shilman sure :) For example this very basic button component:

@Component({
  selector: 'my-button',
  template: `
    <button [class.primary]="color !== 'secondary'" [class.secondary]="color === 'secondary'" [disabled]="disabled">
      <ng-content></ng-content>
    </button>
  `,
  styles: [`
    .primary { background: red; }
    .secondary { background: blue; }
  `],
})
export class MyButtonComponent {
  @Input() public color: 'primary' | 'secondary' = 'primary';

  @Input() public disabled = false;
}

If you currently try to use this with a story like this:

export const ButtonWithIcon = ({ content, icon, ...args }) => ({
  props: args,
  template: `
    <div class="some-needed-container">
      <my-button [disabled]="${args.disabled}">
        <my-icon [icon]="${icon}"></my-icon>
        ${content}
      </my-button>
    </div>
  `,
});

ButtonWithIcon.args = {
  content: 'Args test',
  icon: 'user',
  disabled: false,
  color: 'secondary'
};

It gets rendered as:

<my-button color="primary" [disabled]="false"></my-button>

With the template option we should at least render it like this:

<div class="some-needed-container">
  <my-button [disabled]="true">
    <my-icon [icon]="'user'"></my-icon>
    Args test
  </my-button>
</div>

This is fairly straight forward with the additional template option, I've already created a POC and could submit a PR very soon.

Further considerations

The only thing I don't love about this is that input properties which aren't set in the template don't show up in the source code (in the example color) like they do when the source code gets rendered from the component.

I can think of 3 possible solutions for this

  • Don't let storybook handle it at all. When a user specifies type: 'template' he/she gets exactly that. Requires no additional work besides using the template for the source.

  • Create and expose a helper function which can be used by the users in the template string. This function would gets a list of properties and the context (which includes the doc information about input/output props) and does something similar to the code in computesTemplateSourceFromComponent to get a string of correctly set input/output props.

    Usage like template: '<div><my-button ${inputOutputProps(args, context)}>Test</my-button></div>'

  • Rework how computesTemplateSourceFromComponent works so the source code actually reflects the rendered component. Currently all input properties get applied to the first instance of the component. But it's hard to find the correct component and modify it in the template string. Probably even worse: figure out how to handle if the user already set the property in the template.

    This is actually how I think it should actually work, but this is more complicated to implement if it can be done at all for all possible edge cases.

@ThibaudAV
Copy link
Contributor

  • Don't let storybook handle it at all. When a user specifies type: 'template' he/she gets exactly that. Requires no additional work besides using the template for the source.

abolument not an expert on the subject but this solution seems good no?

@shilman
Copy link
Member

shilman commented Aug 2, 2021

@stefan-schweiger this is pretty nice. we could generalize it to every framework by allowing the user specify the template in the parameter:

ButtonWithIcon.parameters = {
  docs: {
    source: {
      type: 'template',
      template: `.....`
    }
  }
}

If the user sets the source.type to 'template', the implementation could use source.template if available or the story's template if not. WDYT?

@stefan-schweiger
Copy link
Contributor

@shilman I like it, but it feels somewhat redundant to source.type = 'code'. Maybe source.template should accept functions with args+context to make it more useful than just using code? (Even though I don't know if this is technically possible)

@shilman
Copy link
Member

shilman commented Aug 2, 2021

In general we avoid functions since they are difficult to serialize and process in integrations with other tools. Using a template string is easy and flexible and satisfies that constraint.

@shilman
Copy link
Member

shilman commented Aug 10, 2021

Crikey!! I just released https://github.com/storybookjs/storybook/releases/tag/v6.4.0-alpha.29 containing PR #15743 that references this issue. Upgrade today to the @next NPM tag to try it out!

npx sb upgrade --prerelease

Closing this issue. Please re-open if you think there's still more to do.

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

6 participants