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

fix: fixes multiple storybook stories and updates to the new sb7 format #12683

Merged
merged 2 commits into from
Dec 6, 2023
Merged

fix: fixes multiple storybook stories and updates to the new sb7 format #12683

merged 2 commits into from
Dec 6, 2023

Conversation

stabildev
Copy link
Contributor

What does this PR do?

Replacement of #12628 to account for updates made in #12673 by @ThyMinimalDev (cc @emrysal)

A major change from Storybook 6 to Storybook 7 is the new separation of stories and documentation. Previously, stories and docs would be defined together in one .mdx file. With SB7, stories are defined in *.stories.tsx files and the corresponding docs are defined in a .mdx file. For the sake of clarity I chose the suffix .docs.mdx.

  • Converts the following stories to the new format (splits component into .stories.tsx and .docs.mdx)
    • Tooltip
    • Dialog
    • Booker
    • EventMeta
    • VerifyCodeDialog
  • Fixes the following stories
    • Badge (typo)
    • Breadcrumb (missing import)
    • Vertical Tabs (wrong import)
    • Color picker (redundant import TooltipProvider, see above)
    • Wizard story (unused import useRouter)
  • Adds TooltipProvider and StorybookTrpcProvider as global decorators to make them wrap all stories automatically
    • StorybookTrpcProvider mocks TRPC, esp. for the Booker
  • Migrates main.js and preview.jsx to the new typescript config format
  • Excludes storybook cover image and favicon from gitignore
  • Removes unnecessary GFM (Github flavored markdown) plugin
  • Comments out lazy build option (didn't work for me)
  • Fixes color picker test (await)

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • Chore (refactoring code, technical debt, workflow improvements)

@vercel Vercel
Copy link

vercel bot commented Dec 5, 2023

@stabildev is attempting to deploy a commit to the cal Team on Vercel.

A member of the Team first needs to authorize it.

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Dec 5, 2023 •
edited

Thank you for following the naming conventions! 🙏 Feel free to join our discord and post your PR link to collect XP and win prizes!

@stabildev stabildev mentioned this pull request Dec 5, 2023
Closed
3 tasks
@socket-security Socket Security
Copy link

Removed dependencies detected. Learn more about Socket for GitHub ↗︎

🚮 Removed packages: @radix-ui/react-select@0.1.1, @storybook/addon-mdx-gfm@7.6.3

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Dec 5, 2023

📦 Next.js Bundle Analysis for @calcom/web

This analysis was generated by the Next.js Bundle Analysis action. 🤖

This PR introduced no changes to the JavaScript bundle! 🙌

@ThyMinimalDev ThyMinimalDev self-requested a review December 5, 2023 22:38
@ThyMinimalDev
Copy link
Contributor

awesome work I'll take a look more in detail tomorrow

ThyMinimalDev
ThyMinimalDev reviewed Dec 5, 2023
View reviewed changes
packages/ui/components/form/color-picker/colorpicker.test.tsx
@@ -48,7 +48,7 @@ describe("Tests for ColorPicker component", () => {
const colorInput = screen.getByRole("textbox");
await act(async () => userEvent.clear(colorInput));
const newColorValue = "#00FF00";
await act(async () => userEvent.type(colorInput, newColorValue));
await act(async () => await userEvent.type(colorInput, newColorValue));
Copy link
Contributor

Choose a reason for hiding this comment

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

this seems like a recurring issue, this test is quite flaky, do we even need act ?

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 had an issue with this in my previous PR #12628 where I updated many more packages, including Vite from 4 to 5 in atoms, which I now moved to #12681 . I added this fix for completeness as I think this will pop up again. await is the correct way to handle this

Copy link
Contributor

Choose a reason for hiding this comment

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

@testing-library doc is quite explicit that act should not be necessary, need to double check why we need to chain await like that

ThyMinimalDev
ThyMinimalDev reviewed Dec 5, 2023
View reviewed changes
apps/storybook/.storybook/preview.tsx
Comment on lines +43 to +44
<StorybookTrpcProvider>
<TooltipProvider>
Copy link
Contributor

Choose a reason for hiding this comment

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

awesome 🔥

ThyMinimalDev
ThyMinimalDev reviewed Dec 5, 2023
View reviewed changes
apps/storybook/.storybook/main.ts
@@ -85,6 +89,8 @@ module.exports = {
},
Copy link
Contributor

Choose a reason for hiding this comment

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

what's your opinion on autodocs ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMO not needed for Cal.com since all stories have an explicit mdx

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 didn't see any difference when adding or omitting this option so I don't think it matters.

What was your reasoning behind lazy builds?

ThyMinimalDev
ThyMinimalDev approved these changes Dec 6, 2023
View reviewed changes
Copy link
Contributor

@ThyMinimalDev ThyMinimalDev left a comment

Choose a reason for hiding this comment

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

great work

@ThyMinimalDev ThyMinimalDev merged commit ca0d235 into calcom:main Dec 6, 2023
33 of 41 checks passed
hbjORbj pushed a commit to codemod-com/cal.com-demo that referenced this pull request Dec 21, 2023
@stabildev @hbjORbj
updates storybook config and fixes and updates stories (calcom#12683)
feb4856 
add packages
haranrk pushed a commit to haranrk/cal.com that referenced this pull request Jan 4, 2024
@stabildev @haranrk
updates storybook config and fixes and updates stories (calcom#12683)
b6aba9e 
add packages
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ThyMinimalDev ThyMinimalDev ThyMinimalDev 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.

None yet
2 participants
@stabildev @ThyMinimalDev