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

Svelte: Improved decorators #13785

Merged
merged 1 commit into from
Feb 10, 2021
Merged

Conversation

j3rem1e
Copy link
Contributor

@j3rem1e j3rem1e commented Feb 1, 2021

Issue: None

What I did

Today, writing decorators for svelte is hard:

  • A decorator must be a svelte component and the decorated story must be injected "by hand" with properties and a svelte:component/
  • It's not possible to use svelte context

In a "svelte world", data is passed to child through context, and a decorator has children with a <slot/>

This PR allow to use a syntax closest to Svelte.

A decorator component can be declared as:

<!-- MarginDecorator.svelte -->
<div>
  <slot/>
</div>

<style>
  div { margin: 3em; }
</style>

And a decorator for this component is just () => MarginDecorator.

It's possible to use Svelte context with () => { setContext('myKey', 'myValue') }

Moreover, this PR:

How to test

  • Is this testable with Jest or Chromatic screenshots? Done
  • Does this need a new example in the kitchen sink apps? Done
  • Does this need an update to the documentation? Done

Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

@j3rem1e is this a breaking change?

Also, if the user specifies Component in the story output, does it still get used? Also can you add it to MIGRATION.md? And, can you add a deprecation warning that it's no longer needed and reference MIGRATION.md?

Example: #13858

@shilman shilman changed the title Simplify Svelte decorators Svelte: Improve decorators Feb 10, 2021
@shilman shilman changed the title Svelte: Improve decorators Svelte: Improved decorators Feb 10, 2021
Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

Did not mean to approve this, just ask questions first.

@j3rem1e
Copy link
Contributor Author

j3rem1e commented Feb 10, 2021

@j3rem1e is this a breaking change?

No, it works like before, at least in my tests. Most of this PR is a rewrite of the renderer to use svelte component instead of "hacking internals", and to call the story function inside a svelte context.

Storyshots generates the same snapshots, and decorators implemented like in the documentation continue to work as before.

Also, if the user specifies Component in the story output, does it still get used?

Yes, it works like before.

Also can you add it to MIGRATION.md? And, can you add a deprecation warning that it's no longer needed and reference MIGRATION.md?

I am note sure to understand 😅 In svelte/storybook, without the "native svelte format" PR, it's almost always necessary to use in a story a component which is not the component defined in meta - because a story can't define a custom template like in vue or react. What I have done is to make this Component optional, and if it's not set, then I use the component defined in meta. It's not really deprecated.

I can add a message if the component in a story is the same component as in meta, but I don't think it's really a deprecation warning, as it will not be removed.

Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

Hi @j3rem1e thanks for the clarification. Merging!

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

Successfully merging this pull request may close these issues.

3 participants