Skip to content
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: : add number config options #2371

Merged
merged 6 commits into from
Nov 26, 2017
Merged

Addon-knobs: : add number config options #2371

merged 6 commits into from
Nov 26, 2017

Conversation

lifeiscontent
Copy link
Member

Fix inconsistency in number API

fix step,min,max attributes in number type

Issue:

What I did

Fixed inconsistency in number type.

How to test

when setting the options of a number, the step,min,max attributes now get applied.

Is this testable with jest or storyshots? Yes, but I didn't write a test.

Does this need a new example in the kitchen sink apps? No.

Does this need an update to the documentation? Not really, but I could add an example.

If your answer is yes to any of these, please make sure to include it in your PR.

Fix inconsistency in number API

fix step,min,max attributes in number type
@codecov
Copy link

codecov bot commented Nov 25, 2017

Codecov Report

Merging #2371 into master will decrease coverage by <.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2371      +/-   ##
==========================================
- Coverage   19.92%   19.92%   -0.01%     
==========================================
  Files         293      293              
  Lines        6484     6485       +1     
  Branches      752      755       +3     
==========================================
  Hits         1292     1292              
- Misses       4597     4607      +10     
+ Partials      595      586       -9
Impacted Files Coverage Δ
addons/knobs/src/components/types/Number.js 7.93% <0%> (-0.13%) ⬇️
addons/knobs/src/components/types/Color.js 8.21% <0%> (ø) ⬆️
...es__/update-addon-info/update-addon-info.output.js 0% <0%> (ø) ⬆️
addons/events/src/components/Event.js 0% <0%> (ø) ⬆️
addons/a11y/src/components/Report/Rules.js 0% <0%> (ø) ⬆️
lib/ui/src/libs/key_events.js 23.25% <0%> (ø) ⬆️
lib/ui/src/modules/shortcuts/actions/shortcuts.js 6.25% <0%> (ø) ⬆️
...rc/modules/ui/components/left_panel/text_filter.js 30.98% <0%> (ø) ⬆️
addons/jest/src/components/Indicator.js 0% <0%> (ø) ⬆️
addons/a11y/src/A11yManager.js 0% <0%> (ø) ⬆️
... and 27 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 553bccd...652f862. Read the comment docs.

@Hypnosphi
Copy link
Member

Hypnosphi commented Nov 25, 2017

That's great, thanks! This fixes #1632

Does this need a new example in the kitchen sink apps

Actually, I think it does. Please add one here: https://github.com/storybooks/storybook/blob/master/examples/cra-kitchen-sink/src/stories/addon-knobs.stories.js

You may need to run yarn jest -u to update storyshots after that

@ndelangen
Copy link
Member

Thank you for this PR @lifeiscontent !

@Hypnosphi Looks like the min,max,step was already in the example?
https://github.com/storybooks/storybook/blob/master/examples/cra-kitchen-sink/src/stories/addon-knobs.stories.js#L52

@Hypnosphi
Copy link
Member

@ndelangen we need the same but without range: true

@lifeiscontent
Copy link
Member Author

@Hypnosphi how do I run the kitchen sink example?

I tried the following.

yarn
yarn bootstrap # and selected core and examples
cd storybook/examples/cra-kitchen-sink
yarn storybook

but I get issues saying none of the storybook modules exist.

@lifeiscontent
Copy link
Member Author

@Hypnosphi updated!

@@ -47,7 +47,7 @@ class AsyncItemLoader extends React.Component {

storiesOf('Addon Knobs.withKnobs', module)
.addDecorator(withKnobs)
.add('tweaks static values', () => {
.add('tweaks static values via range', () => {
Copy link
Member

@Hypnosphi Hypnosphi Nov 26, 2017

Choose a reason for hiding this comment

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

This example includes a lot of other knobs, not only the range input

Copy link
Member

Choose a reason for hiding this comment

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

Let’s add min/max/step to dollars below instead of duplicating the example

Copy link
Member

Choose a reason for hiding this comment

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

Please run yarn jest -u after that

@@ -47,7 +47,7 @@ class AsyncItemLoader extends React.Component {

storiesOf('Addon Knobs.withKnobs', module)
.addDecorator(withKnobs)
.add('tweaks static values', () => {
.add('tweaks static values via range', () => {
Copy link
Member

Choose a reason for hiding this comment

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

Let’s add min/max/step to dollars below instead of duplicating the example

@lifeiscontent
Copy link
Member Author

lifeiscontent commented Nov 26, 2017

@Hypnosphi updated.

I ran jest, but it seems there are tests failing that are unrelated.

@Hypnosphi
Copy link
Member

Ok looks like it passes as is. I'll be to test the example manually in a few hours

@Hypnosphi Hypnosphi merged commit ce69ca8 into storybookjs:master Nov 26, 2017
@shilman shilman changed the title Update Number.js Addon-knobs: : add number config options Dec 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants