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

Installed Storybook with Vite and Lit support. #18

Merged
merged 5 commits into from
Mar 9, 2023
Merged

Conversation

garrettmflynn
Copy link
Member

@garrettmflynn garrettmflynn commented Mar 8, 2023

This PR installs a new version of Storybook with Vite and Lit support.

A Caveat

The Sidebar component is visible in Storybook—but it looks nothing like the application itself because of all the global styles that are required. This can be cleaned up in future PRs.

… the last commit

Note that the Sidebar component is visible in Storybook, but it looks nothing like the application itself because of all the global styles that are required
@garrettmflynn garrettmflynn self-assigned this Mar 8, 2023
@CodyCBakerPhD
Copy link
Collaborator

The Sidebar component is visible in Storybook

What does this mean? Can you explain how to use it?

@garrettmflynn
Copy link
Member Author

Yeah, my bad for not including an explanation of the script. Run the following command:

npm run storybook

If this doesn't work, update the dependencies using npm ci (or install) and try again.

You should then be able to open the link displayed in the console (ctrl click) and view the Storybook UI in the console. This will have some generic components created on initialization, as well as the Sidebar component that looks terrible because of a lack of global styling.

I'm not sure how to get the latest changes from the lit branch to storybook, but I could also add the new nwb-multiselect-form to the Storybook and this'd give you a better idea for how this should generally behave.

@CodyCBakerPhD
Copy link
Collaborator

npm run storybook

Ohh wow that is cool!

Just some conflicts to resolve and this should be good

Base automatically changed from lit to main March 9, 2023 17:56
@garrettmflynn
Copy link
Member Author

@CodyCBakerPhD Not used to the Github Pull Request tab so didn't see those conflicts. Should be good to go!

@CodyCBakerPhD
Copy link
Collaborator

Screen Shot 2023-03-09 at 2 34 09 PM

OK so this is why I'm so against having the package-lock.json not be in the .gitignore, there is just no way that +/- on this scale is OK by git standards. Do other projects really follow this practice? (Also, I thought npm ci should have run without triggering changes to that file, as per our previous discussion)

Are there any actual changes to the file that are important to us, or required by the PR?

@garrettmflynn
Copy link
Member Author

Yes these changes are important. The CI won't pass unless the '@storybook /xxx' dependencies are included in the package-lock.json file, which will not happen unless we either (1) keep the file outside of the .gitignore file or (2) manually include it when we need to—both of which will result in the same result you see here.

The recommended way to install Storybook (npx storybook init) must not have updated that file, so it didn't register in the first commit and why I've just added it in a separate commit by running npm install (as directed by the CI tooling).

Ultimately, there will often be changes to package-lock.json when we update our dependencies. If you look at production repos like React, you'll see they don't block the yarn.lock file that tracks their dependencies.

Does that make sense?

@CodyCBakerPhD
Copy link
Collaborator

I guess just shock factor at the scale of it...

In Python we only make deliberate, small, clear changes to dependency stack when absolutely necessary and it never produces inflated changelogs

Some example PRs from what you pointed to seem to just go with it: facebook/react#26326, facebook/react#26205, facebook/react#24916

Sure hope it stabilizes more over time

@garrettmflynn
Copy link
Member Author

Yeah, I understand where you're coming from.

The impact on the changelog really depends on the number of dependencies that the dependencies you're adding have. If you refer back to #17, this also updates the package-lock.json as a result of adding two dependencies—but you'll notice that the log notes ~17x fewer changes.

Storybook is a monstrous dependency, hence all the changes here.

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

Successfully merging this pull request may close these issues.

2 participants