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(components): sub in storybook for styleguidist #7549

Merged
merged 91 commits into from
May 3, 2021

Conversation

b-cooper
Copy link
Contributor

@b-cooper b-cooper commented Mar 26, 2021

Overview

This migrates the sandbox renderer for our component library from styleguidist to storybook.

In addition to working cleanly with TypeScript, Storybook has more features and flexibility compared with styleguidist.

Give it a whirl here

TODOs

  • create stories for all non-deprecated components
  • hook up storybook to build and deploy machinery
  • remove vestiges of styleguidist

Following Work

  • react-storybook's usage of react-docgen is not fully operational in this PR, a follow up should address this and take advantage of richer docstring parsing making it into the Docs tab of each component's story.
  • stories are currently being excluded from tsconfig as well as eslint to sidestep an issue with storybook's usage of emotion conflicting with our usage of styled-components. This should be mended soon, so that our stories can take advantage of linting and typechecking.

Changelog

  • make Component.stories.tsx for each non-deprecated component story in the library
  • hoist storybook functionality to the root of the repo to allow us to put stories anywhere across JS files in our mono-repo
  • remove all styleguidist markdown files
  • add TODO comments to components that didn't require stories, tag them for later reorganization

Review requests

  • [] try run make dev -C components locally and confirm that it starts the storybook dev server.

Risk assessment

low, no code should have been altered

@b-cooper b-cooper marked this pull request as ready for review April 15, 2021 19:52
@b-cooper b-cooper requested review from a team as code owners April 15, 2021 19:52
@b-cooper b-cooper requested review from Kadee80 and removed request for a team April 15, 2021 19:52
@b-cooper b-cooper added chore and removed WIP labels Apr 19, 2021
Copy link
Contributor

@mcous mcous left a comment

Choose a reason for hiding this comment

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

First review pass, 77 out of 114 files. I think the lack of linting and autoformatting on the stories is making this review a little harder, so opened #7708 to get that working

components/Makefile Outdated Show resolved Hide resolved
components/package.json Outdated Show resolved Hide resolved
components/src/alerts/alerts.css Outdated Show resolved Hide resolved
components/src/alerts/AlertItem.stories.tsx Show resolved Hide resolved
components/src/buttons/Button.tsx Outdated Show resolved Hide resolved
tsconfig-base.json Outdated Show resolved Hide resolved
install.sh Outdated Show resolved Hide resolved
components/src/tooltips/useTooltip.ts Outdated Show resolved Hide resolved
components/src/forms/SelectField.tsx Outdated Show resolved Hide resolved
components/src/forms/Select.css Show resolved Hide resolved
Copy link
Contributor

@mcous mcous left a comment

Choose a reason for hiding this comment

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

Took a pass at this, and looks good to me!

  • Can build storybook locally
  • Can run dev server locally
  • CI builds look like they're working
  • No source code changes to actual components

Lack of typechecking in components in annoying, but for now, VS Code still handles the files in isolation, so that's not the worst

Copy link
Member

@shlokamin shlokamin left a comment

Choose a reason for hiding this comment

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

Not sure what's going on but it seems like storybook isn't getting installed after a make setup-js. there is something weird happening after i do a make teardown-js, but i dont think that's the issue. Any idea what this could be?

Screen Shot 2021-04-29 at 3 17 46 PM

@@ -37,7 +37,7 @@ merge_timeout=300
esproposal.optional_chaining=enable
; default value of 19 (16 * 2^19 bytes > 8ish MB) isn't quite enough and can
; result in flow crashing with `Unhandled exception: SharedMem.Hash_table_full`
sharedmemory.hash_table_pow=20
sharedmemory.hash_table_pow=21
Copy link
Member

Choose a reason for hiding this comment

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

out of curiosity how come we gotta bump this up?

Copy link
Contributor

@mcous mcous Apr 29, 2021

Choose a reason for hiding this comment

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

Not bumping it can result in flow crashing with Unhandled exception: SharedMem.Hash_table_full 🙃

But yeah, as the codebase grew, we started seeing that particular flow crash. Added the config to bump it up to 20 in #4959. As Brian was working on this PR, he started seeing this crash again, and bumping it again (from 16 MB to 32 MB) seemed to resolve it.

Since Flow is on its way out, it seemed like the most straightforward thing to do to keep things chugging along rather than investigating it too deeply

@@ -1,7 +1,7 @@
# opentrons component library makefile

# dev server port
port ?= 8081
port ?= 6060
Copy link
Member

Choose a reason for hiding this comment

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

how come we're switching this to port 6060? upon googling looks like 6060 is used to indicate the use of the X Windows Protocol which i'd never heard of!

Copy link
Contributor

Choose a reason for hiding this comment

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

😬 so yeah, turns out we never used this $(port) variable. If you run make -C components dev in edge right now, the dev server will launch on port 6060. This PR changes this value to match the currently used port and actually uses it in the make dev target now

@mcous mcous requested a review from shlokamin April 29, 2021 20:35
@b-cooper b-cooper merged commit be85f3e into edge May 3, 2021
@b-cooper b-cooper deleted the components_add-storybook branch May 3, 2021 19:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore components Affects the `components` project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants