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

Framework: Add Component Storybook. #17475

Merged
merged 9 commits into from Oct 4, 2019

Conversation

@epiqueras
Copy link
Contributor

commented Sep 18, 2019

Description

This PR adds Storybook support to the repo and uses it to create a storybook for @wordpress/components using the Button component as an example.

It also adds plugins for automatically running and displaying accessibility tests right besides stories, simulating color blindness, and changing the viewport size/device.

The PR also modifies the Travis config so that the storybook is deployed to GitHub Pages using the /design-system/components path.

This gave me the idea that maybe the GitHub Pages page should use a custom domain in wordpress.org and present some sort of landing page for the JS focused WordPress efforts. That can then link to the editor playground and the storybook.

How has this been tested?

The new storybook was verified to work in development (npm run design-system:dev) and production (npm run design-system:build) modes.

Screenshots

Passing a11y Tests

Screen Shot 2019-09-18 at 1 49 58 PM

Failing a11y Tests

Screen Shot 2019-09-18 at 1 51 04 PM

Color Blindness Simulator

Screen Shot 2019-09-18 at 2 51 57 PM

Changing Viewports

Screen Shot 2019-09-18 at 2 15 13 PM

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
@gziolo

This comment has been minimized.

Copy link
Member

commented Sep 19, 2019

How would it play with #17173 from @ItsJonQ where he proposes a site with live examples of components?

There is also the playground.

In addition, there is also a proposal to create some specialized testing app for RichText proposed by @ellatrix in #17303.

Moving forward, we will also need playgrounds for iOS and Android example apps working with React Native. See #17456 from @Tug.

I guess, all individual solutions makes sense but we should coordinate everything and bring them as close as possible so we don't have to maintain 6 completely independent apps with their own unique tooling and build steps. Do you have any thoughts on how this could be consolidated as much as possible?

Should we start with some sort of proposal which describes the role of every tool and how they can benefit workflows of different types of contributors?

/cc @jameslnewell @griffbrad @youknowriad

@nb

This comment has been minimized.

Copy link
Member

commented Sep 19, 2019

Thanks, @epiqueras – storybook has improved since the last time I looked at it. The keyboard shortcuts are handy.

Is there a chance we can get as part of the netlify deploy? Not having to run it locally will be super useful. I thought twice before running it locally :)

The in-browser test also look like a useful tool.

On styles – the link uses the default browser styles, do you think it would make sense to put it in some WordPress style context?

How was the story format chosen? Both mdx and storiesOf look interesting.

And few additions that would be helpful, even if not in this PR:

  • A way to see the source code of the story.
  • Be able to change props on the fly. I liked the “knobs” tab in this example.
  • Have a link to the documentation for each component.
@epiqueras

This comment has been minimized.

Copy link
Contributor Author

commented Sep 19, 2019

@gziolo

How would it play with #17173 from @ItsJonQ where he proposes a site with live examples of components?

This is a much simpler alternative approach. We should however, theme it so that it looks more like WordPress docs, but that can be a different PR.

There is also the playground.

In addition, there is also a proposal to create some specialized testing app for RichText proposed by @ellatrix in #17303.

Yeah, those are parallel things.

Moving forward, we will also need playgrounds for iOS and Android example apps working with React Native. See #17456 from @Tug.

Yeah, Storybook can support React Native.

I guess, all individual solutions makes sense but we should coordinate everything and bring them as close as possible so we don't have to maintain 6 completely independent apps with their own unique tooling and build steps. Do you have any thoughts on how this could be consolidated as much as possible?

Should we start with some sort of proposal which describes the role of every tool and how they can benefit workflows of different types of contributors?

This is a very conservative start. I'm making no assumptions about any sort of decision beyond using Storybook for the component playground.

@nb

Thanks, @epiqueras – storybook has improved since the last time I looked at it. The keyboard shortcuts are handy.

Yeah it looks like it reclaimed its throne in the UI playground space. They even support MDX docs now.

Is there a chance we can get as part of the netlify deploy? Not having to run it locally will be super useful. I thought twice before running it locally :)

Yeah me too. I just added it.

On styles – the link uses the default browser styles, do you think it would make sense to put it in some WordPress style context?

I'm not sure. I added the Gutenberg styles, but if this will be potentially used outside of a WordPress context, maybe it's a sign we need to add more default styles?

How was the story format chosen? Both mdx and storiesOf look interesting.

Good question.

The storiesOf is the old, very clunky API, which is no longer recommended.

MDX is for authoring freeform Docs with embedded stories like React Styleguidist. It can and should complement the playground.

A way to see the source code of the story.

The addon that does that is out of date, so it might be a bit involved, but another PR can take care of it.

Be able to change props on the fly. I liked the “knobs” tab in this example.

I almost added the Knobs addon, but linking props to it is a very manual process and I want to be conservative about enforcing things on contributors. If we think it's worth it, we can add it.

Have a link to the documentation for each component.

Yeah, that's what MDX will be for.

@epiqueras

This comment has been minimized.

@ItsJonQ

This comment has been minimized.

Copy link
Contributor

commented Sep 24, 2019

Just took a look! @epiqueras thank you for putting this together! I think Storybook is a fantastic addition to the UI dev/design work flow (essential IMO).

@gziolo Re: documentation

@epiqueras shared this link with me:
https://medium.com/storybookjs/storybook-docs-sneak-peak-5be78445094a

If we're able to get this into Storybook, then it is able to accommodate the needs that the doc site (that I had proposed) attempted to address.

cc'ing @jameslnewell 😊

I almost added the Knobs addon, ... If we think it's worth it, we can add it.

P.S. I'm a big fan of Storybook knobs :). The reason is because it allows folks to very quickly QA things in the browser. A great example is text values.

"What if this person has no first name?"
"What if the last name is veryyyyyyyy long"?

You can add these things via knobs, and see if/how the UI breaks

Folks can do this directly in the Netlify generated deploy preview within PRs.
No local dev required 👍

@epiqueras

This comment has been minimized.

Copy link
Contributor Author

commented Sep 24, 2019

I've gone ahead and added the new MDX doc features.

Every story file now gets a docs page automatically generated, but you can overwrite them with custom MDX files.

I've also added a stand-alone documentation page as an example.

I think this new "Docs" feature suite removes the need for most of the plugins talked about above. We can always add more later on PRs that actually make real use of them.

Here is more information on the new features:

https://github.com/storybookjs/storybook/blob/next/addons/docs/README.md#more-resources

Take a look at how pages are automatically generated, how MDX can be used to overwrite them, and how you can theme the docs and export them on their own.

@diegohaz

This comment has been minimized.

Copy link

commented Sep 24, 2019

@epiqueras Could you try to create stories for a component that has JSX?

I'm using your PR to create stories for Toolbar, but seeing this error on the Storybook page:

ReferenceError: React is not defined
    at Module../packages/components/src/popover/index.js (http://localhost:50417/main.b1ea0813c451b7179bb3.bundle.js:3941:1)
    at __webpack_require__ (http://localhost:50417/runtime~main.b1ea0813c451b7179bb3.bundle.js:786:30)
    at fn (http://localhost:50417/runtime~main.b1ea0813c451b7179bb3.bundle.js:151:20)
    at Module../packages/components/src/tooltip/index.js (http://localhost:50417/main.b1ea0813c451b7179bb3.bundle.js:5884:66)
    at __webpack_require__ (http://localhost:50417/runtime~main.b1ea0813c451b7179bb3.bundle.js:786:30)
    at fn (http://localhost:50417/runtime~main.b1ea0813c451b7179bb3.bundle.js:151:20)
    at Module../packages/components/src/icon-button/index.js (http://localhost:50417/main.b1ea0813c451b7179bb3.bundle.js:2903:66)
    at __webpack_require__ (http://localhost:50417/runtime~main.b1ea0813c451b7179bb3.bundle.js:786:30)
    at fn (http://localhost:50417/runtime~main.b1ea0813c451b7179bb3.bundle.js:151:20)
    at Module../packages/components/src/toolbar-button/index.js (http://localhost:50417/main.b1ea0813c451b7179bb3.bundle.js:5463:70)

It's fixed if I add an empty .babelrc file to packages/components/storybook, but it's weird anyway! 🙃

@epiqueras

This comment has been minimized.

Copy link
Contributor Author

commented Sep 24, 2019

It's fixed if I add an empty .babelrc file to packages/components/storybook, but it's weird anyway!

@diegohaz It was picking up the root babel config and that transforms JSX into calls to wp.element.createElement. I've added the specific .babelrc file and it works fine now.

Thanks!

@gziolo

This comment has been minimized.

Copy link
Member

commented Sep 25, 2019

It's fixed if I add an empty .babelrc file to packages/components/storybook, but it's weird anyway! 🙃

It seems like Storybook performs some magic operations on Babel config, similar to what Parcel does. This is what we had to add for Parcel:

{
"presets": ["@wordpress/babel-preset-default"],
"plugins": [
[ "@babel/plugin-transform-react-jsx", {
"pragma": "createElement"
} ],
"babel-plugin-inline-json-import"
]
}

"playground:start": "concurrently \"npm run dev:packages\" \"parcel playground/src/index.html -d playground/dist\"",
"preenv": "npm run check-engines",
"env": "wp-scripts env"
"env": "wp-scripts env",
"design-system:dev": "concurrently \"npm run dev:packages\" \"start-storybook -c ./packages/components/storybook\"",

This comment has been minimized.

Copy link
@youknowriad

youknowriad Sep 25, 2019

Contributor

for playground we use :start and for gutenberg we use :dev I personally prefer start but we should try to consolidate all these similar commands on :dev maybe to avoid impacting the plugin devs.

This comment has been minimized.

Copy link
@epiqueras

epiqueras Sep 25, 2019

Author Contributor

I've seen start used more to start production apps like web servers, so I kind of liked we opted for dev.

This comment has been minimized.

Copy link
@youknowriad

youknowriad Oct 2, 2019

Contributor

Can we update the playground one?

This comment has been minimized.

Copy link
@epiqueras

epiqueras Oct 2, 2019

Author Contributor

Done


# Introduction

Hello World

This comment has been minimized.

Copy link
@youknowriad

youknowriad Sep 25, 2019

Contributor

Are we concerned about the duplication in terms of docs, this will introduce. Components have READMEs that are currectly synced into the WordPress handbook and they'll have special docs that will live in the storybook?

This comment has been minimized.

Copy link
@epiqueras

epiqueras Sep 25, 2019

Author Contributor

These are the design system docs.

A lot of that content will probably be moved here though.

This comment has been minimized.

Copy link
@youknowriad

youknowriad Sep 25, 2019

Contributor

These are the design system docs.

This is what the README are for. so the components part for the design system will get duplicated?

This comment has been minimized.

Copy link
@epiqueras

epiqueras Sep 25, 2019

Author Contributor

Yes, but MDX is much better than a simple README.md and will be the new framework for them.

I guess we can delete the READMEs once the new docs are fleshed out.

This comment has been minimized.

Copy link
@youknowriad

youknowriad Sep 25, 2019

Contributor

We can't delete the READMEs until we figure out how to show Storybook in the wp.org devdocs. I'm concerned that it will take a lot of time and that we'll have to maintain duplicated content for a long time.

This comment has been minimized.

Copy link
@epiqueras

epiqueras Sep 25, 2019

Author Contributor

Storybook can export static files, that won't be hard to do. And it won't be duplicated, the new docs will be better and there won't necessarily be a 1 to 1 mapping, because the goal is to overhaul this package into a proper design system component kit.

@@ -0,0 +1,41 @@
/**

This comment has been minimized.

Copy link
@youknowriad

youknowriad Sep 25, 2019

Contributor

Is it possible to move it out of the components package?

This comment has been minimized.

Copy link
@epiqueras

epiqueras Sep 25, 2019

Author Contributor

It can be wherever, but I don't think that's a good idea. These are like tests and should live next to components.

Other storybooks, like the block editor's, might warrant a different file structure though.

This comment has been minimized.

Copy link
@youknowriad

youknowriad Sep 25, 2019

Contributor

I can see that but if we're using Storybook as a doc website, I think it defeats that argument. We should first clarify what we want Storybook to be used for.

If it's for a docs website, I believe a separate location is better, if it's just for having stories to develop the component, I think having them in subfolders is fine.

I can also see a folder structure like that:

playground
   - design-system
   - block-editor (current playground) 
    - ios (these exist today but in the mobile repo)
    - android

basically a folder for all playgrounds.

This comment has been minimized.

Copy link
@epiqueras

epiqueras Sep 25, 2019

Author Contributor

We are using it for both. Storybook has a component playground mode and a docs website mode.

The playground mode files should live next to their components, because they are like tests and will need to reference them heavily. It also lets you easily see which components are missing stories.

The docs mode files should live in a docs folder in the root of their package, like docs/introduction.story.mdx here, because they are just freeform documentation and are not tied to a specific component.

This comment has been minimized.

Copy link
@youknowriad

youknowriad Sep 25, 2019

Contributor

I feel we're trying to solve a lot at the same time. While I agree that it would be great to use storybook for both, I think we should have a clear plan about how to update the docs, how to sync them to wp.org docs. Right now when I look at the output, it's a mix of both which I'm not sure is perfect to sync as is into wp.org. Also how to do it.

I'd be ok to land storybook as an experiment for now until we figure out all these things but an experiment shouldn't impact the current codebase a lot that's why I wonder if it's better to start as a separate folder

This comment has been minimized.

Copy link
@epiqueras

epiqueras Sep 25, 2019

Author Contributor

I feel we're trying to solve a lot at the same time.

We are just solving "choosing the docs framework for reusable components".

I think we should have a clear plan about how to update the docs, how to sync them to wp.org docs.

Travis job that deploys static files.

Right now when I look at the output, it's a mix of both which I'm not sure is perfect to sync as is into wp.org

There's CLI options for separating the 2 or we can theme it so that it fits better into .org's site patterns.

I'd be ok to land storybook as an experiment for now until we figure out all these things but an experiment shouldn't impact the current codebase a lot that's why I wonder if it's better to start as a separate folder

What could change is how we theme and layout Storybook's output, but not the use of Storybook, it's by far the best option out there for this.

Right now this just uses defaults for everything, but with this as a base, designers can lay things out with custom navigation, layouts, colors, and whatever else the .org site needs.

@epiqueras

This comment has been minimized.

Copy link
Contributor Author

commented Sep 25, 2019

It seems like Storybook performs some magic operations on Babel config, similar to what Parcel does. This is what we had to add for Parcel:

Yeah, we don't need that though, because Storybook uses its own version of React.

Copy link
Contributor

left a comment

I'm willing to merge this but we need a plan to move this forward, should we create a follow-up issues about adding stories for all components, how about docs? who own this?

@epiqueras epiqueras force-pushed the add/storybook-for-components branch from 80006cc to b06c88b Oct 2, 2019
@epiqueras

This comment has been minimized.

Copy link
Contributor Author

commented Oct 2, 2019

I'm willing to merge this but we need a plan to move this forward, should we create a follow-up issues about adding stories for all components, how about docs? who own this?

cc @ItsJonQ

Merging as soon as Travis is finished here 🎉

@epiqueras epiqueras force-pushed the add/storybook-for-components branch from b06c88b to 22ee0cb Oct 2, 2019
@gziolo

This comment has been minimized.

Copy link
Member

commented Oct 4, 2019

Added unit tests might fail from time to time so it would be nice to address this comment from my accompanied PR:

#17762 (comment)

@epiqueras epiqueras force-pushed the add/storybook-for-components branch 3 times, most recently from 2c47dcc to 1778807 Oct 4, 2019
@epiqueras epiqueras force-pushed the add/storybook-for-components branch from 1778807 to edd53d4 Oct 4, 2019
@epiqueras epiqueras merged commit 9230d0c into master Oct 4, 2019
3 of 7 checks passed
3 of 7 checks passed
pull-request-automation
Details
Header rules - gutenberg-playground No header rules processed
Details
Pages changed - gutenberg-playground All files already uploaded
Details
Redirect rules - gutenberg-playground No redirect rules processed
Details
Mixed content - gutenberg-playground No mixed content detected
Details
Travis CI - Pull Request Build Passed
Details
netlify/gutenberg-playground/deploy-preview Deploy preview ready!
Details
@gziolo gziolo deleted the add/storybook-for-components branch Oct 7, 2019
@gziolo

This comment has been minimized.

Copy link
Member

commented Oct 7, 2019

https://wordpress.github.io/gutenberg/ is broken, not sure if this is related to this PR but we need ot investigate it.

See the section about the playground:

https://github.com/WordPress/gutenberg/blob/master/docs/contributors/getting-started.md#playground

before_deploy:
- npm install
- npm run playground:build -- --public-url '/gutenberg'
- npm run design-system:build

This comment has been minimized.

Copy link
@gziolo

gziolo Oct 7, 2019

Member

It looks like with the changes applied in this PR, design-system:build will run twice on Travis:

"playground:build": "npm run build:packages && parcel build playground/src/index.html -d playground/dist && npm run design-system:build",

This comment has been minimized.

Copy link
@epiqueras

epiqueras Oct 7, 2019

Author Contributor
@epiqueras

This comment has been minimized.

Copy link
Contributor Author

commented Oct 7, 2019

@gziolo

https://wordpress.github.io/gutenberg/ is broken, not sure if this is related to this PR but we need ot investigate it.

#17818 also fixes this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.