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

Addon-docs: Fix MDX stories with multiple children #9531

Merged
merged 5 commits into from
May 4, 2020

Conversation

atanasster
Copy link
Member

Issue: #8571

What I did

mdx-compiler-plugin now filters all potential children.
added example to official storybook addon-docs.stories.mdx - multiple children
added test case to mdx-compiler-plugin.tests

How to test

official storybook
yarn test --core

@vercel
Copy link

vercel bot commented Jan 18, 2020

This pull request is being automatically deployed with ZEIT Now (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://zeit.co/storybook/monorepo/62sikvvz3
✅ Preview: https://monorepo-git-fork-atanasster-mdx-compiler-multiple-children.storybook.now.sh

@vercel vercel bot temporarily deployed to Preview January 18, 2020 18:37 Inactive
if (body.type === 'JSXExpressionContainer') {
// FIXME: handle fragments
body = body.expression;
const bodyParts = bodyNodes.map(bodyNode => {
Copy link
Member

Choose a reason for hiding this comment

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

Would be simpler, and even more correct, if we just wrapped the child nodes in a fragment?

Copy link
Member Author

Choose a reason for hiding this comment

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

Can you please explain, not sure i follow where you have it in mind?

Copy link
Member

@shilman shilman Jan 19, 2020

Choose a reason for hiding this comment

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

Pseudocode since I'm on my phone:

if(len(Non-text children) > 1):
 story.children = [wrapInFragment(story.children)]

Copy link
Member

Choose a reason for hiding this comment

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

Hmm I need to think about the non-react case too 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

I added a comment below on the code, thats what the coe does if i understand correct;y. The comment here about FIXME was from the old code/

// 1. Add line breaks
// 2. Enclose in <> ... </>
storyCode = bodyParts.map(({ code }) => code).join('\n');
const storyReactCode = bodyParts.length > 1 ? `<>\n${storyCode}\n</>` : storyCode;
Copy link
Member Author

Choose a reason for hiding this comment

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

@shilman - here it is enclosing the elements in <> </> if more than one child - is that what you meant for the fragment?

I think this is more correct to use pure JSX instead of React.Fragment, no?

Copy link
Member

Choose a reason for hiding this comment

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

I'll play around with it more. Not what I expected the code to look like, but I also haven't really dug in yet. Thanks for your patience!

Copy link
Member Author

@atanasster atanasster left a comment

Choose a reason for hiding this comment

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

Yes, let me know. I had to make some code compromises to keep compatibility and the tests running. Another constraint was to have the <> tags in the code so it works, but to not have them for-the story source preview

if (body.type === 'JSXExpressionContainer') {
// FIXME: handle fragments
body = body.expression;
const bodyParts = bodyNodes.map(bodyNode => {
Copy link
Member Author

Choose a reason for hiding this comment

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

I added a comment below on the code, thats what the coe does if i understand correct;y. The comment here about FIXME was from the old code/

@shilman shilman self-assigned this Jan 28, 2020
@shilman shilman added this to the 6.0.0 milestone Jan 28, 2020
@stale
Copy link

stale bot commented Feb 18, 2020

Hi everyone! Seems like there hasn't been much going on in this issue lately. If there are still questions, comments, or bugs, please feel free to continue the discussion. Unfortunately, we don't have time to get to every issue. We are always open to contributions so please send us a pull request if you would like to help. Inactive issues will be closed after 30 days. Thanks!

@stale stale bot added the inactive label Feb 18, 2020
@stale stale bot removed the inactive label Feb 18, 2020
@shilman shilman modified the milestones: 6.0, 6.0 docs Mar 26, 2020
@shilman shilman changed the title Mdx compiler multiple children Addon-docs: Fix MDX stories with multiple children May 4, 2020
@shilman shilman merged commit eb297a7 into storybookjs:next May 4, 2020
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.

2 participants