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(storybook): Storybook 7, migrate docs from READMEs to stories, add guides #109

Merged

Conversation

sashathor
Copy link
Contributor

@sashathor sashathor commented Aug 13, 2023

Description of changes

This PR implements:

See preview

image

Checklist

Before merging to main:

  • Tests
  • Manually tested in React apps
  • Release notes
  • Approved

Release notes

# App changes

## Storybook

- Migrated to Storybook version 7.
- Fixed HMR support for the stories.
- Added a11y checker.
- Added documentation: Overview, Installation, Usage, Customization, Migration guide, Contributing, APIs.
- Added custom components documentation.
- Updated READMEs.
- Updated component stories.
- Added Propel's favicon.
- Added Propel's logo.
- Added access token endpoint that automatically generates a token to Tacasoft sample data for Connected components on every page load.
- Introduced a solution to display API props for non-primitive types.

@vercel
Copy link

vercel bot commented Aug 13, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
ui-kit ⬜️ Ignored (Inspect) Visit Preview Sep 19, 2023 6:21am

@sashathor sashathor marked this pull request as ready for review September 18, 2023 13:11
Copy link
Contributor

@felipecadavid felipecadavid left a comment

Choose a reason for hiding this comment

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

Overall it lgtm, I just have a question regarding axios and I think I found a bug in the storybook

app/storybook/package.json Show resolved Hide resolved
Comment on lines +11 to +13
await axiosInstance.get(tokenUrlValue).then((res) => {
setAccessToken(res.data.access_token)
})
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we use fetch here instead? What are the benefits of using axios?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a matter of personal preferences. It's a Storybook dependency and will not affect the bundle size of the libs. There are also some benefits of using axios:

  • Axios has built-in XSRF(Cross-Site Request Forgery) protection, while Fetch does not.
  • Axios has the ability to intercept HTTP requests but Fetch, by default, does not.
  • Axios allows canceling requests and request timeout but fetch does not.
  • IE11 support

Also, to make it work in Storybook it's required to use an addon to handle real API calls (not mocks). storybook-fetch-addon is 4 years old vs storybook-axios is 2 years old.

@felipecadavid
Copy link
Contributor

It seems like changing the theme is not working? Maybe this is not a feature for this PR and we'd want to remove this button instead?

Screen.Recording.2023-09-18.at.10.58.08.AM.mov

@sashathor
Copy link
Contributor Author

It seems like changing the theme is not working? Maybe this is not a feature for this PR and we'd want to remove this button instead?

Screen.Recording.2023-09-18.at.10.58.08.AM.mov

Yeah, we don't have theme support yet. I made a few experiments and this is a legacy. I've temporarily removed the theme switcher from the toolbar.

@sashathor sashathor changed the title WIP: feat(storybook): Storybook 7, migrate docs from READMEs to stories, add guides feat(storybook): Storybook 7, migrate docs from READMEs to stories, add guides Sep 19, 2023
sidebar: {
filters: {
patterns: (item) => {
return !item.tags?.includes('pattern')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The pattern tag is used to mark (hidden) stories that shouldn't be added to the Storybook's sidebar. We use it for some component stories that are part of the documentation (e.g. Leaderboard CustomStyleStory) and for non-primitive type stories (e.g. ChartBarProps).

@@ -0,0 +1,2 @@
// DO NOT REMOVE. This file is used in the build and deploy Storybook on Vercel.
// @TODO: look for more elegant solution
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Establishing the serverless function on Vercel for Storybook setup is a bit tricky.
First of all, there is an official instruction: Deploying Storybook with Vercel.
Secondly, Serverless Functions Quickstart.
The thing is that seems like Vercel runs some simple HTTP server for the Storybook setup, which can’t handle unbundled files (e.g. *.ts).
So, I’ve seen two main roads here:

  1. Configure a dummy Next.js project to handle calls to the functions and serve static Storybook content.
  2. Find a way to force Vercel’s Storybook env to handle function calls.
    I didn’t find a quick solution for 1 and honestly, I don’t like the idea of proxying calls to static files through the next.js env.
    I decided to follow 2. The initial assumption was to pre-build a script for the serverless function with tsc to the vercel’s api/ folder (Vercel’s reserved folder). The build workflow works without errors locally and on Vercel, but for some reasons, Vercel refuses to include tsc output in the final Deployment.
    What helped was the creation of an empty file (a placeholder) and adding it to the repo, e.g. api/token/route.js.
    With this hack Vercel started to include the function to the Deployment.

@sashathor sashathor merged commit 7ffff38 into main Sep 19, 2023
3 checks passed
@sashathor sashathor deleted the sasha/feat/PRO-2106_migrate-to-storybook-7-and-update-docs branch September 19, 2023 13:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants