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

refactor!: Story children no longer forwards args & storyContext & add new props - asChild and template #228

Open
wants to merge 15 commits into
base: next
Choose a base branch
from

Conversation

xeho91
Copy link
Collaborator

@xeho91 xeho91 commented Nov 8, 2024

Note

Concluded by feedback discussion thread on Discord


TODO

📦 Published PR as canary version: 5.0.0--canary.228.e697925.0

✨ Test out this PR locally via:

npm install @storybook/[email protected]
# or 
yarn add @storybook/[email protected]

@xeho91 xeho91 self-assigned this Nov 8, 2024
@xeho91 xeho91 added major Increment the major version when merged next Related to the next version labels Nov 8, 2024
@xeho91 xeho91 marked this pull request as ready for review November 10, 2024 06:37
@xeho91 xeho91 requested a review from JReinhold November 10, 2024 06:37
@xeho91
Copy link
Collaborator Author

xeho91 commented Nov 10, 2024

@JReinhold I need a second eye. I'm not sure if I have overlooked anything.

The only thing I didn't try to achieve in this PR is allowing passing a string to children arg - I believe this one needs a separate PR.

src/runtime/Story.svelte Outdated Show resolved Hide resolved
@xeho91 xeho91 changed the title refactor!: Story children becomes static & add template prop refactor!: Story children becomes static & add new props - asChild and template Nov 13, 2024
@xeho91 xeho91 changed the title refactor!: Story children becomes static & add new props - asChild and template refactor!: Story children no longer forwards args & storyContext & add new props - asChild and template Nov 13, 2024
@xeho91
Copy link
Collaborator Author

xeho91 commented Nov 13, 2024

@JReinhold
I might need your help.

Both are related to the Story source code.

Case 1

When there's a component specified in meta & and children provided.

Preview

<Story name="Long content">The very long content</Story>

Currently it displays:
image

And the expected output is:

<Button onclick={onClickFn}>The very long content</Story>

Case 2

Preview

This is a small "nit" pick on the visual details. New lines \n are converted to space.
image

Comment on lines 147 to 158
{:else if children && isSnippet(children)}
{#if asChild}
{@render children()}
{:else if renderer.storyContext.component}
<renderer.storyContext.component {...renderer.args}>
{@render children()}
</renderer.storyContext.component>
{:else}
{@render children()}
{/if}
{:else if storiesTemplate}
{@render storiesTemplate(renderer.args, renderer.storyContext)}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@dominikg Does this seem correct to you?

src/runtime/Story.svelte Outdated Show resolved Hide resolved
src/runtime/Story.svelte Outdated Show resolved Hide resolved
{@render children()}
{/if}
{:else if storiesTemplate}
{@render storiesTemplate(renderer.args, renderer.storyContext)}

Choose a reason for hiding this comment

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

does this include children in the args somewhere? (not sure if dealbreaker when not but what would happen with this?

{#snippet template(args)}
  <h1>button</h1>
  <Button {...args}/>
{/snippet}
<Story {template}>
  Hello!
</Story>

would Hello! be put in the children prop of Button in the template?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, it doesn't include - I'm testing it right now.

The following template will crash at runtime using examples/Button.stories.svelte, because it is trying to call {@render} on undefined children.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

However, if you provide children via meta, for example:

const { Story } = defineMeta({
  args: {
    children: createRawSnippet(() => ({ render: () => "Hello!" })),
  }
});

Then yes, the Hello! will be included.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think in this case, when users provide both template and children snippets... we should probably throw a documented error. WDYT?

At the current moment, I managed to create an error on the TypeScript level.

Choose a reason for hiding this comment

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

fine with template and children colliding in this case, as long as you can still forward children through args manually. An error thats helpful enough to prevent the user from doing this is good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
major Increment the major version when merged next Related to the next version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants