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: enable story source for current stories #557

Merged
merged 49 commits into from
Feb 25, 2020

Conversation

rshen91
Copy link
Contributor

@rshen91 rshen91 commented Feb 18, 2020

Summary

This PR enables the story source add-on for stories while consumer facing storybook is in progress.

Visual regression tests were rerun because of the url naming changes with changing the fs for /stories/.

The webpack config was edited to add story-source for each folder containing separate stories. Please feel free to propose a more succinct solution in the webpack config for the 14 or so folders (wildcard didn't work in testing).

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

Move `Datum`, `Rotation`, `Position` and `Color` to `utils/commons`. Decouple legend from axis position method and move the `scales` to `utils/scales`.
@markov00
Copy link
Member

Hey @rshen91 I've seen that we can maybe do a cleaner path instead of reenabling the info addon that bring in some issues: we can use the StorySource addon that adds a new tab near the Knobs with the code listed directly from the story.
The minor drawback here is that, to avoid long waiting times caused by our very long story source files we should split the stories into multiple files (one per story) that is easy to read and display.
I've tested it out with the scales story that is the shortest one, and works fine. These are the passages I've done:

  • add the addon: yarn add @storybook/addon-storysource --dev
  • register it in .storybook/addons.js import '@storybook/addon-storysource/register';
  • add the source loader for webpack (I've added it as the first rule, but not sure if this can be used in a different order:
config.module.rules.push({
    test: /\.tsx?$/,
    include: [path.resolve(__dirname, '../stories', 'scales')],
    loaders: [
      {
        loader: require.resolve('@storybook/source-loader'),
        options: { parser: 'typescript' },
      },
    ],
    enforce: 'pre',
  });
  • I've created a scales folder and I've splitted each story in its own file, adding a 1_, 2_ prefix on each file to order them as it was before

For me it seems to render a more readable and usable code than the addon-info
Screenshot 2020-02-19 at 11 04 57

@markov00 markov00 added wip work in progress :docs Anything related to documentation, API, storybook labels Feb 19, 2020
@codecov-io
Copy link

codecov-io commented Feb 19, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@3117a11). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #557   +/-   ##
=========================================
  Coverage          ?   71.82%           
=========================================
  Files             ?      212           
  Lines             ?     6143           
  Branches          ?     1181           
=========================================
  Hits              ?     4412           
  Misses            ?     1712           
  Partials          ?       19

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 3117a11...ba1f285. Read the comment docs.

rshen91 and others added 19 commits February 19, 2020 11:28
This commit will decouple the tooltip component from the XY chart to allow Partition and other chart type an ease use.

BREAKING CHANGE: the `SeriesIdentifier` type is generalized into a simplified object with two values in common: `specId` and `key`. A specialized `XYChartSeriesIdentifier` extends now the base `SeriesIdentifier`. The `SettingsSpec` prop `showLegendDisplayValue` is renamed to `showLegendExtra` and its default value is now `false` hiding the current/last value on the legend by default.

close elastic#246
@markov00 markov00 force-pushed the story-source branch 2 times, most recently from 4d73ff3 to 03c227c Compare February 24, 2020 22:55
Copy link
Contributor Author

@rshen91 rshen91 left a comment

Choose a reason for hiding this comment

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

This looks awesome! Thanks for fixing the naming and making it much more intuitive

  • I like how Annotations is broken into lines and rects.
  • Also, the info text is much cleaner now too.
  • I also like the selectedPanels addition - I think it's an improvement for the user experience.

Copy link
Collaborator

@nickofthyme nickofthyme 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 in Chrome and Firefox. Looked through dozens of stories and all seem to be fine. All screenshots are just renamed with no changes.

The separation of story files vastly improves the source load time.

Love the linting rule. Finally someone listened to me 😭

@@ -39,7 +39,7 @@ export const Basic = () => {
<Chart className={className}>
<BarSeries
id={specId}
name={'Simple bar series'}
name="Simple bar series"
Copy link
Collaborator

Choose a reason for hiding this comment

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

😍❤️💝💕

cry

@@ -4,22 +4,22 @@ describe('Annotations stories', () => {
describe('rotation', () => {
it('rotation - 0', async () => {
await common.expectChartAtUrlToMatchScreenshot(
'http://localhost:9001/?path=/story/annotations--test-line-annotation-single-value-histogram&knob-debug=&knob-chartRotation=0',
'http://localhost:9001/?path=/story/annotations-lines--single-bar-histogram&knob-debug=&knob-chartRotation=0',
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

@@ -5,7 +5,7 @@
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
Collaborator

Choose a reason for hiding this comment

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

🎉

wiki/overview.md Outdated
@@ -48,18 +48,18 @@ A spec can be something like the following:
```jsx
<Chart renderer={renderer}>
<Settings rotation={rotation} animateData={true} />
<Axis id={getAxisId('bottom')} position={AxisPosition.Bottom} title={`Rendering test`} />
<Axis id={getAxisId('left')} position={AxisPosition.Left} />
<Axis id={('bottom')} position={AxisPosition.Bottom} title={`Rendering test`} />
Copy link
Collaborator

@nickofthyme nickofthyme Feb 25, 2020

Choose a reason for hiding this comment

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

??? no linting on mds 😞

Might as well clean this up when you update the branch

@markov00 markov00 merged commit d4f31b8 into elastic:master Feb 25, 2020
@rshen91 rshen91 deleted the story-source branch March 2, 2020 15:12
markov00 added a commit that referenced this pull request Mar 2, 2020
This commit add the storySource addon to visualize the source code of each stories on the right panel

Co-authored-by: Marco Vettorello <vettorello.marco@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:docs Anything related to documentation, API, storybook
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants