-
Notifications
You must be signed in to change notification settings - Fork 6
Provide stories with argTypes for primitives #3
Conversation
bin/generateArgTypes.ts
Outdated
@@ -0,0 +1,30 @@ | |||
#!/usr/bin/env node --experimental-loader esbuild-node-loader |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: convention for file names is dash separated lower case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to self: need a linter for this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason why BTW? I prefer CamelCased (like in code) especially to match component names.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I used to like camel cased too because it matches component names, but I am not using default export any more most of the time and components export more than one thing, so there is no matching, also I don't encourage putting a component per module.
Additionally it avoids case-sensitivity file system/git issues you are probably aware of
bin/generateArgTypes.ts
Outdated
@@ -0,0 +1,30 @@ | |||
#!/usr/bin/env node --experimental-loader esbuild-node-loader | |||
|
|||
import path from "path"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to self: need to add prettier config
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
npx mrm prettier && npx mrm lint-staged
;)
bin/generateArgTypes.ts
Outdated
const tsConfigParser = docgen.withCustomConfig(tsConfigPath, options); | ||
|
||
// Search for components | ||
const componentFiles = fg.sync(['./src/components/*.tsx', '!./src/**/*.stories.tsx']); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably should be an arg to the script as a single pattern
bin/generateArgTypes.ts
Outdated
const jsonPath = filePath.replace('.tsx', '.props.json') | ||
const res = tsConfigParser.parse(filePath) | ||
const argTypes = propsToArgTypes(res[0].props) | ||
fs.ensureFileSync(jsonPath) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you don't need this line because its fs-extras, makes sure path exists, correct me if I am wrong
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it doesn't.
bin/generateArgTypes.ts
Outdated
const res = tsConfigParser.parse(filePath) | ||
const argTypes = propsToArgTypes(res[0].props) | ||
fs.ensureFileSync(jsonPath) | ||
fs.writeJsonSync(jsonPath, argTypes) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can pass options to format it with 2 spaces and make it human readable and also easy to see future changes in a diff
https://github.com/jprichardson/node-fs-extra/blob/HEAD/docs/writeJson-sync.md
src/arg-types/index.ts
Outdated
@@ -0,0 +1,71 @@ | |||
export const globalArgTypes = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can be removed?
src/arg-types/loader.js
Outdated
@@ -0,0 +1,24 @@ | |||
const path = require('path') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think loader isn't used any more, right?
src/arg-types/utils.ts
Outdated
|
||
const validAttributes = (prop: PropItem) => { | ||
if (prop.parent) { | ||
return ['ButtonHTMLAttributes', 'HTMLAttributes', 'AriaAttributes'].includes(prop.parent.name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be extended for all elements?
}, {} as ArgTypes); | ||
} | ||
|
||
const matchers = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needs to be extended to support all controls?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's how it's done in Storybook now. I would not do it here at all TBH.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, does this compromise our ability to render controls in both correctly? I guess if we do a better job at rendering than storybook its not a problem, unless we become incompatible
} | ||
|
||
// args that end with date e.g. purchaseDate | ||
if (matchers.date && matchers.date.test(name)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this might be too smart, because a field with purchaseDate could actually expect a non-date string or some kind of specific format and we would be probably enforcing an ISO date or something ... unless the type in typescript is an actual date, we probably shouldn't guess like this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree it's too "smart" and it's presentational logic that IMO should live in the designer, not in the data set. I would not even add control
property to the set and only export pure parsed prop types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, but then we would loose the interop with storybook
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really. We could do enhance the data with controls
in the designer or use already provided control
property from CSF.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I imagine the future where a user can provide their components with stories and those could be potentially manually written, right?
So in this case we say: we always generate argTypes ourselves, no matter what user has defined in the story?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if user doesn't have types ? :)
src/arg-types/utils.ts
Outdated
return {control: {type: 'number'}}; | ||
case 'enum': { | ||
const {value} = type; | ||
// @ts-ignore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs type fix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is any
in the types of react-docgen-typescript
so it would be worth fixing there I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replaced with @ts-expect-error
<BoldPrimitive {...args} /> | ||
); | ||
|
||
export const Bold = Template.bind({}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to self: we can rewrite these to just exporting a component that renders the primitive instead of using .args, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It can be done, but it will disable addon-controls
in the Storybook UI 🤷♂️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right, forgot about it, we need them
src/components/bold.stories.tsx
Outdated
export default { | ||
title: "Components/Bold", | ||
component: BoldPrimitive, | ||
argTypes: {}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
forgotten one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
@@ -0,0 +1,10 @@ | |||
export { default as Button } from "./button.stories"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can now export the argTypes directly and avoid exporting stories which could have some storybook specific stuff we don't need in the designer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case it won't be CSF anymore but yeah, I think you can simple import JSONs now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, in the future designer should be able to discover stories automatically from the file system, without having user provide a meta.ts with all the stories in it, so yeah lets keep it as-is for now and then later add a way for discovering stories automatically
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First round done
@okonet want to finish it or should I? |
I will finish this one. Thanks for comments. |
@kof please re-review. |
merged it |
webstudio-is/webstudio-designer/#24