Restructure project and add Storybook #84
Conversation
Run the storybook init script as isntructed in the storybook docs at https://storybook.js.org/docs/react/get-started/install
There was a problem hiding this comment.
The dependabot.yml file might be outdated now, I'm not sure if it checks yarn workspaces and updates those correctly, especially with some packages being in multiple package.jsons
GitHub says that ssri=6.0.1 is used which has a vulnerability. There are also mentions of 8.0.1 so I'm not sure which version is actually being used, but should definitely make sure that it's not possible to install <8.0.1.
I haven't looked at the tsconfigs yet but some of the references to types/ might be redundant.
typings/ is also the standard name for a directory like that.
Overall great changes and sets good foundations for the final storybook setup (e.g. msw).
| @@ -2,25 +2,17 @@ | |||
| "private": true, | |||
| "repository": "https://github.com/thunderstore-io/thunderstore-ui", | |||
| "workspaces": [ | |||
| "nextjs", | |||
| "thunderstore-components" | |||
There was a problem hiding this comment.
Maybe something like common-components would be a better name than packages/components
| @@ -2,25 +2,17 @@ | |||
| "private": true, | |||
| "repository": "https://github.com/thunderstore-io/thunderstore-ui", | |||
| "workspaces": [ | |||
| "nextjs", | |||
There was a problem hiding this comment.
Is nextjs/ really a "package"? The name package implies to me that it's going to be published to a registry or is intended to be imported somewhere, for example, some component libraries use packages/ to split up components into sub-packages so you can install a subset, but nextjs/ feels more like a project.
There was a problem hiding this comment.
I don't think it really matters, keeps the configuration simpler to have everything under the same rules of structuring. If anything you could rename the packages to something like projects.
There was a problem hiding this comment.
I think projects sounds better since we're unlikely to have more libraries and the components are mostly for our own projects. I don't have a strong preference between the two though, it's pretty minor
| - yarn workspaces, as defined in the root `package.json` | ||
| - typescript configuration inheritance and path aliases | ||
| - tweaks for module resolution done in other tools such as nextjs or webpack, | ||
| making sure the path aliased packages are appropriately resolved |
There was a problem hiding this comment.
These should probably start with capital letters
| @@ -0,0 +1,24 @@ | |||
| { | |||
| "name": "@thunderstore/storybook", | |||
| "version": "0.1.0", | |||
There was a problem hiding this comment.
Maybe this should be 0.0.0 to imply it's unversioned
| - tweaks for module resolution done in other tools such as nextjs or webpack, | ||
| making sure the path aliased packages are appropriately resolved | ||
|
|
||
| All sub-projects exist in the `packages` directory, includinig the following: |
There was a problem hiding this comment.
| All sub-projects exist in the `packages` directory, includinig the following: | |
| All sub-projects exist in the `packages` directory, including the following: |
| "name": "@thunderstore/storybook", | ||
| "version": "0.1.0", | ||
| "private": true, | ||
| "repository": "https://github.com/thunderstore-io/thunderstore-ui/tree/master/packages/storybook", |
There was a problem hiding this comment.
From the docs:
The URL should be a publicly available (perhaps read-only) url that can be handed directly to a VCS program without any modification. It should not be a url to an html project page that you put in your browser. It's for computers.
A relevant example:
{
"repository": {
"type": "git",
"url": "https://github.com/facebook/react.git",
"directory": "packages/react-dom"
}
}There was a problem hiding this comment.
Iherited the pattern from the existing two projects, but I'll change this and those to just point to the root repo here
| <FormErrorMessage>{errors.communities.message}</FormErrorMessage> | ||
| )} | ||
| {errors.communities && | ||
| errors.communities.map( |
There was a problem hiding this comment.
This gives me TypeError: errors.communities.map is not a function if I press Upload without entering any values (as it is a string, not a list of strings)
There was a problem hiding this comment.
In that case the type definition for errors.communities was wrong and should be looked at. I just made it pass typechecks here.
There was a problem hiding this comment.
Yeah that's why I left it at the time. I think we might need to make an issue on react-hook-form. It's either a list of strings or a string
There was a problem hiding this comment.
PackageUploadFormInputs.communities should be changed to NestedValue<string[]> as "non-primitive types" should be wrapped with NestedValue (the docs aren't very clear). This fixes the type for errors
| "alwaysStrict": true, | ||
|
|
||
| "noUnusedLocals": true, | ||
| "noUnusedParameters": false, |
There was a problem hiding this comment.
noUnusedParameters should be enabled since ts(6133) and @typescript-eslint/no-unused-vars enforce no unused parameters (and there isn't really a reason to allow it anyway)
There was a problem hiding this comment.
sometimes you might have a callback where you for example need the 3rd parameter, and IMO it's nicer to just name them out instead of having _, _, something for example, which is why I figured this could be disabled.
|
Replaced by PR #289 . |
No description provided.