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

Move and include @storybook/components in standalone Storybook #19598

Merged

Conversation

JReinhold
Copy link
Contributor

@JReinhold JReinhold commented Oct 24, 2022

This PR continues the work from #19465 and adds the components to the mix.

What I did

  • Moved @storybook/components to code/ui/components.
  • Simplified the main configuration as we didn't need the classic runtime after all.

Most notable is the fact that some stories in MDX currently doesn't work. The typography stories contained code fences which I had to remove, because SyntaxHighlighter2 wasn't working, which hints that we might have issues with double imports when we have stories that include doc blocks. This makes sense and is somewhat expected when running a Storybook for Storybook. This is only an issue in mdx files though, and not even auto-generated docs, so I've disabled the offending features for now and might look into it later if I have more time.

How to test

cd code
yarn storybook:ui

@@ -17,42 +24,6 @@ const config: StorybookConfig = {
name: '@storybook/react-vite',
options: {},
},
viteFinal: (config) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

all this code was just to apply jsxRuntime: 'classic' but it turns out it wasn't needed after all, is was just a misinterpretation of warnings and errors on my end.

@@ -9,8 +9,8 @@ import {
convert,
styled,
useTheme,
} from '@storybook/theming';
import { Symbols } from '@storybook/components';
} from '../../lib/theming';
Copy link
Contributor Author

@JReinhold JReinhold Oct 24, 2022

Choose a reason for hiding this comment

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

ensures we've only got one instance of emotion running, and a host of other problems related to double imports are fixed.

Copy link
Member

Choose a reason for hiding this comment

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

🤔 are there symlinks involved @JReinhold ? How is this SB installed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there might be!
The Storybook is started with cwd at code/ and uses all the modules from code/node_modules, which I guess in link mode are symlinks. The actual SB config is at code/ui/.storybook, and it's all tied together with this yarn script:

    "storybook:ui": "./lib/cli/bin/index.js dev --port 6006 --config-dir ./ui/.storybook --no-manager-cache",

If I recall correctly the reason cwd is code and not code/ui is because Vite is looking for modules at cwd, and we don't have any node_modules in code/ui, besides the Vite cache.

Copy link
Member

Choose a reason for hiding this comment

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

You could try running with NODE_OPTIONS="--preserve-symlinks --preserve-symlinks-main" and see if it helps.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Holy s**t that worked, even fixed the issue with SyntaxHighlighter2 being undefined which has driven me nuts.
You have to explain the logic behind this, this doesn't match at all with what I thought the problem was.

Copy link
Member

Choose a reason for hiding this comment

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

I can explain what it does. I have no idea why it fixes the problem but I'm sure we can work that out together :)

@@ -0,0 +1,13 @@
import { Badge } from './Badge';
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 following stories in this PR have just been moved, and many of them have been converted from storiesOf to CSF3.

The diff shows completely new content, not really true.

@@ -1,7 +1,7 @@
import React from 'react';

import { DocumentWrapper } from './DocumentWrapper';
import MarkdownSample from './DocumentFormattingSample.md';
import MarkdownSample from './DocumentFormattingSample.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.

required to make Prettier format MDX2 comments correctly.

@JReinhold JReinhold self-assigned this Oct 24, 2022
@JReinhold JReinhold added the build Internal-facing build tooling & test updates label Oct 24, 2022
@JReinhold
Copy link
Contributor Author

I'm very confused as to why this PR adds two new stories to all our sandbox Chromatics - a Primary and Secondary button in iframes..

@tmeasday any ideas?

@JReinhold JReinhold marked this pull request as ready for review October 24, 2022 11:47
@JReinhold JReinhold changed the title Jeppe/sb-604-create-a-standalone-storybook-for-our-phase-2 Move and include IncluJeppe/sb-604-create-a-standalone-storybook-for-our-phase-2 Oct 24, 2022
@JReinhold JReinhold changed the title Move and include IncluJeppe/sb-604-create-a-standalone-storybook-for-our-phase-2 Move and include @storybook/components in standalone Storybook Oct 24, 2022
@tmeasday
Copy link
Member

I'm very confused as to why this PR adds two new stories to all our sandbox Chromatics - a Primary and Secondary button in iframes..

@tmeasday any ideas?

@JReinhold I think it's something unrelated because we briefly ran out of snapshots on Chromatic and a few builds on next were skipped. @shilman accepted them all here: #19600, I suspect if you update from next you should be OK.

…-604-create-a-standalone-storybook-for-our-phase-2
Copy link
Member

@tmeasday tmeasday left a comment

Choose a reason for hiding this comment

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

Looks great!

One question: are we testing these SBs with Chromatic? We should set that up.

@@ -9,8 +9,8 @@ import {
convert,
styled,
useTheme,
} from '@storybook/theming';
import { Symbols } from '@storybook/components';
} from '../../lib/theming';
Copy link
Member

Choose a reason for hiding this comment

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

🤔 are there symlinks involved @JReinhold ? How is this SB installed?

import { Badge } from './Badge';

export default {
title: 'Basics/Badge',
Copy link
Member

Choose a reason for hiding this comment

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

We should move towards using autotitle for all our stories (not necessarily in this PR)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As you probably noticed I did this in #19615 where possible.

@JReinhold
Copy link
Contributor Author

One question: are we testing these SBs with Chromatic? We should set that up.

I'm working on this as we speak, in a follow up PR.

@JReinhold JReinhold merged commit 9d6b4e6 into next Oct 25, 2022
@JReinhold JReinhold deleted the jeppe/sb-604-create-a-standalone-storybook-for-our-phase-2 branch October 25, 2022 08:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Internal-facing build tooling & test updates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants