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

[Bug] Storybook 7 Controls doesn't change component within iframe unless manually trigger refresh #80

Closed
haleksandre opened this issue Nov 27, 2022 · 20 comments · Fixed by #99
Assignees
Labels
bug Something isn't working released This issue/pull request has been released.

Comments

@haleksandre
Copy link

Describe the bug

Controls values doesn't reflect on the component unless clicking manually the remount button or a full page refresh with args preset in url.

Reproduction repo: https://github.com/haleksandre/storybook7-svelte

Steps to reproduce the behavior

  1. Run storybook (npm run storybook)
  2. Go to button component
  3. Change controls values
  4. Component doesn't get refreshed (until manual clicking remount or full page refresh)

Expected behavior

  1. Run storybook (npm run storybook)
  2. Go to button component
  3. Change controls values
  4. Component get refreshed realtime with controls values

Environment

  • OS: Ubuntu
  • Node.js version: v18.12.1
  • NPM version: 8.19.2
  • Browser: Brave
@haleksandre haleksandre added the bug Something isn't working label Nov 27, 2022
@jnschrag
Copy link

@JReinhold It looks like this is the same/similar issue that I mentioned to you on Discord where changing the controls were not updating the component when using an "in between" component like in this example.

However, if I change it to follow this kind of pattern instead, changing the controls works:

<script>
  import Button from '../Button.svelte'

export let backgroundColor;
export let prefix;
export let suffix;
export let defaultSlot;
export let onClick;
// ...etc.
</script>


<Button {backgroundColor} on:click={onClick}>
  <svelte:fragment slot="prefix">
    {prefix} <!-- could add @html if html is inserted here as well -->
  </svelte:fragment>

  {defaultSlot}

  <svelte:fragment slot="suffix">
    {suffix} <!-- could add @html if html is inserted here as well -->
  </svelte:fragment>
</Button>

@JReinhold JReinhold self-assigned this Dec 16, 2022
@ZackMitkin
Copy link

I've created a reproduction of this issue here in this repo. Reproduction steps are in the README.md
https://github.com/ZackMitkin/storybook-svelte-csf-vite-example

@hallefsantos
Copy link

hey @haleksandre, any updates on this?

@JReinhold JReinhold moved this to Required for RC in Core Team Projects Feb 10, 2023
@shilman shilman moved this from Required for RC to Nice to have in Core Team Projects Feb 10, 2023
@vekunz
Copy link

vekunz commented Feb 13, 2023

This still happens with the currently latest versions of storybook (beta.46)

@alexlafroscia
Copy link

alexlafroscia commented Feb 17, 2023

I've noticed two other related issues (maybe it's actually the same issue, expressed two ways) with the way that Controls work with argTypes in the current Svelte handling (in my case, using beta.47).

These are not present in the version of Storybook used in the reproduction that @haleksandre provided; I took that reproduction and upgraded the Storybook packages to the version I have in my own app, to validate that the issue I'm seeing in my app is present there as well. Both of these issues are reproducible in @haleksandre's reproduction repo if Storybook is upgraded to beta.47 and @storybook/addon-svelte-csf updated to 3.0.0-next.3:

  • A defaultValue for an argument is not actually set as the default for an arg when used in the story; the story receives undefined as the value instead
  • The control does not default to displaying the right value if you haven't explicitly set a value; for example, this argType configures a defaultValue but the control does not reflect it

CleanShot 2023-02-17 at 12 44 30@2x

@ihym
Copy link

ihym commented Mar 8, 2023

still an issue in beta.62

@jacksmethurst
Copy link

Having the same issue in (@storybook/addon-svelte-csf: 3.0.0-next.4)

@mantismamita
Copy link

Hello, with @storybook/sveltekit 7.0.0-rc.9 and @storybook/addon-svelte-csf: 3.0.0-next.4 controls don't work even after the page is refreshed. I noticed the control object has changed place in argTypes but this only makes the controls visible but doesn't allow usage.

@leika
Copy link
Contributor

leika commented Apr 5, 2023

Had the same issue with controlls not refreshing the component/story after args changed. Created a PR that fixed this for me #99

Essentially, the Wrapper Component would need to react on $$props change and recreate the Story Component. No iframe refresh needed in those cases.

@mantismamita
Copy link

Essentially, the Wrapper Component would need to react on $$props change and recreate the Story Component. No iframe refresh needed in those cases.

It would amazing if the iframe didn't refresh as it triggers an additional and unnecessary onMount which can complicate testing.

@leika
Copy link
Contributor

leika commented Apr 5, 2023

With the Change I'm testing, the iframe doesn't refresh. The Story-proxy is remounted which will reflect the change.

This was the easiest fix for now.

@leika
Copy link
Contributor

leika commented Apr 5, 2023

The other fix would be to wrap the args context into a writeable and make sure Template and Story components are correctly react on changes to tis Store

@mantismamita
Copy link

The other fix would be to wrap the args context into a writeable and make sure Template and Story components are correctly react on changes to tis Store

IMO, it should reflect the behaviour of the component outside of an iframe. The store option seems like a good idea.

@leika
Copy link
Contributor

leika commented Apr 5, 2023

Agree, need to look into this. We'd need stores for args and context in this case (otherwise changes to context.globals would equally be lost, which is important in case someone uses global toolbar buttons for theme switching or similar)

@leika
Copy link
Contributor

leika commented Apr 5, 2023

I did update the PR now, using stores and also a clean context at render time. I think that's very close to vanilla svelte behaviour.

0625703
.... Works also on svelte-kit apps when SB7+ is embeded via the install wizard

@bjornet
Copy link

bjornet commented Apr 17, 2023

I also encountered this issue – where controls do not update the canvas on change. I tried @leika:s solution. It worked well, looking forward for this to get into main 👍🏽

@gbkwiatt
Copy link

Is there a reason why it's still not fixed ? does it need testing power or something ? It really is the only thing that current storybook is missing (for svelte)

@shilman
Copy link
Member

shilman commented Apr 21, 2023

🚀 Issue was released in v3.0.2 🚀

@shilman shilman added the released This issue/pull request has been released. label Apr 21, 2023
@miedzikd
Copy link

miedzikd commented May 8, 2023

Hey! Does anyone still experience this issue? After changing the component parameters, the component is not unmounted, so if any variables inside the component rely on the attribute value, they won't be refreshed...

I have prepared a minimal reproduction ( https://stackblitz.com/edit/storybook-svelte-bug?file=README.md,assets%2Fsvelte%2Fui%2Fatoms%2FButton%2FButton.svelte ). Please note that the button component has 2 parameters:

  • The first one, color does not refresh because the classes variable is built based on it. In previous versions of Storybook, this also used to refresh.
  • The change of the second parameter, disabled" is immediately visible because it is directly used in the HTML code.

So is this how Storybook Svelte works, and is it the intended behavior? Do I need to change all the components? Or is this considered incorrect behavior?

@JReinhold
Copy link
Collaborator

Hey! Does anyone still experience this issue? After changing the component parameters, the component is not unmounted, so if any variables inside the component rely on the attribute value, they won't be refreshed...
So is this how Storybook Svelte works, and is it the intended behavior? Do I need to change all the components? Or is this considered incorrect behavior?

@miedzikd this is indeed intended behavior. In Storybook 7 we fixed a bug, where changing any arg would remount the whole component. Not only was that slow, it also made it impossible for users to test how their components react to changes to props. So now changing args will trigger props changes, not full remounts.
And since your Button doesn't expect the color to ever change - and only supports setting the color on initial render - your button doesn't change color.
In general I'd advice against that pattern, because the consumer of your Button might expect it to support changing props, but you can best judge that yourself.
For completion, I rebuild your example in the Svelte REPL to make sure it was also an issue outside of Storybook: https://svelte.dev/repl/98db30686b304b1c9a3dc8b746b1be77?version=3.59.1

As a workaround you can wrap your Button within the Story with the {#key} syntax, that should remount your whole component whenever the key changes:

<Story name="Button" let:args>
    {#key args.color}
        <Button on:click={handleClick} {...args}>
            Count: {count}
        </Button>
    {/key}
</Story>

There might be edge cases where this workaround fails, I haven't tested it extensively.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working released This issue/pull request has been released.
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.