Open atanasster opened 5 years ago
Great idea. I think there are a few aspects to this:
this thread might be interesting https://stackoverflow.com/questions/5859561/getting-the-closest-string-match .
I can see how this would be necessary for composite components.
Question:
Some strawman UI ideas:
@domyen very good points.
Sort of related: In non-react frameworks there are more things to put in tables than just props. For example, Vue
has slots & events: https://github.com/storybookjs/storybook/issues/6663
Also sort of related: Multiple components / sub-components: https://github.com/storybookjs/storybook/issues/7811
@domyen - do you have some animated expand/collapse (ie https://github.com/Stanko/react-animate-height) in sb.components or sds?
Is it ok to introduce dependency on react-animate-height
?
Hi everyone! Seems like there hasn't been much going on in this issue lately. If there are still questions, comments, or bugs, please feel free to continue the discussion. Unfortunately, we don't have time to get to every issue. We are always open to contributions so please send us a pull request if you would like to help. Inactive issues will be closed after 30 days. Thanks!
Hi everyone! Seems like there hasn't been much going on in this issue lately. If there are still questions, comments, or bugs, please feel free to continue the discussion. Unfortunately, we don't have time to get to every issue. We are always open to contributions so please send us a pull request if you would like to help. Inactive issues will be closed after 30 days. Thanks!
Looks great @domyen and the indentation is very user friendly , just a reminder to possibly default expand the first interface.
@domyen @atanasster I like the design you propose, with:
I think this can use the sections API of PropsTable:
export interface PropsTableSectionsProps {
sections?: Record<string, PropDef[]>;
expanded?: [string];
}
If expanded is provided, it will make the rows expandable/collapsible, with the provided sections expanded by default. The other props tables (Vue, Angular, WC, etc.) won't provide expanded, so their behavior shouldn't change.
Sounds good @shilman
@shilman @domyen - what about adding the number of props to the section header as in the initial screenshot? Just a note that we cant really know which are own props - except try to substring it, or simply use the first interface listed ( i think its more visible in my jnitial screenshot)
what about adding the number of props to the section header as in the initial screenshot?
That makes sense to me! We do that in the animated gif in the column beside the name ("1 remaining unreviewed") – could do the same thing here.
@atanasster At this point it would be hard to fit into 5.3, but would be happy to merge it as soon as the release goes out!
@shilman - I did some further tests:
adding very simple component
import React, { FC } from 'react';
interface ButtonProps {
/** Own ButtonProps label */
label?: string;
}
/** Component description imported from comments inside the component file
* This React component has its own properties but also accepts all the html `button` attributes.
*/
export const TypescriptButton: FC<ButtonProps & JSX.IntrinsicElements['button']> = ({
label,
...rest
}) => (
<button type="button" {...rest}>
{label}
</button>
);
results in *260+ properties (quite disturbing), here is a screenshot:
Also an unfortunate finding is that react-docgen-typescript-loader
does not actually support the parent name/fileName (https://github.com/strothj/react-docgen-typescript-loader/issues/50) , so there is really no way to implement this feature with the typescript presets coming with docs:
So we would have to first change the loader to https://github.com/atanasster/webpack-react-docgen-typescript which does carry the parent property is we can differentiate where the property belongs:
I hope this makes sense but in short without adding the & JSX.IntrinsicElements['button']
, a typescript user wouldn't even be able to specify any of button
attribute ie disabled
etc.
On the other hand, such components will display hundreds of properties in the props table in storybook.
Sorry to jump in like that.
Is there value in rendering the props from native JSX components?
Wouldn’t it be easier to exclude those props?
This is what I was planning to do when I get back from vacation.
@patricklafrance - the more (fedback) the merrier :)
you can still filter properties out, although I am not sure how you would do it with react-docgen-typescript-loader
since it does not distinguish any composed props.
Can you please elaborate what you had in mind already?
@patricklafrance I looked at your use case.
semantic-ui-react
are not exposing the intrinsic properties, although they do have an extensive list of duplicated properties that are inherited.
IMHO that's a (typescript, even though semantic-ui-react
is not typescript and just exposes d.ts files) bug - and semantic-ui-react
is taking a shortcut to avoid typescript errors, but basically ignoring type safety for all intrinsic properties that are not listed.
Here is a button example of what I encountered:
@patricklafrance - the more (fedback) the merrier :) you can still filter properties out, although I am not sure how you would do it with
react-docgen-typescript-loader
since it does not distinguish any composed props.Can you please elaborate what you had in mind already?
What I have in mind is implemented in the storybook config of cra-ts-kitchen-sink
What I have in mind is implemented in the storybook config of cra-ts-kitchen-sink
I am not sure I understand - the cra-ts-kitchen-sink Button component is not really representative:
const Button: FunctionComponent<Props> = ({ children, type = 'default', onClick }) => {
return (
<button type="button" onClick={onClick}>
{type}: {children}
</button>
);
};
You can pass only two props to the Button
- type
and onClick
- other properties like id
or any aria-
etc attributes will generate typescript errors.
That might be a different use case then!
I haven’t use typescript much (and I am on my phone), I might not understand correctly the problem you are solving here.
For the Button component in cra-ts-kitchen-sink, Before the props filter had been configured on the typescript loader about 200 props from the React FC interface were rendered.
@patricklafrance - you mean the code that says Currently not working :)
propFilter: prop => {
// Currently not working, prop.parent is always null.
if (prop.parent) {
return !prop.parent.fileName.includes('node_modules/@types/react/');
}
return true;
IMHO opinion we do need to support combined props - its not just JSX.IntrinsicElements
but the users can compose interfaces in many ways. Most react UI libraries are just starting to really support typescript, so I guess its not that big of an issue yet, bit something that is good to be prepared for.
Lets talk more when you are back from vacation, and if you come around Tampa, give me a shout out :)
Btw the filter above would work with my webpack-react-docgen-typescript
loader, thats why I said filtering is an option. However not with the default react-docgen-typescript-loader
filtering is not currently an option.
@patricklafrance - you mean the code that says Currently not working :)
propFilter: prop => { // Currently not working, prop.parent is always null. if (prop.parent) { return !prop.parent.fileName.includes('node_modules/@types/react/'); } return true;
IMHO opinion we do need to support combined props - its not just
JSX.IntrinsicElements
but the users can compose interfaces in many ways. Most react UI libraries are just starting to really support typescript, so I guess its not that big of an issue yet, bit something that is good to be prepared for.Lets talk more when you are back from vacation, and if you come around Tampa, give me a shout out :)
Haha it end up working after updating the library, forgot to remove the comment :)
Sounds good!
strange, its still an open issue on webpack-react-docgen-typescript
afaics (posted a link above)
Just tested on the latest cra-ts-kitchen-sink, and the cra preset doesn't seem to work to show any props at all :(
I don't see any component in cra-ts-kitchen-sink using JSX.IntrinsicElements that would generate 200 properties - when you are back from vacation would love to get our information merged :)
aaah, I see - the filter by parent on react-docgen-typescript-loader
should work, the issue was just about parent not being brought back.
How would I enable this feature?
We've just released zero-config typescript support in 6.0-beta. Please upgrade and test it!
Thanks for your help and support getting this stable for release!
the typescript prop tables can often times get very long with components that are accepting props from many interfaces. Example:
the prop table in this case is really long: https://atanasster.github.io/grommet-controls/?path=/docs/controls-input-dropinput--main
The example prop table json file: https://github.com/atanasster/grommet-controls/blob/master/src/components/DropInput/doc/DropInput.json
A screenshot how the parent interface name can be collapsed. By default suggestion is to have expanded only the first interface name: