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

docs: upgrade storybook to 5.2.8 and refactor stories to CSF #508

Merged
merged 25 commits into from
Jan 24, 2020

Conversation

rshen91
Copy link
Contributor

@rshen91 rshen91 commented Jan 3, 2020

Summary

This PR is addressing issue #498. Stories have been refactored to be in CSF format and further documentation improvement has been broken into issue #523 to address in a subsequent PR.

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

  • Any consumer-facing exports were added to src/index.ts (and stories only import from ../src except for test data & storybook)
    - [ ] This was checked for cross-browser compatibility, including a check against IE11
  • Proper documentation or storybook story was added for features that require explanation or tutorials
  • Unit tests were updated or added to match the most common scenarios
  • Each commit follows the convention

@codecov-io
Copy link

codecov-io commented Jan 3, 2020

Codecov Report

Merging #508 into master will decrease coverage by 0.03%.
The diff coverage is 16.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #508      +/-   ##
==========================================
- Coverage   75.36%   75.33%   -0.04%     
==========================================
  Files         192      192              
  Lines        5769     5773       +4     
  Branches     1109     1115       +6     
==========================================
+ Hits         4348     4349       +1     
- Misses       1405     1408       +3     
  Partials       16       16
Impacted Files Coverage Δ
...ypes/partition_chart/renderer/canvas/partition.tsx 43.47% <16.66%> (-1.76%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eac67b2...d3596a5. Read the comment docs.

@rshen91
Copy link
Contributor Author

rshen91 commented Jan 6, 2020

Choose any story that uses knobs and it hangs with all knobs available disregarding defaults. After clicking any of the checkboxes, then storybook resolves to the correct knob configuration. Investigating...

In the network tab in developer tools, the hurdle may be linked to webpack where it's loading for 36.07s

The fix ended up being moving variables within the scope of each story vs having them as global variables

Other visual test issue was based on the config which has been changed instead of using loadStories.
Also, changes were made in the test files to correct for the changed url due to CSF changes for bar_chart

@rshen91
Copy link
Contributor Author

rshen91 commented Jan 13, 2020

Thank you @markov00 for the help with commit a75d477 🙇‍♀ and for reference: https://github.com/smrq/babel-plugin-require-context-hook/blob/master/register.js
We are not using babel, the tests are run in jest using puppeteer for the headless state.

Also, this commit includes readded integration tests since all.test.ts was setup differently (currently integration tests do not delete if outdated, so I deleted the entire image_snapshots directory, ran yarn jest:integration and committed the results)

@rshen91
Copy link
Contributor Author

rshen91 commented Jan 21, 2020

Based on previous conversations, we may just create a static table in bar_chart basic for the sake of this PR. I'm including some information on the 5.3.0 storybook release which may include something to incorporate in the future:

In terms of the story source being included in the docs section, there's a slightly relevant issue, but again with storybook 5.3.0 with the fix:

@rshen91
Copy link
Contributor Author

rshen91 commented Jan 22, 2020

@markov00 I have a very sketchy basic table included in bar_chart.tsx with the BarSeries props, and wanted to touch base about other props (in this table or in a new one) that would make sense to be included in the docs section for the basic bar chart.

In terms of the story source, it looks like there are some good paths to explore with MDX (but seems to be problematic with CSF docs rendering here: storybookjs/storybook#7479 (comment)). In commit eac67b2 I'm working on the built in Preview blocks with the docs addon. I'm having issues with the tags with "basic" not being defined. I think this might give us a code sample if I'm understanding correctly (storybookjs/storybook#7607). Let me know if you think this is the right path to go down and if you have any suggestions.

@rshen91 rshen91 marked this pull request as ready for review January 23, 2020 15:34
@rshen91 rshen91 changed the title feat: included basic bar chart docs build: upgrade storybook to 5.2.8 and refactor stories to CSF Jan 23, 2020
@rshen91
Copy link
Contributor Author

rshen91 commented Jan 23, 2020

As in earlier commit, a75d477, in recent commit d3596a5 I deleted the visual regression test image snapshots and reran the tests. There were url changes so the old snapshots would be slightly off.

@rshen91 rshen91 requested a review from markov00 January 23, 2020 18:45
Copy link
Member

@markov00 markov00 left a comment

Choose a reason for hiding this comment

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

LGTM tested locally

@rshen91 rshen91 changed the title build: upgrade storybook to 5.2.8 and refactor stories to CSF docs: upgrade storybook to 5.2.8 and refactor stories to CSF Jan 24, 2020
@rshen91 rshen91 merged commit 1111671 into elastic:master Jan 24, 2020
@rshen91 rshen91 deleted the rshen-add-docs branch January 24, 2020 20:31
@markov00
Copy link
Member

🎉 This PR is included in version 17.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@markov00 markov00 added the released Issue released publicly label Jan 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released Issue released publicly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants