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.

Sign up for GitHub

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

Jump to bottom

feat: add storybook #115

Merged
merged 1 commit into from
Jul 7, 2021
Merged

feat: add storybook #115

merged 1 commit into from
Jul 7, 2021

Conversation

fbwoolf
Copy link
Contributor

@fbwoolf fbwoolf commented May 28, 2021 •
edited
Loading

Description

This PR adds Storybook to the UI library making the following changes:

  1. Installed required dependencies
  2. Setup scripts to run/build Storybook
  3. Default Storybook configuation
  4. Some boilerplate stories to get started

For details refer to issue #50

Type of Change

  • New feature
  • Bug fix
  • API reference/documentation update
  • Other

Does this introduce a breaking change?

No.

Are documentation updates required?

TBD.

@changeset-bot
Copy link

changeset-bot bot commented May 28, 2021 •
edited
Loading

🦋 Changeset detected

Latest commit: 4638481

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 4 packages
Name Type
@stacks/ui Minor
@stacks/ui-core Minor
@stacks/ui-theme Minor
@stacks/ui-utils Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@vercel
Copy link

vercel bot commented May 28, 2021 •
edited
Loading

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/blockstack/blockstack-ui/4kQ6Ak44tqy8FuWNtNZ8dCSzU7TN
✅ Preview: Failed

[Deployment for f7522f8 failed]

@fbwoolf fbwoolf linked an issue May 28, 2021 that may be closed by this pull request
[ui] Add stories to document the components #50
Closed
@fbwoolf fbwoolf requested review from aulneau and kyranjamie May 28, 2021 17:51
@kyranjamie
Copy link
Contributor

Nice one @fbwoolf.

My preference would be to put the stories of a given component in with the component itself. Thoughts?

@fbwoolf
Copy link
Contributor Author

fbwoolf commented Jun 1, 2021

Nice one @fbwoolf.

My preference would be to put the stories of a given component in with the component itself. Thoughts?

Yeah, I like that idea. I can do a sample story for one of our components for this PR and remove the generated examples.

@fbwoolf
Copy link
Contributor Author

fbwoolf commented Jun 8, 2021

Was going to try and wrap this up, realized maybe this shouldn't be at the root level but just in the ui package with the components?

@kyranjamie
Copy link
Contributor

Makes sense. I think stories should be coupled as close to components themselves as possible.

@fbwoolf fbwoolf force-pushed the feat/add-storybook branch from fe3bd4f to a3413f8 Compare June 11, 2021 19:36
@fbwoolf
Copy link
Contributor Author

fbwoolf commented Jun 11, 2021

@aulneau @kyranjamie this can be reviewed again. I dug in a bit to how best handled with a monorepo and was suggested to leave installed at the root, but move stories into the packages where needed. I removed the default stories and created a sample button.stories.tsx in the ui package. We can add other packages to the config if needed but just targeted ui for now.

@fbwoolf fbwoolf force-pushed the feat/add-storybook branch from a3413f8 to c9d731d Compare June 11, 2021 20:21
kyranjamie
kyranjamie approved these changes Jun 14, 2021
View reviewed changes
Copy link
Contributor

@kyranjamie kyranjamie left a comment

Choose a reason for hiding this comment

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

LGTM @fbwoolf—though what are the jsxRuntime comments for, can that be configured elsewhere than comments?

@fbwoolf
Copy link
Contributor Author

fbwoolf commented Jun 14, 2021

LGTM @fbwoolf—though what are the jsxRuntime comments for, can that be configured elsewhere than comments?

I was running into this error Syntax error: pragma and pragmaFrag cannot be set when runtime is automatic. and that was the suggested solution I found but let me look if there is a better way to do it.

@fbwoolf
Copy link
Contributor Author

fbwoolf commented Jun 16, 2021

LGTM @fbwoolf—though what are the jsxRuntime comments for, can that be configured elsewhere than comments?

I ended up trying a few other suggestions, but nothing worked. This is where I found the current solution: emotion-js/emotion#2041 (comment) @aulneau any thoughts or ok with what I added?

@fbwoolf fbwoolf force-pushed the feat/add-storybook branch from c9d731d to c1531db Compare June 16, 2021 19:34
@fbwoolf fbwoolf force-pushed the feat/add-storybook branch from c1531db to 36ad56b Compare June 20, 2021 20:15
@fbwoolf fbwoolf force-pushed the feat/add-storybook branch from 36ad56b to fa732e1 Compare June 21, 2021 13:45
aulneau
aulneau suggested changes Jun 21, 2021
View reviewed changes
Copy link
Contributor

@aulneau aulneau left a comment

Choose a reason for hiding this comment

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

This is great, I'd just prefer to make any pragma changes to storybook config instead of the components themselves

packages/ui/src/spinner.tsx Outdated
@@ -1,3 +1,4 @@
/** @jsxRuntime classic */
Copy link
Contributor

Choose a reason for hiding this comment

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

would this work instead? https://duncanleung.com/emotion-css-prop-jsx-pragma-storybook/

or maybe this? storybookjs/storybook#12881 (comment) setting it to "classic"?

Copy link
Contributor

Choose a reason for hiding this comment

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

or perhaps this storybookjs/storybook#12952 (comment)

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 tried a few things, but can't remem if any of these specifically ...let me try.

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 tried these but can't get them to work.

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 ended up working: storybookjs/storybook#7540 (comment)

@fbwoolf fbwoolf force-pushed the feat/add-storybook branch from fa732e1 to c498486 Compare June 22, 2021 01:35
@aulneau
Copy link
Contributor

aulneau commented Jun 22, 2021

@fbwoolf awesome, feel free to merge

@fbwoolf
Copy link
Contributor Author

fbwoolf commented Jun 22, 2021

@fbwoolf awesome, feel free to merge

How do I get past the deploy failure to merge it?

@fbwoolf fbwoolf force-pushed the feat/add-storybook branch from c498486 to f7522f8 Compare June 28, 2021 14:01
@aulneau
Copy link
Contributor

aulneau commented Jul 7, 2021

@fbwoolf sorry seems like there are conflicts -- can you rebase? I've disconnected vercel so that should no longer fail

@fbwoolf
Copy link
Contributor Author

fbwoolf commented Jul 7, 2021

@fbwoolf sorry seems like there are conflicts -- can you rebase? I've disconnected vercel so that should no longer fail

Yep, will do. Thx!

@fbwoolf fbwoolf force-pushed the feat/add-storybook branch from f7522f8 to 4638481 Compare July 7, 2021 17:57
@fbwoolf
Copy link
Contributor Author

fbwoolf commented Jul 7, 2021

@fbwoolf sorry seems like there are conflicts -- can you rebase? I've disconnected vercel so that should no longer fail

@aulneau I rebased but the merge button is disabled for me.

@aulneau aulneau merged commit 06dd54e into main Jul 7, 2021
@github-actions github-actions bot mentioned this pull request Jul 7, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aulneau aulneau aulneau approved these changes

@kyranjamie kyranjamie kyranjamie approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[ui] Add stories to document the components
3 participants
@fbwoolf @kyranjamie @aulneau