-
-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
Addon-knobs: improve types via generics and readonlyarray #7411
Merged
shilman
merged 3 commits into
storybookjs:next
from
emilio-martinez:fix/addon-knobs-generics-and-readonlyarray
Jul 18, 2019
Merged
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,204 @@ | ||
import { | ||
number, | ||
color, | ||
files, | ||
object, | ||
boolean, | ||
text, | ||
select, | ||
date, | ||
array, | ||
button, | ||
knob, | ||
radios, | ||
optionsKnob as options, | ||
} from '../index'; | ||
|
||
// Note: this is a helper to batch test return types and avoid "declared but never read" errors | ||
function expectKnobOfType<T>(..._: T[]) {} | ||
|
||
const groupId = 'GROUP-ID1'; | ||
|
||
/** Text knob */ | ||
|
||
expectKnobOfType<string>( | ||
text('text simple', 'Batman'), | ||
text('text with group', 'default', groupId) | ||
); | ||
|
||
/** Date knob */ | ||
|
||
expectKnobOfType<number>( | ||
date('date simple', new Date('January 20 1887')), | ||
date('date with group', new Date(), groupId) | ||
); | ||
|
||
/** Boolean knob */ | ||
|
||
expectKnobOfType<boolean>( | ||
boolean('boolean simple', false), | ||
boolean('boolean with group', true, groupId) | ||
); | ||
|
||
/** Color knob */ | ||
|
||
expectKnobOfType<string>( | ||
color('color simple', 'black'), | ||
color('color with group', '#ffffff', groupId) | ||
); | ||
|
||
/** Number knob */ | ||
|
||
expectKnobOfType<number>( | ||
number('number basic', 42), | ||
number('number with options', 72, { range: true, min: 60, max: 90, step: 1 }), | ||
number('number with group', 1, {}, groupId) | ||
); | ||
|
||
/** Radios knob */ | ||
|
||
expectKnobOfType( | ||
radios<string>( | ||
'radio with string values', | ||
{ | ||
1100: '1100', | ||
2200: '2200', | ||
3300: '3300', | ||
}, | ||
'2200' | ||
), | ||
radios<number>('radio with number values', { 3: 3, 7: 7, 23: 23 }, 3), | ||
radios( | ||
'radio with mixed value', | ||
{ | ||
1100: '1100', | ||
2200: 2200, | ||
3300: '3300', | ||
}, | ||
null, | ||
groupId | ||
) | ||
); | ||
|
||
/** Select knob */ | ||
|
||
enum SomeEnum { | ||
Type1 = 1, | ||
Type2, | ||
} | ||
enum ButtonVariant { | ||
primary = 'primary', | ||
secondary = 'secondary', | ||
} | ||
const stringLiteralArray: ('Apple' | 'Banana' | 'Grapes')[] = ['Apple', 'Banana', 'Grapes']; | ||
|
||
expectKnobOfType<string>( | ||
select( | ||
'select with string options', | ||
{ | ||
None: 'none', | ||
Underline: 'underline', | ||
'Line-through': 'line-through', | ||
}, | ||
'none' | ||
), | ||
select('select with string array', ['yes', 'no'], 'yes'), | ||
select('select with string literal array', stringLiteralArray, stringLiteralArray[0]), | ||
select('select with readonly array', ['red', 'blue'] as const, 'red'), | ||
select<ButtonVariant>('select with string enum options', ButtonVariant, ButtonVariant.primary), | ||
select('select with null option', { a: 'Option', b: null }, null, groupId) | ||
); | ||
|
||
expectKnobOfType<number>( | ||
select('select with number options', { 'type a': 1, 'type b': 2 }, 1), | ||
select<SomeEnum>( | ||
'select with numeric enum options', | ||
{ 'type a': SomeEnum.Type1, 'type b': SomeEnum.Type2 }, | ||
SomeEnum.Type2 | ||
), | ||
select('select with number array', [1, 2, 3, 4], 1), | ||
select('select with readonly number array', [1, 2] as const, 1) | ||
); | ||
|
||
/** Object knob */ | ||
|
||
expectKnobOfType( | ||
object('object simple', { | ||
fontFamily: 'Arial', | ||
padding: 20, | ||
}), | ||
object('object with group', {}, groupId) | ||
); | ||
|
||
/** Options knob */ | ||
|
||
type Tool = 'hammer' | 'saw' | 'drill'; | ||
const visibleToolOptions: Record<string, Tool> = { | ||
hammer: 'hammer', | ||
saw: 'saw', | ||
drill: 'drill', | ||
}; | ||
|
||
expectKnobOfType( | ||
options<Tool>('options with single selection', visibleToolOptions, 'hammer', { | ||
display: 'check', | ||
}), | ||
options<Tool>('options with multi selection', visibleToolOptions, ['hammer', 'saw'], { | ||
display: 'inline-check', | ||
}), | ||
options<Tool>('options with readonly multi selection', visibleToolOptions, ['hammer'] as const, { | ||
display: 'radio', | ||
}), | ||
options('options with group', {}, '', { display: 'check' }) | ||
); | ||
|
||
/** Array knob */ | ||
|
||
const arrayReadonly = array('array as readonly', ['hi', 'there'] as const); | ||
|
||
expectKnobOfType<string[]>( | ||
array('array simple', ['red', 'green', 'blue']), | ||
arrayReadonly, | ||
array('array with group', [], ',', groupId) | ||
); | ||
|
||
// Should return a mutable array despite the readonly input | ||
arrayReadonly.push('Make sure that the output is still mutable although the input need not be!'); | ||
|
||
/** Button knob */ | ||
|
||
expectKnobOfType( | ||
button('button simple', () => {}), | ||
button('button with group', () => undefined, groupId) | ||
); | ||
|
||
/** Files knob */ | ||
|
||
expectKnobOfType<string[]>( | ||
files('files simple', 'image/*', []), | ||
files('files with group', 'image/*', ['img.jpg'], groupId) | ||
); | ||
|
||
/** Generic knob */ | ||
|
||
expectKnobOfType<string>( | ||
knob('generic knob as text', { type: 'text', value: 'a' }), | ||
knob('generic knob as color', { type: 'color', value: 'black' }), | ||
knob<'select', string>('generic knob as string select', { | ||
type: 'select', | ||
value: 'yes', | ||
options: ['yes', 'no'], | ||
selectV2: true, | ||
}) | ||
); | ||
|
||
expectKnobOfType<number>( | ||
knob('generic knob as number', { type: 'number', value: 42 }), | ||
knob('generic knob as select', { type: 'radios', value: 3, options: { 3: 3, 7: 7, 23: 23 } }), | ||
knob('generic knob as number select', { | ||
type: 'select', | ||
value: 1, | ||
options: [1, 2], | ||
selectV2: true, | ||
}) | ||
); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
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.
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.
@emilio-martinez Is this breaking type change intentional? Using
number
s will cause a typescript error.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.
To quote the README for this add-on:
Unfortunately, the package from DefinitelyTyped is inaccurate—that field uses a text-based input of comma delimited values, so even if you were to pass a number array initially, it would subsequently return a string array on change, i.e., here's no number parsing logic for that control.
That said, I'd love to hear more about your use case and help in any way I can. Would you mind sharing?
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.
Hey @emilio-martinez Thanks for the reply. Yeah it seems like you guys are just having to bite the bullet on these type changes going from javascript to typescript, which I totally feel your pain.
This is my use case 👇. It's fixable by just parsing the values and there is probably a better way to go about it. Just got this error when upgrading from
v5.1.9
tov5.2.0-beta.19
.https://github.com/elastic/elastic-charts/blob/8ebe600df1ae476e25032ab1c7707de398143e43/stories/annotations.tsx#L41
I'm curious if you guys are gradually upgrading you codebase to ts or doing it in a single release (I'm too lazy to check for myself)? If it is in a single release why wouldn’t you just release ts in
v6
rather than producing all these stricter type errors on a minor release?Thanks 👍
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.
TS migration is an epic started at the end of 2018, we recently reached an important step as 50% of Storybook codebase in TS 🎉.
Migration progress is followed in #5030 😉. Avoiding breaking changes is one of our goals but we somethings found weird/wrong types/docs even in SB own codebase.
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.
@gaetanmaisse Sweet congrats on the big 5-0!! 🎉 All the best keep up the great work.
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.
Actually, there are many work around and we don't buy in strict type and
noImplicitAny
at day one (I think is for easier migration). For that reason, there are sheer amount ofany
flood in our typing. That somehow explains why we inevitably will have breaking changes in type system, we need to improve it gradually due to the fact that the codebase is pretty huge. But I think this brings more organization and community collaboration to make it awesome in long term.