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

cli: colocate stories and components, centralize main.js #10913

Closed
wants to merge 56 commits into from

Conversation

tooppaaa
Copy link
Contributor

@tooppaaa tooppaaa commented May 25, 2020

Issue: #10901

What I did

  • Move components from @storybook/< framework >/demo to cli/frameworks/react/js and /ts
  • Copy stories folder to user src (if any) or to root
  • mutualise generator

How to test

  • E2E should be green
  • use CLI

@tooppaaa tooppaaa added the cli label May 25, 2020
@tooppaaa tooppaaa self-assigned this May 25, 2020
@tooppaaa tooppaaa added the maintenance User-facing maintenance tasks label May 25, 2020
Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

This looks great @tooppaaa! Some small nitpicks in the review, but otherwise feels good.

lib/cli/src/frameworks/react/js/0-Welcome.stories.js Outdated Show resolved Hide resolved
HTMLAttributes,
} from 'react';

type MainProps = Omit<DetailedHTMLProps<HTMLAttributes<HTMLElement>, HTMLElement>, 'style'>;
Copy link
Member

Choose a reason for hiding this comment

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

Can we simplify the TypeScript in our welcome component? Otherwise it's very welcoming!

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 have in mind of having Welcome being a mdx story and having another more simple/friendly component. Not sure how to simplify this one

Copy link
Member

Choose a reason for hiding this comment

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

Agree with @shilman a simpler type would be great but I'm a React newbie so I don't if we can do better or if there is a workaround.

lib/cli/src/frameworks/react/ts/Welcome.tsx Outdated Show resolved Hide resolved
@@ -54,4 +56,58 @@ describe('Helpers', () => {
}).toThrowError(expectedMessage);
});
});

it.each`
language | exists | expected
Copy link
Member

Choose a reason for hiding this comment

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

WHOA!!! 😍

@tooppaaa
Copy link
Contributor Author

tooppaaa commented Jun 8, 2020

#9103 is also impacting this as it's the reason of this nice line of code: framework !== 'angular' && '@storybook/addon-essentials',

I'm hoping removing it before being able to merge this.
@shilman any clue on the chromatic diff ?

Before cleaning up here's what's left:

  • Using SFC for Vue
  • Test react
  • Test react-native
  • Test vue
  • Test angular
  • Test mithril
  • Test riot
  • Test ember
  • Test marionette
  • Test marko
  • Test meteor
  • Test preact
  • Test svelte
  • Test rax
  • Test aurelia
  • Test html
  • Test web-components

I'm confident to quickly have a PR with a single Welcome.mdx to cleanup template-mdx so I won't do much tests, but they should work anyway

Copy link
Member

@gaetanmaisse gaetanmaisse left a comment

Choose a reason for hiding this comment

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

Now let's take a look at this pb with Yarn 2 😄 🙃

HTMLAttributes,
} from 'react';

type MainProps = Omit<DetailedHTMLProps<HTMLAttributes<HTMLElement>, HTMLElement>, 'style'>;
Copy link
Member

Choose a reason for hiding this comment

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

Agree with @shilman a simpler type would be great but I'm a React newbie so I don't if we can do better or if there is a workaround.

lib/cli/src/helpers.ts Outdated Show resolved Hide resolved
lib/cli/src/generators/generator.ts Outdated Show resolved Hide resolved
addons: [
'@storybook/addon-actions',
'@storybook/addon-links',
'@storybook/addon-docs',
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't it be @storybook/addon-essentials instead?

Copy link
Member

Choose a reason for hiding this comment

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

See 2570a66

@gaetanmaisse
Copy link
Member

gaetanmaisse commented Jun 9, 2020

Yarn 2 issue seems related to this line: https://github.com/storybookjs/storybook/blob/next/lib/core/src/server/presets.js#L124

Edit: Or not 🙃

I added 2 commits, I will check the new errors and continue to 🔍 asap

Some addons have peer deps that must be added to make everything works, like '@storybook/addon-docs' => 'react-is'
Comment on lines +54 to +58
const addonsPeerDeps = addons.some(
(addon) => addon === '@storybook/addon-essentials' || addon === '@storybook/addon-docs'
)
? ['react-is']
: [];
Copy link
Member

Choose a reason for hiding this comment

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

@shilman any reason to have react-is as a peerDep and not a regular dep? I'm guessing because having multiples different versions in a single project leads to chaos💥?

@tooppaaa
Copy link
Contributor Author

Closing in favor of #11136

@tooppaaa tooppaaa closed this Jun 12, 2020
@stof stof deleted the tech/colocateReactStories branch May 25, 2022 09:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli maintenance User-facing maintenance tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants