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

fix: Temporarily disable save from UI feature #285

Merged
merged 6 commits into from
Mar 16, 2025
Merged

Conversation

xeho91
Copy link
Collaborator

@xeho91 xeho91 commented Mar 5, 2025

Resolves #240

@xeho91 xeho91 added patch Increment the patch version when merged next Related to the next version labels Mar 5, 2025
@xeho91 xeho91 requested a review from JReinhold March 5, 2025 05:06
@xeho91 xeho91 self-assigned this Mar 5, 2025
@xeho91
Copy link
Collaborator Author

xeho91 commented Mar 5, 2025

I wonder if we should also create a separate issue to restore this feature in the future. Whenever it is feasible to do so.

I mean, the part where Storybook addons are able to control/override what is being inserted into the original code of the stories file(s). This would need exploration on the Storybook core internals.

@JReinhold
Copy link
Collaborator

Nice! I think we can simplify it. Parameters don't have to be statically analysable (and they can't be, because they're very dynamic), so maybe we should be able to get away with this by just setting the parameter at runtime here: https://github.com/storybookjs/addon-svelte-csf/blob/next/src/runtime/create-runtime-stories.ts#L51

Which seems a lot simpler to me than the current AST-based approach?

(I know this context is a little late to give now, sorry 🙇)

I'm fine with the refactor you did to the AST parsing for the parameters/description though, if you still want to keep that around.

I wonder if we should also create a separate issue to restore this feature in the future. Whenever it is feasible to do so.

Yes we can do that.

@xeho91
Copy link
Collaborator Author

xeho91 commented Mar 6, 2025

Nice! I think we can simplify it. Parameters don't have to be statically analysable (and they can't be, because they're very dynamic), so maybe we should be able to get away with this by just setting the parameter at runtime here: https://github.com/storybookjs/addon-svelte-csf/blob/next/src/runtime/create-runtime-stories.ts#L51

Hm, I haven't though of that 🤔.
I'm a little puzzled by what you meant that parameters (I think we're talking about both in defineMeta and <Story>) are very dynamic?
In my mind, I'm stuck at example with inserting description either from JSDoc comment above defineMeta or HTML comment above every <Story />. I don't know if that's just one unique case.

(I know this context is a little late to give now, sorry 🙇)

No worries ❤️!
I should've communicated firstly how I planned to solve this solution. It seems like I got too comfortable solving through AST transformation, which can lead to overcomplicating 😆

@JReinhold
Copy link
Collaborator

I'm a little puzzled by what you meant that parameters (I think we're talking about both in defineMeta and <Story>) are very dynamic? In my mind, I'm stuck at example with inserting description either from JSDoc comment above defineMeta or HTML comment above every <Story />. I don't know if that's just one unique case.

By parameters I mean parameters that you can set on either meta or story. They can be anything, users are free to set whatever they want, and however they want - such as functions that calls other stuff or imports variables. The point is, they are so dynamic that Storybook will most likely never try to statically analyse them for whatever reason (that's actually why we introduced tags, a statically analysable version of parameters).

My point is, that there is no need to set the parameter directly in the source using AST, it's totally fine to do it at runtime, which I'd expect to be simpler here.

The problem with the description is that to get the description from the comments, you have to do static analysis with the AST, but when you have that information you don't need to set parameters.docs.description statically, it's fine to set it at runtime. But in this specific case with the description, given you're already doing AST stuff to read it, it might just be easier to also set it with the AST - that's up to you.

Am I making any sense? 😅

@xeho91
Copy link
Collaborator Author

xeho91 commented Mar 10, 2025

@JReinhold

Am I making any sense? 😅

Yes, thank you!

I've pushed the latest commit, with mutating meta inside the createRuntimeStories. However, I think the only way I can test it is manually.
I've noticed that neither AST solution, nor mutating at runtime works. I've used pnpm install to make sure my dependencies are up to date.

I also tried manually adding parameters.disableSaveFromUI: true to defineMeta, as well <Story parameters={{ disableSaveFromUI: true }} />.

I'm missing something, do you see it?

@JReinhold
Copy link
Collaborator

JReinhold commented Mar 12, 2025

Haha, it's really subtle in the docs, but it's actually parameters.controls.disableSaveFromUI 😅

https://storybook.js.org/docs/essentials/controls#parameters-1
https://storybook.js.org/docs/essentials/controls#disable-creating-and-editing-of-stories

@xeho91 xeho91 merged commit 91b2816 into next Mar 16, 2025
7 checks passed
@xeho91 xeho91 deleted the disable-save-from-ui branch March 16, 2025 07:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
next Related to the next version patch Increment the patch version when merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Creating stories from Controls fails in Svelte CSF with SyntaxError: Unexpected token, expected "}"
2 participants