Skip to content

Fix proptypes warning for kitchen-sink #1238

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

Fix proptypes warning for kitchen-sink #1238

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
75645e3
Fix proptypes warning
alexandrebodin Jun 9, 2017
0bcda4f
Add test-cra to learn bootstrap to use symlinks
alexandrebodin Jun 9, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion addons/info/src/components/Props.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ Props.defaultProps = {
Props.propTypes = {
node: PropTypes.shape({
props: PropTypes.object,
type: PropTypes.object.isRequired,
type: PropTypes.oneOfType([PropTypes.string, PropTypes.func]).isRequired,
}).isRequired,
singleLine: PropTypes.bool,
maxPropsIntoLine: PropTypes.number.isRequired,
Expand Down
26 changes: 13 additions & 13 deletions examples/test-cra/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,19 +17,19 @@
"react-dom": "^15.5.4"
},
"devDependencies": {
"@storybook/addon-actions": "file:../../addons/actions",
"@storybook/addon-centered": "file:../../addons/centered",
"@storybook/addon-info": "file:../../addons/info",
"@storybook/addon-knobs": "file:../../addons/knobs",
"@storybook/addon-links": "file:../../addons/links",
"@storybook/addon-notes": "file:../../addons/notes",
"@storybook/addon-options": "file:../../addons/options",
"@storybook/addon-storyshots": "file:../../addons/storyshots",
"@storybook/addons": "file:../../lib/addons",
"@storybook/channel-postmessage": "file:../../lib/channel-postmessage",
"@storybook/channels": "file:../../lib/channels",
"@storybook/react": "file:../../app/react",
"@storybook/ui": "file:../../lib/ui",
"@storybook/addon-actions": "^3.0.1",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This removes the purpose of test-cra

I at one point set it up as an alternative if bootstrapping wouldn't work.

Nowadays bootstrapping is relatively fast-ish (compared to what is was) and it's reliable (at least for me & on CI).

I think we can discontinue this example now, and stop using file dependencies. @tmeasday I'd like your thoughts on this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously there was an advantage to test-cra because it flattened dependencies (with npm<5) and thus "looked like" a real app in terms of how require/import resolved.

However, in npm@5, file:// URLs are treated like npm links (AFAICT), so that advantage is gone. It seems like we can just about get away with it with aggressive use of peerDependencies in packages and linking every storybook related package in the app (this is OK).

So I don't think there's any point in distinguishing test-cra from cra-storybook any more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I definitely don't think linking to versions is what we want.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you OK with me just removing it then?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so if you agree with the motivation for it in the first place above.

"@storybook/addon-links": "^3.0.0",
"@storybook/addons": "^3.0.0",
"@storybook/addon-centered": "^3.0.0",
"@storybook/addon-info": "^3.0.0",
"@storybook/addon-knobs": "^3.0.0",
"@storybook/addon-notes": "^3.0.0",
"@storybook/addon-options": "^3.0.0",
"@storybook/addon-storyshots": "^3.0.0",
"@storybook/channels": "^3.0.0",
"@storybook/react": "^3.0.0",
"@storybook/channel-postmessage": "^3.0.0",
"@storybook/ui": "^3.0.1",
"react-scripts": "1.0.2",
"react-test-renderer": "^15.4.2"
},
Expand Down
2 changes: 1 addition & 1 deletion examples/test-cra/src/stories/Button.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ const Button = ({ children, onClick }) =>
</button>;

Button.propTypes = {
children: PropTypes.string.isRequired,
children: PropTypes.oneOfType([PropTypes.string, PropTypes.array]).isRequired,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be PropTypes.node

onClick: PropTypes.func,
};
Button.defaultProps = {
Expand Down
2 changes: 1 addition & 1 deletion examples/test-cra/src/stories/Welcome.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ export default class Welcome extends React.Component {
}

Welcome.propTypes = {
showApp: PropTypes.function,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rookie mistake, whoever wrote this... Ow wait this was me.

showApp: PropTypes.func,
};
Welcome.defaultProps = {
showApp: log,
Expand Down
6 changes: 6 additions & 0 deletions examples/test-cra/src/stories/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,12 @@ storiesOf('Button', module)
})
.addWithInfo('with some info', 'Use the info addon with its painful API.', () =>
<Button>click the "?" in top right for info</Button>
)
.addWithInfo(
'with some info 2',
'Use the info addon with its painful API.',
() => <Button>click the "?" in top right for info</Button>,
{ inline: true, propTables: [Button] }
);

storiesOf('Centered Button', module)
Expand Down
1 change: 0 additions & 1 deletion lerna.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
"commands": {
"bootstrap": {
"ignore": [
"test-cra",
"react-native-vanilla"
]
},
Expand Down
1 change: 0 additions & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
"scripts": {
"bootstrap": "lerna bootstrap",
"bootstrap:docs": "cd docs && npm i",
"bootstrap:test-cra": "lerna exec --scope test-cra -- npm i",
"bootstrap:react-native-vanilla": "lerna exec --scope react-native-vanilla -- npm i",
"changelog": "pr-log --sloppy",
"precommit": "lint-staged",
Expand Down