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

CLI: Add common welcome MDX and cleanup #11422

Merged
merged 20 commits into from
Jul 10, 2020
Merged

CLI: Add common welcome MDX and cleanup #11422

merged 20 commits into from
Jul 10, 2020

Conversation

tooppaaa
Copy link
Contributor

@tooppaaa tooppaaa commented Jul 5, 2020

Issue: #10794

What I did

Use @domyen welcome story in all frameworks template with a single implementation
Remove old welcome stories
Update e2e

Relates to #9103
I updated angular CLI to use this setup out of the box: https://github.com/storybookjs/storybook/blob/next/addons/docs/angular/README.md#props-tables

How to test

  • E2E

@tooppaaa tooppaaa self-assigned this Jul 5, 2020
@tooppaaa tooppaaa added cli maintenance User-facing maintenance tasks labels Jul 5, 2020
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.

Awesome awesome. Can't speak to the CLI changes but LGTM


<Meta title="Example/Introduction" />

<style>{`
Copy link
Member

Choose a reason for hiding this comment

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

Do you think we should maybe import this style too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we should if it's easy. I tried to import a css file in mdx but it doesn't work. Is that supposed to be supported ?

If it's not, maybe we should log a ticket and see if it gets community support ?

Either it's wanted or not, I think we can ship this and improve later.

@yannbf
Copy link
Member

yannbf commented Jul 5, 2020

Awesome stuff! I'll check it out and give my feedback soon

@yannbf
Copy link
Member

yannbf commented Jul 5, 2020

The following links don't exist:
https://storybook.js.org/docs/presets
https://storybook.js.org/docs/webpack
https://storybook.js.org/docs/styling
https://storybook.js.org/docs/data

I assume we only want to have this deployed once the links are working?

</div>

<div class="tip-wrapper">
<span class="tip">Tip</span>Edit the Markdown in <code>src/storybook-examples/welcome.mdx</code>
Copy link
Member

Choose a reason for hiding this comment

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

Here it says src/storybook-examples/welcome.mdx although the generated file is stories/0.Introduction.stories.mdx

Copy link
Member

Choose a reason for hiding this comment

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

That was my mistake! @tmeasday why is the file called 0.Introduction? Is it so Storybook shows it as the first story without having extra configuration in main.js?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I believe so. However, in the example it's not being shown first 🤔

image

Copy link
Member

Choose a reason for hiding this comment

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

@tmeasday why is the file called 0.Introduction? Is it so Storybook shows it as the first story without having extra configuration in main.js?

That was originally why, yeah. Can we just change the stories field to import mdx first? Or is that too complex?

Copy link
Member

Choose a reason for hiding this comment

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

@tmeasday I think it looks weird to have 0.name when I first install. I'd rather the default story import be something reasonable like showing MDX files first.

Copy link
Member

Choose a reason for hiding this comment

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

I have experienced quite some colleagues keeping this nomenclature in order to have properly ordered stories, and of course it all starts because the template does it.
For the ones that want to have a simpler way of organizing the stories (per category is something I see a lot), notice people creating a custom sorting mechanism like this one: https://github.com/ing-bank/lion/blob/master/.storybook/preview.js#L30

Sorting stories should be something simple by default and if people want to do something more complex they should be free to do so, IMO.

@tooppaaa
Copy link
Contributor Author

tooppaaa commented Jul 5, 2020

There's an issue which I reproduced while updating @babel/core from 7.10.2 to 7.10.4

WARN ./src/stories/0.Introduction.stories.mdx
WARN Module build failed (from ./node_modules/babel-loader/lib/index.js):
WARN SyntaxError: /tmp/storybook-e2e-testing/cra-vlatest/src/stories/0.Introduction.stories.mdx: Support for the experimental syntax 'jsx' isn't currently enabled (20:10):
 

WARN   18 | const makeShortcode = name => function MDXDefaultShortcode(props) { 
WARN   19 |   console.warn("Component " + name + " was not imported, exported, or provided by MDXProvider as global scope")
 
WARN > 20 |   return <div {...props}/>
WARN        |          ^
WARN   21 | };
WARN   22 | 

WARN   23 | const layoutProps = {
 
WARN Add @babel/preset-react (https://git.io/JfeDR) to the 'presets' section of your Babel config to enable transformation.
 
WARN If you want to leave it as-is, add @babel/plugin-syntax-jsx (https://git.io/vb4yA) to the 'plugins' section to enable parsing.

@shilman shilman added this to the 6.0 rc milestone Jul 6, 2020
Copy link
Member

@gaetanmaisse gaetanmaisse left a comment

Choose a reason for hiding this comment

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

💪

@shilman shilman changed the title CLI: add common welcome and cleanup CLI: Add common welcome MDX and cleanup Jul 10, 2020
Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

Looking good! Going to merge this and resolve some of the issues in a followup PR. Great work @tooppaaa 🔥

@shilman shilman merged commit 34962f7 into next Jul 10, 2020
@yannbf yannbf mentioned this pull request Jul 12, 2020
2 tasks
@stof stof deleted the feature/welcomeMDX branch May 25, 2022 09:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli maintenance User-facing maintenance tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants