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

Fix addon Knobs: add array in Object PropTypes #2227

Merged
merged 5 commits into from
Nov 4, 2017
Merged

Fix addon Knobs: add array in Object PropTypes #2227

merged 5 commits into from
Nov 4, 2017

Conversation

jebarjonet
Copy link
Contributor

Knobs addon

value prop in object type of knob now accepts PropTypes.array, while only PropTypes.object was accepted.

We now avoid warnings when we use arrays of objects in object knobs, which actually works perfectly fine.

Example working fine but throwing warnings in browser console concerning Proptypes.

const values = object('options', [
  {
    value: 'red',
    title: 'Red',
    condition: '1',
  },
  {
    value: 'blue',
    title: 'Blue',
    condition: '1',
  },
  {
    value: 'yellow',
    title: 'Yellow',
    condition: '1',
  },
  {
    value: 'green',
    title: 'Green',
    condition: '1',
  },
])

screen shot 2017-11-03 at 5 17 09 pm

@codecov
Copy link

codecov bot commented Nov 3, 2017

Codecov Report

Merging #2227 into master will not change coverage.
The diff coverage is 0%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2227   +/-   ##
=======================================
  Coverage   22.16%   22.16%           
=======================================
  Files         268      268           
  Lines        5875     5875           
  Branches      710      701    -9     
=======================================
  Hits         1302     1302           
- Misses       4027     4045   +18     
+ Partials      546      528   -18
Impacted Files Coverage Δ
addons/knobs/src/components/types/Object.js 5.81% <0%> (ø) ⬆️
lib/ui/src/modules/ui/configs/handle_routing.js 28.04% <0%> (ø) ⬆️
lib/ui/src/modules/api/actions/api.js 50.47% <0%> (ø) ⬆️
addons/knobs/src/react/WrapStory.js 19.54% <0%> (ø) ⬆️
...s/left_panel/stories_tree/tree_decorators_utils.js 45.23% <0%> (ø) ⬆️
...codemod/src/transforms/update-organisation-name.js 40.62% <0%> (ø) ⬆️
lib/ui/src/modules/ui/configs/init_panels.js 25% <0%> (ø) ⬆️
lib/ui/src/modules/ui/containers/down_panel.js 23.52% <0%> (ø) ⬆️
...rc/modules/ui/components/left_panel/text_filter.js 30.98% <0%> (ø) ⬆️
addons/knobs/src/components/types/Color.js 8.1% <0%> (ø) ⬆️
... and 11 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 c2a965e...4613dcb. Read the comment docs.

@Hypnosphi
Copy link
Member

This is already fixed in release/3.3 with #1701, but maybe we should do the same on master
@ndelangen what do you think?

@Hypnosphi Hypnosphi requested a review from ndelangen November 3, 2017 18:47
Copy link
Member

@ndelangen ndelangen left a comment

Choose a reason for hiding this comment

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

great!

@Hypnosphi
Copy link
Member

Should we backport the changes in docs as well?
https://github.com/storybooks/storybook/pull/1701/files

@ndelangen
Copy link
Member

Yes that sounds like the logical thing to do!

@ndelangen
Copy link
Member

@jebarjonet Do you want to add the documentation changes?

@Hypnosphi
Copy link
Member

@jebarjonet do you want to do that? It should be just a matter of copypasting

@jebarjonet
Copy link
Contributor Author

@ndelangen @Hypnosphi I updated the doc :) (I did not update stories as they are not the same as in release/3.3)

@Hypnosphi Hypnosphi merged commit 27d4f41 into storybookjs:master Nov 4, 2017
@ndelangen
Copy link
Member

@jebarjonet Thank you for this, really appreciate you helping us out, every small improvement helps! 🙇

If you want to help more and are looking for something, feel free to message me 👍

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