-
Notifications
You must be signed in to change notification settings - Fork 87
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
Rewrite in Typescript: Spinner #317
Conversation
cb1d516
to
c26166e
Compare
src/scripts/Spinner.tsx
Outdated
@@ -34,27 +45,14 @@ export default class Spinner extends React.Component { | |||
} | |||
|
|||
render() { | |||
const { container, ...props } = this.props; | |||
const { container = true, size = 'small', ...props } = this.props; |
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.
@stomita
I quit using defaultProps and use destructuring assignment instead.
It works, but I couldn't make use of type check here, it means I can assign any value to both container
and size
.
If you know any good solution, please tell me.
stories/SpinnerStories.tsx
Outdated
@@ -17,19 +18,21 @@ storiesOf('Spinner', module) | |||
.add( | |||
'Controlled with knobs', | |||
() => { | |||
// TODO: After https://github.com/storybookjs/storybook/issues/4487 is solved, | |||
// - Add `undefined` to sizeOptions. | |||
// - Change '' to `undefined` in typeOptions. |
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 problem that I can't add { '(none)': undefined }
to sizeOptions
is because of addon-knobs' issue:
addon-knobs select should be able to have undefined options. · Issue #4487 · storybookjs/storybook
First I fixed the component logic itself to work properly if given empty string (b438ac3).
But I thought we shouldn't change the logic for the story.
Closed because the last PR #342 had been merged. |
ref. #289
TODO: after #316 is merged, change base branch to 3.0.