Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
types of csf properties #3
Changes from 3 commits
e5d7e70
b12e1c1
841af83
65f5322
9f1e14c
2b54c32
e82f0ce
725c958
0bf6408
a740081
2381349
b9ef576
d90a1e9
81170eb
5d35c34
dabd642
e329b85
a65b41a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 ponderingdata
ordataTypes
.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.
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 possibleThere 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
here name is the
text
variable name. If we rename the label to name: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:
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.