Open eschaefer opened 8 years ago
nikgraf
commented
8 years ago Oh, seems like React-docgen fails in case you export multiple components. Can you post an example file e.g. BrowseHero.jsx here so we have a case to test for to fix the bug.
eschaefer
commented
8 years ago No problem. Plz don't judge the messy code of this component. It will be refactored :)
To be fair, eslint is telling me that I should only declare/export one component per file...
Seems like there would be other projects that do this multiple exporting though.
import classNames from 'classnames';
import './BrowseHero.less';
import Heading from './Heading';
import BackButton from './BackButton';
import PrimaryButton from './PrimaryButton';
import SecondaryButton from './SecondaryButton';
import GhostButton from './GhostButton';
import ShareDialog from './ShareDialog';
import Image from './Image';
export default class BrowseHero extends React.Component {
static get defaultProps() {
return {
buttonText: 'Watch now',
isBuffered: false,
isLoading: false,
};
}
static get propTypes() {
return {
category: React.PropTypes.string,
isLoading: React.PropTypes.bool,
isBuffered: React.PropTypes.bool,
alignRight: React.PropTypes.bool,
shareButton: React.PropTypes.bool,
containerSize: React.PropTypes.string,
backButton: React.PropTypes.string,
title: React.PropTypes.string,
image: React.PropTypes.string,
children: React.PropTypes.array,
};
}
constructor(...args) {
super(...args);
this.state = {
isShareVisible: false,
};
}
render() {
const className = classNames('browse-hero', {
'browse-hero--buffered': this.props.isBuffered,
'browse-hero--loading': this.props.isLoading,
'browse-hero--align-right': this.props.alignRight,
});
return (
<div className={className}>
<div className="browse-hero__background">
<Image url={this.props.image} forceLoading={this.props.isLoading} />
</div>
{!this.props.isLoading ? [
<div key="overlay" className="browse-hero__overlay" />,
<div key="content" className="browse-hero__container">
<div className="browse-hero__content">
{React.Children.map(this.props.children, (component, index) => {
if (!component) {
return;
}
if (component.type === BackButton) {
return (
<div key={index} className="browse-hero__back">
{component}
</div>
);
}
if (component.type === Heading) {
return (
<div key={index} className="browse-hero__title">
{component}
</div>
);
}
if (component.type === Description) {
return component;
}
return null;
})}
<div className="browse-hero__actions">
{React.Children.map(this.props.children, (child, index) => {
if (child) {
if (child.type === PrimaryButton
|| child.type === SecondaryButton
|| child.type === GhostButton) {
return (
<div key={`actions-item${index}`} className="browse-hero__actions-item">
{child}
</div>
);
}
}
return null;
})}
{this.props.shareButton ? (
<div className="browse-hero__actions-item">
<ShareDialog
title={this.props.title}
visible={this.state.isShareVisible}
onCloseClick={() => {
this.setState({
isShareVisible: false,
});
}}
/>
<GhostButton
text="Share"
icon="share-alt"
onClick={() => {
this.setState({
isShareVisible: true,
});
}}
/>
</div>
) : null}
</div>
</div>
</div>,
] : null}
</div>
);
}
}
export class Description extends React.Component {
render() {
return (
<div className="browse-hero__description">
{this.props.children}
</div>
);
}
}
oori
commented
8 years ago I experienced the same error, with simpler components following this design:
export class Header extends React.Component {
render() {
// ...
}
}
Header.propTypes = { /* ... */ }
export default HeaderThe solution was to export only once per file.
nikgraf
commented
8 years ago @eschaefer don't worry about your code, actually it looks pretty good 😄
Alright, yes the issue lies within the tool we use to identify the types (react-docgen). We could write a resolver that can parse multiple components and then change the architecture in carteblanche.
I remember making a conscious decision about this when we built it. The conclusion was: this would mean increased complexity to support a feature which is not best practise.
I'm happy to merge a PR to add it, but to be honest I rather I recommend to go with one component per file. I just makes your codebase so much more sane 😉 /cc @oori
cef62
commented
7 years ago I'm not sure if this is still an issue with carte-blanche but I'm sharing the info in case someone is interested. react-styleguidist has a similar problem styleguidist/react-styleguidist#185, resolved updating react-docgen resolver options.
I suppose it's possible to do something similar also with carte-blanche.
wmertens
commented
7 years ago @cef62 thanks, so basically use require('react-docgen').resolver.findAllComponentDefinitions instead of just the resolver @nikgraf.
Not sure how to approach this one...
components declared in webpack config: