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

Storybook #768

Merged
merged 1 commit into from
Jan 8, 2019
Merged

Storybook #768

merged 1 commit into from
Jan 8, 2019

Conversation

BPScott
Copy link
Member

@BPScott BPScott commented Dec 15, 2018

WHY are these changes introduced?

We currently use the playground as a scratchpad and as a way of showcasing our existing components examples (as extracted from the component READMEs). The existing components pages are screenshot using Percy for regression testing and ran through pa11y for accessibility auditing.

Other teams within Shopify want to do similar things with their component libraries and are looking at using Storybook as the tool to do it.

Leveraging an existing tool and its plugin ecosystem feels preferable to making our own home-baked one from a maintenance and improvement perspective. Packaging any of our own custom functionality as storybook plugins that can then be consumed by other teams that would also find it useful also feels like an easier sell than making them use a bespoke thing.

WHAT is this pull request doing?

This PR introduces storybook as replacement to the playground. The dev features you know - a hot reloadable playground and examples extracted from the component READMEs are both present, along with support for percy screenshots (using percy's official storybook plugin) and pa11y integration.

How to 🎩

  • yarn run dev to run storybook and click around the examples
  • Check hot-reloading works in the playground (still living at playground/Playground.tsx)
  • Check percy diffs - they will have changed due to the layout and urls being slightly different but that's not the end of the world.

@BPScott BPScott temporarily deployed to polaris-react-pr-768 December 15, 2018 02:13 Inactive
@BPScott BPScott temporarily deployed to polaris-react-pr-768 December 18, 2018 02:09 Inactive
@BPScott BPScott temporarily deployed to polaris-react-pr-768 December 19, 2018 01:27 Inactive
@BPScott
Copy link
Member Author

BPScott commented Dec 19, 2018

I'm going to park the bus on this and come back to it later.

I've managed to do something where running in dev mode (i.e. yarn storybook) works but running a prod build (yarn build-storybook) results in horrible breakages, which I think is down to our usage of require.context or some variable hoisting weirdness. Unfortunatly my webpack-fu is failing me at the moment. Alas the percy plugin acts upon the built output so I can't look at building that (or pa11y stuff as I was planning on using a similar pattern) till that is fixed.

There's no point merging this till it is close to being a drop-in solution for what playground does already so this will languish for a while.

@BPScott BPScott temporarily deployed to polaris-react-pr-768 December 19, 2018 23:50 Inactive
@BPScott BPScott temporarily deployed to polaris-react-pr-768 December 20, 2018 02:20 Inactive
@BPScott BPScott temporarily deployed to polaris-react-pr-768 December 20, 2018 02:22 Inactive
@BPScott
Copy link
Member Author

BPScott commented Dec 20, 2018

Turns out that was nothing to do with require.context faffing, but instead babel transforms.

A small "fix" later and we're back in business

@BPScott BPScott temporarily deployed to polaris-react-pr-768 December 20, 2018 23:49 Inactive
@BPScott BPScott temporarily deployed to polaris-react-pr-768 December 20, 2018 23:59 Inactive
@BPScott BPScott temporarily deployed to polaris-react-pr-768 December 21, 2018 00:00 Inactive
@BPScott BPScott temporarily deployed to polaris-react-pr-768 December 21, 2018 00:09 Inactive
@BPScott BPScott temporarily deployed to polaris-react-pr-768 December 21, 2018 00:29 Inactive
package.json Outdated
@@ -82,6 +83,7 @@
}
},
"devDependencies": {
"@percy-io/percy-storybook": "^2.1.0",
"@percy/puppeteer": "^0.4.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this still needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope. I'm going to do the big nuking at the end when I'm happy I've not missed anything.

@BPScott BPScott temporarily deployed to polaris-react-pr-768 December 21, 2018 01:56 Inactive
@BPScott BPScott temporarily deployed to polaris-react-pr-768 December 21, 2018 02:23 Inactive
@BPScott BPScott temporarily deployed to polaris-react-pr-768 December 21, 2018 20:18 Inactive
@BPScott BPScott temporarily deployed to polaris-react-pr-768 December 21, 2018 22:25 Inactive
@BPScott BPScott temporarily deployed to polaris-react-pr-768 December 21, 2018 22:39 Inactive
@BPScott BPScott temporarily deployed to polaris-react-pr-768 December 21, 2018 23:33 Inactive
@BPScott BPScott temporarily deployed to polaris-react-pr-768 December 21, 2018 23:36 Inactive

### Testing on mobile or a virtual machine

1. Run `yarn dev:host`
1. Run `yarn dev`
Copy link
Member Author

Choose a reason for hiding this comment

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

storybook binds on all IP addresses so we don't need a special command to run on a particular IP anymore

README.md Outdated Show resolved Hide resolved
@solonaarmstrong-zz
Copy link

Just a couple small suggestions. This looks fantastic!

@BPScott BPScott temporarily deployed to polaris-react-pr-768 January 3, 2019 19:28 Inactive
@BPScott BPScott temporarily deployed to polaris-react-pr-768 January 3, 2019 19:38 Inactive
@BPScott
Copy link
Member Author

BPScott commented Jan 3, 2019

@kaelig: "Is webpack verbosity avoidable" - I've updated to run in "quiet mode" which removes most of the progress noise. You still get the chunk info but at least it doesn't hose your entire scrollback now.
As a side effect it fixes the "informational needs a new line before it item" Looks like v5 will be even quieter by default so I'm happy to wait for that.

.storybook/config.js Outdated Show resolved Hide resolved
.storybook/polaris-readme-loader.js Outdated Show resolved Hide resolved
.storybook/polaris-readme-loader.js Outdated Show resolved Hide resolved
.storybook/polaris-readme-loader.js Show resolved Hide resolved
scripts/pa11y.js Show resolved Hide resolved
@BPScott BPScott temporarily deployed to polaris-react-pr-768 January 3, 2019 21:21 Inactive
Copy link
Contributor

@danrosenthal danrosenthal left a comment

Choose a reason for hiding this comment

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

🎩 LGTM. I'll leave thorough review of the code to a web developer with more context.

My main feedback is there are a number of TODO comments in the code, and you mentioned a couple of things that needed fixing when we went over it during our pairing session. While I recognize those things don't need to be dealt with immediately, I would prefer issues were opened reflecting that work, vs. keeping it in your memory or documenting it in the code.

@BPScott BPScott temporarily deployed to polaris-react-pr-768 January 4, 2019 18:28 Inactive
Copy link
Contributor

@kaelig kaelig left a comment

Choose a reason for hiding this comment

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

LGTM!

@BPScott BPScott temporarily deployed to polaris-react-pr-768 January 4, 2019 19:17 Inactive
@AndrewMusgrave
Copy link
Member

Should we squash the commits into a feature commit to keep the commit history clean?

@BPScott
Copy link
Member Author

BPScott commented Jan 4, 2019

Should we squash the commits into a feature commit to keep the commit history clean?

Yep I was intending to do that just before I merge on monday, after we push our release out.

@BPScott BPScott temporarily deployed to polaris-react-pr-768 January 8, 2019 20:46 Inactive
Ran with the same command (`yarn dev`) this contains all the
functionality you know and love - hot reloading, a playground, percy
snapshots just with a different veneer
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants