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

Storybook for Storybook - step 1: ui/manager #19465

Merged
merged 16 commits into from
Oct 14, 2022

Conversation

JReinhold
Copy link
Contributor

@JReinhold JReinhold commented Oct 12, 2022

This PR creates a Storybook for the manager UI.

  • Create a directory code/ui which will contain multiple packages.
  • For now lib/ui has been moved to ui/manager, but in follow-up PRs we'll move lib/components/, lib/docblocks and maybe also lib/router.
  • We have a new Storybook set up at code/ui/.storybook based on react/vite which will be used for ui, components, and docblocks.

The Storybook can be started with

cd code
yarn storybook:ui

(for now)

@JReinhold JReinhold self-assigned this Oct 13, 2022
Comment on lines 43 to 52
"addons/storyshots/*",
"frameworks/*",
"renderers/*",
"presets/*",
"examples-native/*",
"examples/*",
"examples-native/*",
"frameworks/*",
"lib/*",
"lib/cli/test/run/*"
"lib/cli/test/run/*",
"ui/*",
"presets/*",
"renderers/*"
]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

added ui/*, ordered all alphabetically

useTheme,
} from '@storybook/theming';
import { Symbols } from '@storybook/components';

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 file is mostly just copied from the old official storybook example. I did however remove the logic for showing warnings about missing env.

@@ -1,7 +1,7 @@
import React, { FC, useMemo } from 'react';
import sizeMe from 'react-sizeme';

import { State } from '@storybook/api';
import { type State } from '@storybook/api';
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 of these type imports were just an attempt at fixing eslint warnings

width,
...(mocked ? mockProps : realProps),
};
const props = mocked ? mockProps : realProps;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TS correctly complained that height and width were always being overwritten because mockProps and realProps both contain them.

Copy link
Member

Choose a reason for hiding this comment

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

Thank you TypeScript! ♥️

);
return <Zoom key="zoom" {...{ zoomIn, zoomOut, reset }} />;
});
const ZoomWrapper = React.memo<{ set: (zoomLevel: number) => void; value: number }>(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

butchered diff. the only change here is the better typing of the set function.

@JReinhold JReinhold changed the title WIP: Storybook for Storybook step 1: manager/ui Storybook for Storybook - step 1: manager/ui Oct 14, 2022
@JReinhold JReinhold changed the title Storybook for Storybook - step 1: manager/ui Storybook for Storybook - step 1: ui/manager Oct 14, 2022
@JReinhold JReinhold added the build Internal-facing build tooling & test updates label Oct 14, 2022
@JReinhold JReinhold requested a review from ndelangen October 14, 2022 11:19
@JReinhold JReinhold marked this pull request as ready for review October 14, 2022 11:20
@@ -284,6 +287,7 @@
"fs-extra": "^9.0.1",
"github-release-from-changelog": "^2.1.1",
"glob": "^7.1.6",
"global": "^4.4.0",
Copy link
Member

Choose a reason for hiding this comment

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

I think this dependency was something we were in the process of removing entirely, correct @shilman?

and adding them back in, but with the `jsxRuntime: 'classic'` option.
TODO: When we've upgraded to React 18 all of this shouldn't be necessary anymore
*/
plugins: [...withoutReactPlugins(config.plugins), vitePluginReact({ jsxRuntime: 'classic' })],
Copy link
Member

Choose a reason for hiding this comment

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

Let's (in a follow-up PR) extract this code into the react-vite framework.
We can toggle this behavior in a frameworkOption.

@shilman perhaps this is something we should do this cycle, or the next? And maybe we need a repro for react16+vite, if we do not have one yet.

We could also say react16+vite is unsupported, but that might be a bit too aggressive? (considering we ourselves are using it 🤡

@@ -0,0 +1,4 @@
<script>
// eslint-disable-next-line no-undef
window.global = window;
Copy link
Member

Choose a reason for hiding this comment

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

Let's make it a target for SB7.0 to remove this requirement?

@shilman ☝️ ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@JReinhold JReinhold merged commit 6429309 into next Oct 14, 2022
@JReinhold JReinhold deleted the jeppe/sb-604-create-a-standalone-storybook-for-our branch October 14, 2022 12:31
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