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 Source View in Nextjs projects #21029

Merged
merged 3 commits into from
Feb 15, 2023

Conversation

valentinpalkovic
Copy link
Contributor

@valentinpalkovic valentinpalkovic commented Feb 9, 2023

Resolves #20356

What I did

I have set the parameters.docs.source.excludeDecorators to true per default.

How to test

  1. Run a sandbox for template, e.g. yarn task --task sandbox --start-from auto --template nextjs/default-ts
  2. Open Storybook in your browser
  3. Access Button Docs
  4. Click on "Show Code" and it should show the component code properly

Checklist

  • Make sure your changes are tested (stories and/or unit, integration, or end-to-end tests)
  • Make sure to add/update documentation regarding your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Maintainers

  • If this PR should be tested against many or all sandboxes,
    make sure to add the ci:merged or ci:daily GH label to it.
  • Make sure this PR contains one of the labels below.

["cleanup", "BREAKING CHANGE", "feature request", "bug", "documentation", "maintenance", "dependencies", "other"]

@JReinhold
Copy link
Contributor

JReinhold commented Feb 9, 2023

Was it default true in 6.5 as well and we just lost it at some point, or is this new?
If this is new I would consider this a breaking change that we need to mention in migration notes - and I think we should do it for all renderers if we want to do this.

Alternatively you could set this in the NextJS framework instead of the renderer, if it's only there it is causing trouble.

@valentinpalkovic
Copy link
Contributor Author

valentinpalkovic commented Feb 9, 2023

I have to use the react renderer. Next.js does not have its own renderer. I don’t know about the first part of your question :/

@JReinhold
Copy link
Contributor

I have to use the react renderer. Next.js does not have its own renderer.

I'm pretty sure you can set parameters in frameworks the same way you'd do it in renderes, via the preset.ts. In https://github.com/storybookjs/storybook/blob/next/code/frameworks/nextjs/src/preset.ts :

...

export const parameters = {
  docs: {
    source: {
      excludeDecorators: true,
    }
  } 
};

@valentinpalkovic valentinpalkovic force-pushed the valentin/fix-source-code-view-for-nextjs branch from 2d2b6e3 to d0ef23e Compare February 9, 2023 15:16
@valentinpalkovic
Copy link
Contributor Author

valentinpalkovic commented Feb 9, 2023

@JReinhold Changed, but unfortunately does not have any effect (see chromatic build).

@JReinhold
Copy link
Contributor

I'm still seeing weird sources in the published NextJS Storybook:

https://634ff0d0ec053b270775979d-zjgrspsocr.chromatic.com/?path=/docs/example-button--docs

image

Was this PR supposed to fix this, or am I missing something? Or maybe this only fixes it in dev mode?

@shilman
Copy link
Member

shilman commented Feb 10, 2023

@valentinpalkovic @JReinhold

  • Setting the parameters should happen in preview.tsx not preset.ts
  • We've never set excludeDecorators to true by default, but there's no reason we can't

@valentinpalkovic valentinpalkovic force-pushed the valentin/fix-source-code-view-for-nextjs branch 2 times, most recently from c6ee46c to 5028df5 Compare February 13, 2023 10:17
@valentinpalkovic
Copy link
Contributor Author

@shilman or @tmeasday May I get final approval? :)

Copy link
Member

@tmeasday tmeasday left a comment

Choose a reason for hiding this comment

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

Fine by me! But I think I am missing why we need to drop the decorators in next exactly? Perhaps a comment explaining why there's problematic decorators would help?

@valentinpalkovic valentinpalkovic force-pushed the valentin/fix-source-code-view-for-nextjs branch from 30e064e to 9543df1 Compare February 14, 2023 15:22
@tmeasday
Copy link
Member

@valentinpalkovic I re-enabled the test f5d5989 and it worked great!

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.

[Bug]: Show Code in Sample Components is not showing the component code (7.0.0-beta13 nextjs)
4 participants