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

feat: add Storybook #765

Merged
merged 24 commits into from
Jul 11, 2024
Merged

feat: add Storybook #765

merged 24 commits into from
Jul 11, 2024

Conversation

roushou
Copy link
Contributor

@roushou roushou commented Jul 9, 2024

close #758

@roushou roushou marked this pull request as ready for review July 9, 2024 23:42
@roushou
Copy link
Contributor Author

roushou commented Jul 9, 2024

This should be a good starting point.

yarn watch should be run alongside yarn storybook:dev to get live changes of styles.

Insight:

  • Imo TokenSearch should allow users to specify the source of tokens to search in. It also requires an api key to be able to do real search so current story is not fully functional.
  • Avatar has 2 loading states which is very weird, see story (refresh page on WithBadge story` nvm first loading is storybook loading the component
  • Badge doesn't render correctly when used individually, see story
  • Accessibility checks are context dependent i.e. TokenSelectDropdown doesn't have a11y issues because it's built with div tags and not select/options
  • Documentation is auto-generated using JSDoc annotations from components, it would be great to document components if necessary

I was thinking about mocking requests using service workers but I find that it's actually better to do real requests to know if things are broken or not. Let me know what you think.

Regarding CI, it can be done programmatically using test-runner or visually and collaboratively using Chromatic. I prefer the latter since it's visual but it depends on if you are looking to use Chromatic or not @fakepixels @Zizzamia

Comment on lines 17 to 19
parameters: {
layout: 'centered',
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's move this to .storybook/preview.ts? Looks like it is shared among all stories.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call. Moved all of them inside .storybook/preview.ts

Comment on lines +20 to +24
<WagmiProvider config={wagmiConfig}>
<QueryClientProvider client={new QueryClient()}>
<Story />
</QueryClientProvider>
</WagmiProvider>
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we move this to preview.ts as well?

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'd favor explicitness because:

  • Not every components needs to be wrapped inside
  • Some components only need to be wrapped inside QueryClientProvider

More verbose but it's clear what's needed when reading stories.

.gitignore Outdated
@@ -57,3 +57,5 @@ package-lock.json
# VitePress
docs/.vitepress/cache
docs/.vitepress/dist

*storybook.log
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd also add recommend ignoring storybook-static/*

# Storybook
*storybook.log
storybook-static/*

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call. I though running npx storybook init would update correctly .gitignore.

@Zizzamia
Copy link
Contributor

@roushou can you get the Build pass, please. So than I can review it.

@roushou
Copy link
Contributor Author

roushou commented Jul 11, 2024

@Zizzamia I can't reproduce locally. It seems to come from this line

run: yarn install --frozen-lockfile

Which makes build fail on purpose on CI

https://classic.yarnpkg.com/lang/en/docs/cli/install/#toc-yarn-install-frozen-lockfile

@@ -33,3 +33,4 @@
}

@tailwind utilities;
@tailwind components;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this? @roushou

We have not see the need to have those in a Tailwind application.

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 it's not needed here. I added it because I'm used to have it and I often define custom classes in @layer components but there's none in OnchainKit.

@Zizzamia
Copy link
Contributor

Ok I am going to take care of the Build issue.

@Zizzamia Zizzamia merged commit 05abc3a into coinbase:main Jul 11, 2024
5 of 8 checks passed
@roushou roushou deleted the storybook branch July 11, 2024 16:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Feature Request: add Storybook
3 participants