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]: Components as children of Canvas has Show Code without line breaks #21795

Closed
nstuyvesant opened this issue Mar 28, 2023 · 13 comments
Closed

Comments

@nstuyvesant
Copy link

Describe the bug

Perhaps this is my misunderstanding or I need a property on the Canvas but I have a number of examples in an MDX like this where I do not want to generate a story...

### Default (black border, no href, not stacked)
<Canvas style={{ background: '#fff' }}>
	<CardNode>
		<CardNodeColumn>
			<CardNodeTitle>Title</CardNodeTitle>
			<CardNodeSubtitle>Description</CardNodeSubtitle>
		</CardNodeColumn>
	</CardNode>
</Canvas>

When I hit the "Show Code" button on the lower right, it displays this as a single row with the newlines in handlebars instead of the way I entered it in the MDX...

<CardNode><CardNodeColumn><CardNodeTitle>{"Title"}</CardNodeTitle>{"\n"}<CardNodeSubtitle>{"Description"}</CardNodeSubtitle></CardNodeColumn></CardNode>

I have to use the prior version of MDX because my storybook still has other stories based on storiesOf(). So I have this property set in my .storybook/main.ts...

	features: {
		storyStoreV7: false
	}

To Reproduce

Take example above

System

Environment Info:

  System:
    OS: macOS 13.2.1
    CPU: (10) arm64 Apple M1 Pro
  Binaries:
    Node: 18.15.0 - /opt/homebrew/bin/node
    Yarn: 3.5.0 - /opt/homebrew/bin/yarn
    npm: 9.6.2 - /opt/homebrew/bin/npm
  Browsers:
    Chrome: 111.0.5563.110
    Firefox: 111.0.1
    Safari: 16.3
  npmPackages:
    @storybook/addon-essentials: ^7.0.0-0 => 7.0.0-rc.8 
    @storybook/addon-interactions: ^7.0.0-0 => 7.0.0-rc.8 
    @storybook/addon-links: ^7.0.0-0 => 7.0.0-rc.8 
    @storybook/blocks: ^7.0.0-0 => 7.0.0-rc.8 
    @storybook/manager-api: ^7.0.0-0 => 7.0.0-rc.8 
    @storybook/react: ^7.0.0-0 => 7.0.0-rc.8 
    @storybook/react-vite: ^7.0.0-0 => 7.0.0-rc.8 
    @storybook/testing-library: ^0.0.14-next.1 => 0.0.14-next.1 
    @storybook/theming: ^7.0.0-0 => 7.0.0-rc.8

Additional context

No response

@shilman
Copy link
Member

shilman commented Mar 29, 2023

I think the preferred approach is that you put those examples in stories and we will provide ways to designate stories as "docs only" in 7.1.

In the meantime, I'm not sure the best way to work around this. @JReinhold can you please help out here?

@JReinhold JReinhold changed the title [Bug]: Storybook 7.0.0-rc.8 MDX / Show Code - line breaks are gone [Bug]: Components as children of Canvas has Show Code without line breaks Mar 29, 2023
@nstuyvesant
Copy link
Author

A little background on the problem I was trying to solve... take a look at the DIAGRAMS section on this storybook...
https://charts.carbondesignsystem.com/angular/?path=/story/diagrams-edges-marker--arrow-left

If you expand Nodes/Card, Nodes/Shape, Edges, and Edges/Marker, there are 28 stories generated by data using storiesOf() in a single stories.ts file. None of these stories provide real value other than showing an example without code.

Instead of creating 28 CSFs, I'd like to have 4 x MDXs show in the navigation: Cards, Shapes, Edges, Markers and have the variations displayed in Canvas nodes on each of those MDXs with the little Source code tab for each canvas mixed with a little explanatory text at the top.

Moving to 4 x MDXs is a 400% increase in the number of files from the current approach but having the ability to display the source for each example makes it more valuable.

But moving to 28 x CSFs plus 4 x MDXs just for the DIAGRAMS section is problematic. Also, we still have all the other stories in UTILITY, SIMPLE CHARTS, and COMPLEX CHARTS sections that are generated with a single stories.ts via storiesOf() and a big array that would need to be created as separate files times the number of environments (5): Vanilla JavaScript (storybook html), Angular, React, Svelte and Vue.

Hope that perspective helps.

@JReinhold
Copy link
Contributor

JReinhold commented Mar 29, 2023

This is a nasty bug produced by how the output of MDX2 interacts with @storybook/mdx2-csf.

Reproduced here: https://stackblitz.com/edit/github-nghoh2?file=src/stories/broken-source.stories.mdx

I believe the broken source that is generated, is the JSX that the MDX2 compiler actually outputs from the file. A hint to that is the extra <p> tag added to the source in my reproduction, which is something only the MDX2 compiler would do.
And @storybook/mdx2-csf takes whatever it gets from the output and sets that as the mdxSource prop on Canvas.

I'm unsure what to do about this, other than giving @storybook/mdx2-csf the raw MDX source instead of the compiled output, but I can only imagine that the architecture wouldn't support that at all.

The reproduction also shows that if you try to manually specify the mdxSource prop, the result will actually be a concatenation of the prop and the generated source from the Canvas children, which is not usable either.
I would consider this a bug here: https://github.com/storybookjs/mdx2-csf/blob/051f37c627fb8f219d3328a3deb0cfd28ddfc2cc/src/index.ts#L139
It shouldn't add an mdxSource prop if there's already one there. @shilman

Finally, the reproduction shows that manually specifying mdxSource works as intended in plain .mdx files and is only a problem in .stories.mdx files, so a workaround would be to use .mdx instead of .stories.mdx. However that doesn't work in your case @nstuyvesant, because you're using v6 StoryStore which doesn't support plain .mdx files at all.

So I don't have a good workaround for you yet.

@nstuyvesant
Copy link
Author

@JReinhold - thanks for investigating this one. The master branch for @carbon/charts uses Storybook 5.3.12. I've been working hard to migrate things forward to prepare for 7.0 but the sticking point is that I would have to convert all 895 stories that are data-generated (5 environments, 179 stories for each - at least) via storiesOf() if I want to eliminate this setting in my main.ts...

features: {
  storyStoreV7: false
}

So I've been converting what I can to MDX to take advantage of the markup capabilities but that still leaves the other 895 stories.

@shilman
Copy link
Member

shilman commented Mar 30, 2023

I would consider this a bug here: storybookjs/mdx2-csf@051f37c/src/index.ts#L139
It shouldn't add an mdxSource prop if there's already one there.

@JReinhold I don't consider that a bug. The mdxSource prop is an internal implementation detail that users shouldn't be overriding.

@nstuyvesant
Copy link
Author

nstuyvesant commented Mar 30, 2023

Might not be following. Was trying to create documentation page not connected to a story primarily to show source code with examples and some explanation. Am I overriding something?

@shilman
Copy link
Member

shilman commented Mar 30, 2023

@nstuyvesant Sorry for the confusion, was just comment on one part of @JReinhold 's comment above. Updated my comment. Still thinking about the proper solution to your problem!

@shilman
Copy link
Member

shilman commented Mar 30, 2023

I'm unsure what to do about this, other than giving @storybook/mdx2-csf the raw MDX source instead of the compiled output, but I can only imagine that the architecture wouldn't support that at all.

I'll dig into this when I have time. Getting the raw source was the original intent of mdxSource in MDX1. I think it should be possible in MDX2 as well.

@nstuyvesant
Copy link
Author

Ah ok - I misunderstood. Thanks for the clarification.

@shilman
Copy link
Member

shilman commented Mar 30, 2023

A little background on the problem I was trying to solve... take a look at the DIAGRAMS section on this storybook...

@nstuyvesant this is super useful. can you please add it as a comment on #9828 which is the main issue for migrating off of storiesOf? We're going to come up with at least one migration path for storiesOf users in 7.x so we can remove it completely in 8.0 and having all the existing use cases in one place will be great as we prioritize solutions & solicit community feedback.

@nstuyvesant
Copy link
Author

@shilman - you got it. I just posted something.

@shilman shilman self-assigned this Apr 24, 2023
@shilman shilman moved this to Ready for work in Core Team Projects Jun 12, 2023
@vanessayuenn vanessayuenn moved this from Ready for work to Needs Discussion in Core Team Projects Oct 30, 2023
@shilman
Copy link
Member

shilman commented Dec 20, 2023

Hi @nstuyvesant so sorry this fell off my radar and I'm just getting back to it.

We're no longer maintaining the old MDX behavior at this point, so I think your best bet if you want to keep the source formatting is to define a story, import it into MDX, and then set the docs.source.code parameter and it should preserve the formatting exactly: https://storybook.js.org/docs/api/doc-block-source#code

Please let me know if you have any questions!

@shilman shilman closed this as completed Dec 20, 2023
@nstuyvesant
Copy link
Author

Thanks @shilman

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

No branches or pull requests

4 participants