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
Storybook: Support proper extraction of TypeScript prop types #38842
Storybook: Support proper extraction of TypeScript prop types #38842
Changes from all commits
f60feaf
a6ea38b
9195e71
17cd970
ddc9814
9c46706
64d5f73
e7eca9f
d92ce1b
1f094fa
bb1443c
dbbb7a5
3e53470
37799c4
b51f400
a8dfc1a
0c8c661
f4a448c
cfa4891
3c0a4e2
f95f2cd
ba3ca92
f98d14d
354c730
f75562e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
This file was deleted.
This file was deleted.
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.
adjustLineHeightForInnerControls
can also be aboolean
— should we addtrue
andfalse
to the list of possible values, maybe in a dropdown-like control?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.
Mmm yeah, this one’s a bit overcomplicated too. Based on the code,
true
gives the same result as'medium'
, andfalse
gives the same result asundefined
. Before we graduate<Text>
from experimental status, I'd prefer removing the boolean types from this prop API. (Kind of similar to yourUnitControl
cleanup!)I'll note this in the TS refactor tracking issue.
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 close the circle, this was finally done in #54953 !
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.
These values can be both strings or numbers, is there a way to achieve that with 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.
The
object
control can strictly cover both, since that's basically a JSON string input so it differentiates between1
and"1"
. Though in the vast majority of cases I think a normaltext
control would be sufficient for tinkering and better for UX (because people will have trouble recognizing that they need to quote their strings in aobject
control).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: using a radio with only one option, it's not possible to "deselect" that option, once selected
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.
Kind of like the "reset" problem in the editor sidebar! I actually think this is consistent with all controls (especially auto-generated ones), in that you can't really go back to the
undefined
state once you set a value. We could of course add anundefined
option, but I think the overall design intent is to use the "Reset controls" button ↩️ in the top right.I'm not sure how I feel about this yet! On one hand I don't want to establish a pattern of adding a lot of manual tweaks to auto-generated controls, because it increases maintenance burden. But in some cases it may be annoying to have to hit Reset each time. So... maybe worth it for heavily trafficked props like
variant
onButton
?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.
Agreed. Let's leave it as-is (and rely on the reset button for now). I believe we are amongst the heavy users of this Storybook anyway, so we can make this change if we ever feel like it's really needed.
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 the "limit" for expanding type aliases is if the types that are unioned are all of the same general type or not?
E.g. a union of just strings could be "easier" to expand for the docgen, while a union of strings and boolean may be kept un-expanded?
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 know 🤷
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.
Me neither, but good to keep at the back of our minds for future work :)