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: Make Preview work for arbitrary jsx #7844

Closed
matthewmatician opened this issue Aug 22, 2019 · 14 comments
Closed

Addon-docs: Make Preview work for arbitrary jsx #7844

matthewmatician opened this issue Aug 22, 2019 · 14 comments
Assignees
Milestone

Comments

@matthewmatician
Copy link

Is your feature request related to a problem? Please describe.
Sometimes I we want to use arbitrary jsx in our mdx files, but we'd love to use a <Preview> component to wrap that jsx and allow people to easily see the source.

Describe the solution you'd like
When I use the <Preview> component in an mdx file like this...

<Preview>
  <Story id="ui-foundation-elements-button--defaultexample />
<Preview>

I can see/copy the source code of this story. I'd also like to see the source code when I do this:

<Preview>
  <Button type="fancy">I am a button</Button>
</Preview>

Describe alternatives you've considered
We tried just making a story for every example needed in our mdx files, but then our list of stories gets really long. Also, it's a little painful to have to go find the identifier for all of those stories that were created.

Are you able to assist bring the feature to reality?
I'd love to help if I can. Might need some direction.

@matthewmatician matthewmatician changed the title Make <Preview> work for arbitrary jsx Make <Preview> work for arbitrary jsx Aug 22, 2019
@joeycozza
Copy link
Contributor

joeycozza commented Aug 22, 2019

I'm just making guesses here, so Shilman, you can ignore me if I'm way off.

I assume that the source code is coming from the storysource addon behind the scenes, so only stories have access to the sourcecode.

Would it make more sense/be easier to have a new Doc Block for arbitrary code, and keep <Preview> for stories?

@atanasster
Copy link
Member

hi, it seems the <Story /> name prop is needed to build up the storiesHash. Also the id is already used for loading stories, so I think we would have to have a name in any case.

What do you think of the following solutions, all of which will load the full storiesHash, but will limit the visibility of the story in the navigation (tree view, prev, next):

  1. Using parameters (easiest but will somewhat pollute the parameters space)
<Preview>
  <Story name="unique-id" parameters={{ hidden: true }}>
    <Button onClick={action('clicked')}>preview source</Button>
  </Story>  
</Preview>
  1. Add a hidden prop to be saved/loaded
<Preview>
  <Story name="unique-id" hidden>
    <Button onClick={action('clicked')}>preview source</Button>
  </Story>  
</Preview>
  1. Both above can hide the usage as
<InlinePreview name="unique-id">
  <Button onClick={action('clicked')}>preview source</Button>
</InlinePreview>
  1. autogenerate a unique id behind the scene, but user.api will not know how to select or navigate to the embedded story
<InlinePreview>
  <Button onClick={action('clicked')}>preview source</Button>
</InlinePreview>

@shilman
Copy link
Member

shilman commented Sep 1, 2019

I don't like the idea of creating dummy stories (although I'm doing it for Docs-only MDX #7719, and you could refactor that to handle this use case too if you wanted). The story store has a lot of stuff hanging off it, and I'm worried about adding more unnecessarily.

Meanwhile, I handle source differently between CSF stories and MDX stories. CSF stories use source-loader, which is a complex beast, and MDX stories just generate an extra mdxSource story parameter pretty trivially in the compiler.

Here's my counter-proposal:

  1. Create an optional mdxSource attribute on the Preview component. Show that in the Source if it's present.
  2. Modify the MDX compiler to add the mdxSource attribute to Preview:
    a) If NONE of the children are <Story> objects
    b) OR possibly if ANY of children are non-Story, non-whitespace objects?!

2a would mean the user could use either Story or freeform JSX. 2b would mean the user could use both, but the results would be weird if the user used Story refs.

The only part I haven't done is modify the MDX AST in the compiler, but I'm sure it's possible. And the mdxSource thing has been pretty simple on the Story side, so I don't expect any of this to be problematic. Also there's a good pattern for testing the compiler, so you can use TDD:

yarn jest --testPathPattern=mdx-compiler-plugin.test.js --watch

@shilman shilman changed the title Make <Preview> work for arbitrary jsx Addon-docs: Make Preview work for arbitrary jsx Sep 1, 2019
@atanasster
Copy link
Member

Cool ill give it a try, if i cant figure it out the mdx compiler will post here so someone else can takea stab.

@atanasster
Copy link
Member

@shilman can you please check if this is what you had in mind for the mdx compiler:
https://github.com/atanasster/storybook/blob/0a357bef3dfd183b8f9b5ba071e7ccebfab7e344/addons/docs/src/mdx/mdx-compiler-plugin.js#L203

currently it does a double pass - exporting the strings in addition to updating the props, but we can optimize this.

also I am currently displaying the full code with the Preview element as more formatting was lost when just iterating through the children.

Also if @matthewmatician has multiple of those embedded previews to give it an early test?

@shilman
Copy link
Member

shilman commented Sep 2, 2019

Looks great @atanasster 🙆‍♂

@atanasster
Copy link
Member

Thanks, i ll clean it up, write some tests hopefully to submit a PR tomorrow. I get panic attacks when i have to merge :)
If anyone has more tests for mdx it will help a lot.

@shilman
Copy link
Member

shilman commented Sep 2, 2019

Cool! To be clear, we won't be able to merge this until 5.2 is shipped, which should be next week (knock wood) and once it's merged it'll be "early alpha 5.3" so it's fine if there are some corner case bugs.

@shilman shilman modified the milestones: 5.2.0, 5.3.0 Sep 2, 2019
@atanasster
Copy link
Member

Ah yes, of course. Also when you have some time to review it please let me know if you want to move some of the string exports into props.

@atanasster
Copy link
Member

@shilman - how can I run tests/update snapshots for the addons, or specifically addons/docs?

@shilman
Copy link
Member

shilman commented Sep 2, 2019

@atanasster To update snapshot tests:

yarn test --core --update

@atanasster
Copy link
Member

thanks, I didn't realize --core worked for the addons

@stale
Copy link

stale bot commented Sep 24, 2019

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 Sep 24, 2019
@shilman shilman self-assigned this Oct 10, 2019
@stale stale bot removed the inactive label Oct 10, 2019
@shilman shilman added mdx and removed addon: docs labels Oct 10, 2019
@shilman
Copy link
Member

shilman commented Oct 15, 2019

Hurrah!! I just released https://github.com/storybookjs/storybook/releases/tag/v5.3.0-alpha.20 containing PR #7966 that references this issue. Upgrade today to try it out!

You can find this prerelease on the @next NPM tag.

Closing this issue. Please re-open if you think there's still more to do.

@shilman shilman closed this as completed Oct 15, 2019
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