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

HTML: Add html-vite framework #19698

Merged
merged 14 commits into from
Dec 5, 2022

Conversation

GregLahaye
Copy link
Contributor

@GregLahaye GregLahaye commented Nov 1, 2022

Issue: N/A

What I did

Added html-vite framework, along with repro templates.
I've mostly based this framework off the web-components-vite framework, happy to implement feedback.

How to test

  • Is this testable with Jest or Chromatic screenshots?
  • Does this need a new example in the kitchen sink apps?
  • Does this need an update to the documentation?

Had trouble creating a sandbox locally as it attempts to pull @storybook/html-vite from the Yarn registry instead of locally when linking, but the repro works fine.

@GregLahaye GregLahaye marked this pull request as ready for review November 1, 2022 10:39
@yannbf yannbf requested a review from IanVS November 1, 2022 14:20
Copy link
Member

@IanVS IanVS left a comment

Choose a reason for hiding this comment

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

This seems great to me. I'm not sure what's up with CircleCI though...

code/lib/cli/src/repro-templates.ts Outdated Show resolved Hide resolved
@IanVS
Copy link
Member

IanVS commented Nov 3, 2022

It seems like there's a problem happening in CI:

Unable to find the Storybook folder in "/tmp/storybook/sandbox/html-vite-default-ts/.storybook". Are you sure it exists?

I'm not sure actually the best way to debug this, until the sandbox is merged and published. @shilman what's the best order of operations when adding a new framework? Does the repro need to be added first in a separate PR?

@JReinhold
Copy link
Contributor

Usually there's a manual GitHub workflow maintainers can trigger pointing to a certain branch, that will generate repros from that branch, which should result in it being available in CI. However it only works for branches on the repo, not forks.
I can manually add it, but it will however get overwritten by the next scheduled repro generation from next, so there's a limited time window unfortunately.

cc @yannbf @kasperpeulen this PR might affect/be affected by your current CI work.

@kasperpeulen
Copy link
Contributor

@JReinhold Correct, I added a comment about that.

@ndelangen
Copy link
Member

I was hoping to bring this up to date with next branch, but got this error when trying to push:

> git push GregLahaye add-html-vite-framework:add-html-vite-framework
To https://github.com/GregLahaye/storybook.git
 ! [remote rejected]       add-html-vite-framework -> add-html-vite-framework (refusing to allow an OAuth App to create or update workflow `.github/workflows/trigger-circle-ci-workflow.yml` without `workflow` scope)
error: failed to push some refs to 'https://github.com/GregLahaye/storybook.git'

@shilman shilman added the linear label Dec 1, 2022
@yannbf
Copy link
Member

yannbf commented Dec 2, 2022

Branch is up to date now, with the necessary code changes to get things working!

@shilman shilman changed the title Add html-vite framework HTML: Add html-vite framework Dec 2, 2022
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.

Looking great! One potential change

@@ -0,0 +1,8 @@
// exports for builder-vite
export { createChannel as createPostMessageChannel } from '@storybook/channel-postmessage';
Copy link
Member

Choose a reason for hiding this comment

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

Looks like we don't need any of these exports anymore except the StorybookConfig type. @IanVS any other changes we need to make to get this working?

Copy link
Member

Choose a reason for hiding this comment

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

Nope, you're right, we can remove the extra re-exports now. 👍

"@storybook/node-logger": "7.0.0-alpha.56",
"@storybook/preview-web": "7.0.0-alpha.56",
"magic-string": "^0.26.1",
"vite": "3"
Copy link
Member

Choose a reason for hiding this comment

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

We'll need to open this up to allow version 4, but we can do that in another PR, for all the vite frameworks.

@GregLahaye
Copy link
Contributor Author

@shilman @IanVS I've removed the re-exports and bumped the version numbers.

@shilman shilman merged commit d21d476 into storybookjs:next Dec 5, 2022
@shilman
Copy link
Member

shilman commented Dec 5, 2022

Thanks @GregLahaye !!! This is a great contribution 🙌

@jonniebigodes
Copy link
Contributor

@GregLahaye, thanks for this amazing contribution, one small item related to this pull request. If you could jump into our Discord Server and message me directly (same username)?

Looking forward to hearing from you.

Hope you have a great week.

Stay safe

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants