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: add documentation storybook for consumers #526

Merged
merged 24 commits into from
Feb 11, 2020

Conversation

rshen91
Copy link
Contributor

@rshen91 rshen91 commented Jan 24, 2020

Summary

This PR is addressing issue #523.
This documentation specific storybook can be run using yarn start-docs. The files for this storybook are in the /docs and .storybook-docs directories.

Just to keep in mind, the files in /docs are prefixed with numbers for consistent ordering. You can change the folder structure by changing the Meta title="" prop within each .mdx file.

For instance the pie chart currently has the Meta title prop set to <Meta title="Types of charts/Sunburts/Basic pie chart" />.

There is a charts.tsx file within /docs to copy over components from the /stories directory and import chart dependencies there vs. impacting the visual regression tests by importing through /stories directly. For the most part, the chart.tsx file isn't itself rendering documentation (expect these lines are important:

export default {
  title: 'Introduction',
  includeStories: [],
};

), the files that are doing so are the .mdx files within /docs.

Some tasks to be completed in this PR:

  • Set up yarn start-docs command to run separate storybook
  • Set up MDX formatting for stories
  • Set up props tables
  • Set up story sources that are accurate

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

@rshen91 rshen91 changed the title feat: add documentation storybook for consumers docs: add documentation storybook for consumers Jan 27, 2020
@rshen91 rshen91 marked this pull request as ready for review February 6, 2020 23:12
@rshen91 rshen91 requested a review from markov00 February 6, 2020 23:12
@elastic elastic deleted a comment from robgil Feb 11, 2020
@elastic elastic deleted a comment from robgil Feb 11, 2020
@elastic elastic deleted a comment from codecov-io Feb 11, 2020
@elastic elastic deleted a comment from robgil Feb 11, 2020
@elastic elastic deleted a comment from codecov-io Feb 11, 2020
@elastic elastic deleted a comment from robgil Feb 11, 2020
@elastic elastic deleted a comment from codecov-io Feb 11, 2020
@elastic elastic deleted a comment from codecov-io Feb 11, 2020
@elastic elastic deleted a comment from codecov-io Feb 11, 2020
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.

The overall structure looks good to me.
Before hosting the new docs, we need to clean, review and update them.
One major comment: we should try to avoid copy and paste of code and tables, instead we should prefer the mdx transclusion see https://mdxjs.com/getting-started#documents to import other mdx files inside a single mdx one (see storybookjs/storybook#7644 for issues with importing .md or .mdx)

You can import Chart components from the top-level Elastic Chart module.

```js
import { Axis, BarSeries, Chart, getAxisId, getSpecId, Position, ScaleType } from '@elastic/charts';
Copy link
Member

Choose a reason for hiding this comment

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

We can remove the getAxisId and getSpecId, and reorder this by usage like

Suggested change
import { Axis, BarSeries, Chart, getAxisId, getSpecId, Position, ScaleType } from '@elastic/charts';
import { Chart, BarSeries, ScaleType, Axis, Position } from '@elastic/charts';

Because we first add the Chart, then a BarSeries and the scaleType, then we add an axis and its position

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 5ae0913

## Consuming Elastic Charts


### Components
Copy link
Member

Choose a reason for hiding this comment

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

we can remove this title

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 5ae0913

@@ -0,0 +1,257 @@
import { Meta, Story } from "@storybook/addon-docs/blocks";
Copy link
Member

Choose a reason for hiding this comment

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

This overview is a bit outdated, let's for the moment leave it like that, I will review it later


<Chart className={'story-chart'}>
<BarSeries
id={getSpecId('bars1')}
Copy link
Member

Choose a reason for hiding this comment

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

we need to remove all getSpecsId/getAxisId etc because we don't need to use them anymore

Copy link
Contributor Author

Choose a reason for hiding this comment

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

addressed in 5ae0913



## Code for the basic bar chart
<details>
Copy link
Member

Choose a reason for hiding this comment

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

we can add the summary here and remove the title ## Code for the basic bar chart

with

<details>
<summary>
    Code
</summary>

...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in commit 5ae0913

</details>

***
## Prop Table for the basic bar chart
Copy link
Member

Choose a reason for hiding this comment

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

This comments can be applied to all the props table: the Props table refers to the BarSeries component not to the bar chart in general.
Few things that we can try/fix here:

  • from the title should be clear enough that we are referring to the BarSeries component Prop table, something like :BarSeries component Props table or <BarSeries /> props table should be used
  • I think we can use mdx transclusion to import an mdx file from somewhere else. It will be great if we can have a single file for each prop table and import just the table, so we don't have to copy/paste everything the same code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 5ae0913


| Prop | Type | Default | Note |
|:------|:------:|:---------:|:------|
| id `(required)` | `AxisId or string` | | The id of the spec |
Copy link
Member

Choose a reason for hiding this comment

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

I will review all the tables, but just to mention that this should not be an AxisId

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I caught them all! Addressed in 5ae0913


# Bar Chart with Axes

This chart now includes x and y axes. These axes can be `linear`, `ordinal` or `time`.
Copy link
Member

Choose a reason for hiding this comment

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

If we want to introduce the axes here, we should also introduce the Axis Props table

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right on - added in 5ae0913

@@ -9,7 +9,7 @@ const defaultProps = {
specType: SpecTypes.Series,
seriesType: SeriesTypes.Area,
groupId: DEFAULT_GLOBAL_ID,
xScaleType: ScaleType.Ordinal,
Copy link
Member

Choose a reason for hiding this comment

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

Nice find 👍

@codecov-io
Copy link

codecov-io commented Feb 11, 2020

Codecov Report

Merging #526 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #526   +/-   ##
=======================================
  Coverage   75.88%   75.88%           
=======================================
  Files         194      194           
  Lines        5879     5879           
  Branches     1143     1143           
=======================================
  Hits         4461     4461           
  Misses       1401     1401           
  Partials       17       17

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 510f58f...5ae0913. Read the comment docs.

@rshen91 rshen91 merged commit 75ff2ad into elastic:master Feb 11, 2020
@rshen91 rshen91 deleted the doc-storybook branch February 11, 2020 20:42
@markov00
Copy link
Member

🎉 This PR is included in version 17.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@markov00 markov00 added the released Issue released publicly label Feb 12, 2020
@markov00
Copy link
Member

🎉 This PR is included in version 17.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released on @17.1.x released Issue released publicly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants