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

[7.x] [Canvas] Storybook for testing and development (#29072) #34329

Merged
merged 4 commits into from
Apr 2, 2019

Conversation

clintandrewhall
Copy link
Contributor

@clintandrewhall clintandrewhall commented Apr 2, 2019

Backports the following commits to 7.x:

I made a mistake by not porting my Storybook PR to 7.x, because I had created it before 7.0 was cut. This PR fixes that oversight and adds Storybook and all of the component fixes, plus enables future component fixes.

clintandrewhall and others added 2 commits April 1, 2019 19:26
This PR adds [Storybook](https://storybook.js.org/) to our testing and development suite.

![screen shot 2019-01-21 at 4 35 32 pm](https://user-images.githubusercontent.com/297604/51502196-9f856780-1d9a-11e9-97bf-07c99c3f279b.png)

This will allow us to:

1. create a site outlining all components within Canvas, including their TS type information;
2. demonstrate usage of all components by example;
3. allow for individual component testing, both manually and by Jest;
4. iterate and fix bugs on individual components *without* having to start up Kibana, in a [HMR](https://webpack.js.org/concepts/hot-module-replacement/) environment;
5. automatically generate [snapshots](https://jestjs.io/docs/en/snapshot-testing) based on any examples written;

This PR also converts a few components to Typescript and adds examples.

I was inspired to add this when I was fixing elastic#25342.  In order to fix my changes, I had to run elasticsearch and kibana, as well as refresh my page whenever I needed to test a change.  Had I had a Storybook instance, I would have been done much faster.

In this PR, you'll see I converted `AdvancedFilter` from `renderers` and `FontPicker` and `ImageUpload` from `public/components`.  Would you believe I discovered and fixed bugs just by converting to Typescript and writing examples?

- `onChange` and `commit` are not marked as required in `propTypes`, but the component will error out if they're not supplied.
- `commit` was actually being called twice when 'Apply' was clicked.  This was shown in the 'Actions' panel when I was testing it.

- The `fonts` collection was not strongly-typed, therefore any string could be passed to the `value` parameter without error.
- While the code allows for any font string to be given to the component, there is no way to currently select that value, nor type it in within the control.  This is likely a bug in design.
- The `aria-labeledby` attribute in the drop down includes `undefined`.  This is likely a bug in EUI:

![screen shot 2019-01-21 at 4 25 58 pm](https://user-images.githubusercontent.com/297604/51501908-5ed91e80-1d99-11e9-913a-ce1bb5f4e352.png)

- `cd x-pack/plugins/canvas/`
- Run `node scripts/storybook` to start up a local development version, with HMR.
- Run `node scripts/storybook_build` to build a complete static version of the book.
- Run `node scripts/jest` which will run the Storyshots test; run `node scripts/jest --updateSnapshot` if source code has changed as expected.

- Adding Jest coverage and output to the info panels, ([this](https://www.npmjs.com/package/@storybook/addon-jest) is *sick* functionality).
- Adding automatic [a11y testing](https://www.npmjs.com/package/@storybook/addon-a11y), (currently [blocked](storybookjs/storybook#4889)).
- Adding generic knobs for stories
- Adding more example info, (e.g. who edited last, descriptions, etc).
@clintandrewhall clintandrewhall changed the title [7.x] [Canvas] Storybook for testing and development (#29072) #34326 [7.x] [Canvas] Storybook for testing and development (#29072) Apr 2, 2019
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@clintandrewhall
Copy link
Contributor Author

I tested this thoroughly in Chrome and IE 11.

@clintandrewhall clintandrewhall merged commit aecaffc into elastic:7.x Apr 2, 2019
@clintandrewhall clintandrewhall deleted the pr/34326 branch June 6, 2019 04:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants