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

React: Add official cra-template #10122

Closed
wants to merge 10 commits into from

Conversation

kevin940726
Copy link
Contributor

@kevin940726 kevin940726 commented Mar 13, 2020

Issue: #9327

What I did

Create an official CRA template for Storybook: @storybook/cra-template. This template is targeting 6.0 with addon-docs by default.

The example story is sitting right inside the src directory called Welcome.stories.mdx. Since the <Welcome> component from @storybook/react/demo is outdated for this template, I move the introduction to mdx instead.

All the other files are copied from the official cra-template.

Once this is ready, I'll create another PR for @storybook/cra-template-typescript

How to test

Seems like bootstrapping with custom templates with file: schema will fail (maybe a bug from CRA?). I'm still figuring out how to properly test this without manually coping all the files.

npx create-react-app@latest --template file:./lib/cra-template cra-storybook

Once this is published to npm registry, we can bootstrap it with npx create-react-app --template @storybook my-app.

Screenshot

localhost_6006__path=_docs_welcome--to-storybook

Questions

  1. Any other things we want to include in this template? Like any unreleased 6.0 new features 😉?
  2. How should we maintain the package versions in template.json? I guess we can just pin them to ^6.0 when they're released.
  3. Is it possible to default to highlight docs panel by default?

c.c. @mrmckeb

@shilman shilman added cra Prioritize create-react-app compatibility feature request labels Mar 13, 2020
@shilman shilman changed the title Add official cra-template React: Add official cra-template Mar 13, 2020
Copy link
Member

@mrmckeb mrmckeb left a comment

Choose a reason for hiding this comment

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

Looks great, only a few comments/suggestions - I think a lot of people will value this!

lib/cra-template/package.json Show resolved Hide resolved
lib/cra-template/package.json Outdated Show resolved Hide resolved
lib/cra-template/template.json Outdated Show resolved Hide resolved
lib/cra-template/template/.storybook/main.js Outdated Show resolved Hide resolved
@shilman shilman added this to the 6.0.0 milestone Mar 16, 2020
@kevin940726 kevin940726 requested a review from mrmckeb March 17, 2020 14:14
@stale stale bot added the inactive label Apr 7, 2020
@stale stale bot removed the inactive label Apr 7, 2020
@kevin940726
Copy link
Contributor Author

Pinging @mrmckeb again for another review 😉

@storybookjs storybookjs deleted a comment from stale bot Apr 11, 2020
Copy link
Member

@mrmckeb mrmckeb left a comment

Choose a reason for hiding this comment

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

I'm not sure about the ignores for ESLint and Prettier - maybe long-term we can see if we can fix the root issus?

But definitely this is looking great, and I think we could get it out. What do you think @ndelangen and @shilman?

},
"license": "MIT",
"author": "Storybook Team",
"main": "template.json",
Copy link
Member

Choose a reason for hiding this comment

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

This won't be required in the next minor, but needs to stay for now.

lib/cra-template/template.json Outdated Show resolved Hide resolved
@kevin940726
Copy link
Contributor Author

@mrmckeb I'm not so sure about eslint/prettier issue. I had to ignore it because I want to better align the coding style in the template with the official CRA template. What do you mean of fixing the root issue? I can't think of any alternatives for now 🤔.

@kevin940726
Copy link
Contributor Author

@mrmckeb Pinging you again for another review :) (Take your time!)

@kevin940726 kevin940726 requested a review from mrmckeb May 14, 2020 09:33
@ndelangen ndelangen self-assigned this May 14, 2020
@ndelangen
Copy link
Member

I see 2 errors:

 FAIL  lib/cra-template/template/src/App.test.js
  ● renders learn react link

    TypeError: expect(...).toBeInTheDocument is not a function

       6 |   const { getByText } = render(<App />);
       7 |   const linkElement = getByText(/learn react/i);
    >  8 |   expect(linkElement).toBeInTheDocument();
         |                       ^
       9 | });
      10 |

      at Object.<anonymous>.test (lib/cra-template/template/src/App.test.js:8:23)

Summary of all failing tests
 FAIL  lib/cra-template/template/src/App.test.js
  ● renders learn react link

    TypeError: expect(...).toBeInTheDocument is not a function

       6 |   const { getByText } = render(<App />);
       7 |   const linkElement = getByText(/learn react/i);
    >  8 |   expect(linkElement).toBeInTheDocument();
         |                       ^
       9 | });
      10 |

      at Object.<anonymous>.test (lib/cra-template/template/src/App.test.js:8:23)


Test Suites: 1 failed, 119 passed, 120 total
Tests:       1 failed, 1390 passed, 1391 total
Snapshots:   408 passed, 408 total
Time:        32.276 s
Ran all test suites in 7 projects.

@kevin940726
Copy link
Contributor Author

@ndelangen I just updated all the dependencies and template to the latest version. It should work now!

However, when running yarn storybook, there would be a warning as in #10822, not sure if it's related? Furthermore, when running in development mode, everything works fine, but when running in production mode (yarn build-storybook), the page will load infinitely with no stories available. I'm not sure what's the problem here?

@shilman shilman modified the milestones: 6.0, 6.1 essentials Jul 30, 2020
@shilman
Copy link
Member

shilman commented Dec 1, 2020

Thanks for the contribution @kevin940726. During 6.0 we overhauled the sb init command to generate much better examples than before. Although the template would simplify the install process very slightly for users, it increases our maintenance burden substantially because the CRA install will change over time and our sb init template will change as well, and this seems like it should just contain the result of both of those. So I'm closing this PR. If you want to create a storybook repo for this outside the monorepo, I'd be open to that.

@shilman shilman closed this Dec 1, 2020
@kevin940726 kevin940726 deleted the cra-template branch December 1, 2020 09:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cra Prioritize create-react-app compatibility feature request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants