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

feat: Added storybook with button component #58

Merged
merged 17 commits into from Jul 13, 2022
Merged

Conversation

corn-potage
Copy link
Member

@corn-potage corn-potage commented Jul 11, 2022

This PR adds storybook to the project (along with 2 other default story files included with initialization as reference for now), and a button component based on the current draft specs. Currently only includes 2 icons. Wasn't exactly sure what kind of templates we wanted for stories, but made them based on sizes for now.

@jgresham Do you happen to know what's going on with the typescript errors with the Github Actions? I'm still new with TypeScript, and I looked it up, but it's having issues with the Storybook generated files.. I thought about ignoring these files but that doesn't seem like a good idea either 🤔

resolves #54

@jgresham jgresham added the reviewing a reviewer is reviewing the pr label Jul 11, 2022
@jgresham
Copy link
Member

Very happy to see this! Reviewing...
To see typescript errors, run npm exec tsc --verbose. It should be a step in the git-pre commit steps. I'll try to add this as part of the npm run lint command.

@corn-potage
Copy link
Member Author

corn-potage commented Jul 11, 2022

I'll try to add this as part of the npm run lint command.

Ahh, I put the folder in .eslintignore temporarily to get the commits in, but I'll remove it from the file and see what the behavior is like after your commit. Should be ready to merge after that. Thanks for the help!

@jgresham
Copy link
Member

jgresham commented Jul 11, 2022

I'll try to add this as part of the npm run lint command.

Ahh, I put the folder in .eslintignore temporarily to get the commits in, but I'll remove it from the file and see what the behavior is like after your commit. Should be ready to merge after that. Thanks for the help!

I'll try to add this as part of the npm run lint command.

Ahh, I put the folder in .eslintignore temporarily to get the commits in, but I'll remove it from the file and see what the behavior is like after your commit. Should be ready to merge after that. Thanks for the help!

I'm okay with not linting the stories directory as storybook is mostly for viewing components and visual testing. If linting doesn't cause extra work, I'm ok with enabling it.

EDIT: Also, with VSCode, linting and formatting on file save are very helpful features. This files should be detected by vscode and recommend installations https://github.com/jgresham/nice-node/tree/main/.vscode
If this doesn't work with your editor, lmk.

@jgresham
Copy link
Member

Two suggestions (If you could fix first for this PR that'd be ideal):

  1. The components shouldn't have references to storybook. The components should work in both the app code and in storybook without any changes to the component. See ExternalLink story as an example. For now we can put redesigned components in src/renderer/Generics/redesign/...
  2. If there is app level css(scss/sass maybe) code for the redesign (ex. the purple color we will use everywhere), we can put that in a file and import it for all storybook components (ex. here https://github.com/jgresham/nice-node/pull/58/files#diff-98b614e1838b171ee71c04450a8f1a562753193fb7d4f53a4de8b9b5a7e980eeR1)

@jgresham
Copy link
Member

Great work!! I know config for webpack can be troublesome and hard to debug, so thanks for getting that in here. It's great to see the redesigned button in here too!

The latest commit here is deployed at https://nndesign.netlify.app/

@jgresham jgresham merged commit 805012d into main Jul 13, 2022
@jgresham jgresham deleted the feature/storybook branch July 13, 2022 13:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
reviewing a reviewer is reviewing the pr
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Setup Storybook
2 participants