-
Notifications
You must be signed in to change notification settings - Fork 47
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
types of csf properties #3
Conversation
/** | ||
* userName: { | ||
* type: csf.FieldTypes.TEXT, | ||
* label: '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.
I'd prefer the term 'title' OR '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.
name is already used to refer to the property name, but title is available and I can rename it?
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.
Just to keep in mind, in knobs-1 its label
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.
yes, I know, we are not required to make this match that api.
The word "label" stems from the fact we're showing a <label />
but that's an implementation detail. I'd rather make an attempt to remove confusing language, and name things the same if at all possible
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.
Maybe its me, but title is confusing for me :) Lets get more feedback, but on my end I would rather not change 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 don't much like "label". I'd much rather this be "name" (which represents a human readable string of any characters) and have some other property for what is currently name. Maybe it's:
["name", "label"] -> ["id", "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.
name is the name of the variable that is passed to the story
for example
export const textStory = ({ text }) => text;
textStory.story = {
properties: {
text: { type: PropertyTypes.TEXT, label: 'Text' }
}
}
here name is the text
variable name. If we rename the label to name:
text: { type: PropertyTypes.TEXT, name: 'Text}
now would the user expect to receive { Text } as the prop to the story
Anyway for me - we say variable 'name', not variable 'id' and I will keep confusing what to refer to 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.
I can live with "name" and "label" I guess. The asymmetry with CSFStory
feels really confusing to me.
wouldn't it to be better like this:
export const textStory = ({ text }) => text;
textStory.story = {
label: 'Text Story`,
properties: {
text: { type: PropertyTypes.TEXT, label: 'Text' }
}
}
then we'd say "The story with name 'textStory'"
(on a similar note that we don't have a better name that 'kind' for collections of stories feels super weird)
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.
@shilman - last call - name or label?
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 it was an intentional choice that the "kind" (or "component") has a title but the story has a name -- it makes it simpler to talk about these things and not have to qualify them all the time.
With that in mind it's kind of nice that there's another term for the name of a property.
* optional collection of properties, which values | ||
* will be passed onto the story function | ||
*/ | ||
properties?: StoryProperties; |
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'm not sure about the naming here "properties" -- other options that have been suggested include: "args", "values", "controls". Properties is quite long and perhaps easily confused with React "props" which they are very similar to but not the same.
Also, have we decided this is a top-level field on the story metadata already?
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 can actually go for props - since we already named the enum PropertyTypes so i think we should go consistent on this - properties or props
For the top level, i thought we had already decided :)
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 like the word controls
a lot too. I was also pondering data
or dataTypes
.
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.
controls are rather called the property editors in the UI. The definitions here i cant see being called controls.
src/properties.ts
Outdated
/** | ||
* value/label pairs or array of OptionsValueType | ||
*/ | ||
export type OptionsListType = { [key: string]: string } | OptionsValueType[]; |
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.
Am I right here that
{ label: string; value: any }[]
and
{ [key: string]: string }
might be a bit confusing. Is it possible to make it generic so people can use them with the type they want?
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 mean it's might be useful to define options in kind of this way:
export type OptionsValueType<T> =
| T
| string
| number
| string[]
| number[]
| { label: string; value: T };
export type OptionsListType<T> = { [key: string]: T } | OptionsValueType<T>[];
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.
Thanks for the feedback, yes its all possible and best to get it accepted and then make other PRs with revisions. The whole csf lib is a early version like 0.01 and a better project management practice would have been to get this started by merging the PR and then make separate issues/PRs for all suggestions and fixes.
As it stands I am pretty much over it, and will probably cancel the PR :(
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 that's valuable to have @BeInLife! I added it.
All this looks good great to me!
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.
Approving to expedite the process. For future reference this probably should be two PRs: one with the properties types, and one for typing the existing CSF.
I'll release this as an alpha and we can iterate it as needed over the course of the 6.0 milestone.
@shilman I added this project to CircleCI, might want to make sure the tests have run? |
Thanks a lot for expediting this guys and norbert for adding the great suggestion. We just have one issue, that i was making changes to keep working on the addon-controls - including renaming one last time properties to controls and i think adding min/max rows for textarea. My latest are here: https://github.com/atanasster/storybook/tree/properties-2/lib/common/src - file story-controls What should we do? Send a new PR? I have few more files also there - merging changes of values, reseting values and an some jest tests for those. |
@atanasster Sure, feel free to submit another PR. Or if you think there will be a lot more changes, you can just At any rate, I think it's good to get this batch of changes out for people to start testing--it's easier to give feedback when it's straightforward to try the types out in your own projects. |
Thanks that is perfect. The utility functions - i am not sure they belong in this csf repo, maybe keep a @storybook/common package for utilities used by both preview and manager There will be changes/hopefully as with all specs/ but for me its easiest to use/test the real thing or i get lost which npm linked package contains what. I believe that the changes upcoming should be minimal as addon-controls is getting off the ground. While the csf project is still early - can i maybe get rights to publish it so i dont get jam logged if there are still changes from addon-controls? |
The utility functions define how storybook handles CSF so that other tools that want to implement CSF in the same way can use them. |
Ok i am holding off any other PRs |
🚀 PR was released in |
csf properties typescript definitions
added also a CSFStory type - added a CSF prefix so its not confused with sb Story,