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

chore(docs): introduce storybook and migrate react-tabs examples #873

Merged
merged 14 commits into from
Sep 11, 2020

Conversation

austingreendev
Copy link
Contributor

@austingreendev austingreendev commented Sep 8, 2020

Description

This PR begins our migration from multiple Styleguidist builds to a single Storybook instance. This will significantly reduce our build times and improve the development and consumer experience for our "knobs" style examples.

I've used the react-tabs package as the first migration example as it's already documented on the new website.

Detail

Storybook Config

storybook v6 includes a lot of new things and I've tried to stick as close to their opinionated defaults as possible. Some unique things to point out:

  • Storybook now handles Typescript compilation automatically πŸ₯³ They accomplish this with the @babel/preset-typescript preset and a TS checker that runs in a separate process.
    • Unfortunately this preset is unable handle mixed type exports. As we migrate packages over we may need to tweak our TS syntax to export these types separately, but they don't affect the compiled output in any way.
    • Some tsconfig and module alias changes were necessary for this to compile correctly. This shouldn't affect our build output, but a lot of this complexity can be removed once styleguidist is fully gone.
  • I've added an eslint-loader and svgr to the default webpack config
  • The manager and preview themes have been modified to use our font-family, sizing, and default color set
  • The "essential" addons are included by default, but I've also included the a11y-addon which includes an axe panel showing applicable rules as well as a color filter dropdown.

Story format

For each package we can include the README similar to stylguidist. I've also created a basic example format which:

  • Allows a story to be configured via control knobs
  • Can be tested with RTL and Bedrock styling
  • Shows the prop-sheets for all related components (excluding HTMLAttribute and other extensions)

Screen Shot 2020-09-08 at 10 29 07 AM

Screen Shot 2020-09-08 at 10 29 41 AM

Screen Shot 2020-09-08 at 10 29 19 AM

TODO

  • Fix README reference link formatting
    • Removed root README
  • Correct IE11 issues (something to do with our babel config customization)
    • Now extend the default storybook babel config
  • Once a format is decided, choose whether to update the template package at this time

Checklist

  • πŸ‘Œ design updates are Garden Designer approved (add the
    designer as a reviewer)
  • 🌐 Styleguidist demo is up-to-date (yarn start)
  • ⬅️ renders as expected with reversed (RTL) direction
  • 🀘 renders as expected with Bedrock CSS (?bedrock)
  • β™Ώ analyzed via axe and evaluated using VoiceOver
  • πŸ’‚β€β™‚οΈ includes new unit tests
  • πŸ“ tested in Chrome, Firefox, Safari, Edge, and IE11

.circleci/config.yml Show resolved Hide resolved
@@ -1 +0,0 @@
packages/notifications/index.d.ts
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file should have been removed with the v8 release

ref
) => {
const theme = useContext(ThemeContext);
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 updated the theme reference logic here to align with the more recent components. This doesn't affect the public API.


export default {
title: 'Components/Tabs',
subcomponents: { Tabs, TabList, Tab, TabPanel },
Copy link
Contributor Author

Choose a reason for hiding this comment

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

By only listing subcomponents we don't have to override the arguments that we don't want to display in the story controls.

@zendesk-garden zendesk-garden temporarily deployed to staging September 8, 2020 18:09 Inactive
@coveralls
Copy link

coveralls commented Sep 8, 2020

Coverage Status

Coverage decreased (-0.3%) to 95.82% when pulling 7a81311 on agreen/storybook into 8f0c098 on main.

Copy link
Member

@jzempel jzempel left a comment

Choose a reason for hiding this comment

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

Just a quick run-through while this is in draft mode. This is going to be a very nice QoL improvement.

.eslintrc.json Outdated Show resolved Hide resolved
.storybook/babel.config.js Outdated Show resolved Hide resolved
.storybook/gardenTheme.js Outdated Show resolved Hide resolved
.storybook/manager-head.html Outdated Show resolved Hide resolved
Comment on lines 54 to 77
locale: {
name: 'direction',
description: 'Locale direction',
defaultValue: 'ltr',
toolbar: {
icon: 'globe',
items: [
{ value: 'ltr', title: 'LTR' },
{ value: 'rtl', title: 'RTL' }
]
}
},
bedrock: {
name: 'bedrock',
description: 'CSS Bedrock',
defaultValue: 'disabled',
toolbar: {
icon: 'paintbrush',
items: [
{ value: 'disabled', title: 'Disabled' },
{ value: 'enabled', title: 'Enabled' }
]
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Can these be made to work like the "Grid" toggle rather than choosing from a menu?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking through their docs I haven't been able to find anything about different global toolbar types. Since it's an essential plugin it seems they have a deeper integration than we do in preview.js. I'll keep looking through their docs though.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without a fully-custom addon I'm not sure we will be able to replicate this. Feels like it could be a good follow-on before we would make the global cut.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, let's capture the task and see if we can make something of it. Just having the functionality is fine for now.

demo/index.html Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
packages/tabs/stories/Tabs.stories.tsx Outdated Show resolved Hide resolved
utils/styleguide/public/images/garden.svg Show resolved Hide resolved
utils/test/garden-test-utils.tsx Outdated Show resolved Hide resolved
@zendesk-garden zendesk-garden temporarily deployed to staging September 8, 2020 21:12 Inactive
@austingreendev austingreendev marked this pull request as ready for review September 9, 2020 18:52
@austingreendev
Copy link
Contributor Author

@jzempel IE11 support could be tricky here. I'm now extending the default babel configuration provided by Storybook, but it seems they have an IE11 regression in the most recent release storybookjs/storybook#12179

I feel comfortable going forward with this knowing that all of our internal consumers have recently ended IE11 support. We should still be conscious of it in the main site, but we should be ok in the short-term here.

Copy link
Contributor

@hzhu hzhu left a comment

Choose a reason for hiding this comment

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

Looking good πŸ‘

packages/tabs/README.md Show resolved Hide resolved
@zendesk-garden zendesk-garden temporarily deployed to staging September 9, 2020 19:25 Inactive
@zendesk-garden zendesk-garden temporarily deployed to staging September 9, 2020 20:47 Inactive
@zendesk-garden zendesk-garden temporarily deployed to staging September 11, 2020 00:10 Inactive
@jzempel
Copy link
Member

jzempel commented Sep 11, 2020

IE11 support could be tricky here.

Oh no. What's the plan for Garden to get new components out the door with guaranteed IE11 support (still needed here, unfortunately)?

Copy link
Member

@jzempel jzempel left a comment

Choose a reason for hiding this comment

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

Nice, improved, future-proof config πŸŽ‰ Also, README is looking niiice.

.circleci/config.yml Show resolved Hide resolved
.storybook/gardenTheme.js Outdated Show resolved Hide resolved
.storybook/preview.js Outdated Show resolved Hide resolved
.storybook/preview.js Outdated Show resolved Hide resolved
@zendesk-garden zendesk-garden temporarily deployed to staging September 11, 2020 18:21 Inactive
Copy link
Member

@jzempel jzempel left a comment

Choose a reason for hiding this comment

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

[nit] Should the "change preview background" grid icon remain visible in the header? Doesn't look like it does anything.

This.is.awesome.

@austingreendev
Copy link
Contributor Author

change preview background

This should be removed right now. I've disabled the backgrounds plugin which I believe is where this is coming from. We may need to customize further once we look into tweaking the CSS Bedrock dropdown.

@austingreendev austingreendev merged commit 4e57981 into main Sep 11, 2020
@austingreendev austingreendev deleted the agreen/storybook branch September 11, 2020 18:43
@jzempel
Copy link
Member

jzempel commented Sep 11, 2020

This should be removed right now.

The latest deployment sez otherwise 🀷

@austingreendev
Copy link
Contributor Author

You're correct. It should be gone though. Will add it to the CSS Bedrock task to look into this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

5 participants