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

Fixed prop type validation errors in info addon #1374

Closed
wants to merge 13 commits into from

Conversation

Li0liQ
Copy link
Contributor

@Li0liQ Li0liQ commented Jun 28, 2017

Issue: #1305

What I did

Updated PropVal.js to pass its props to child elements in case of arrays and objects.

How to test

Render a story using addWithInfo with a component that receives an array or an object as a prop. Open console to verify you no longer receive errors due to properties validation for PropVal component.

@Li0liQ Li0liQ changed the title Fix for #1305. Updated PropVal.js to pass its props to child elements in case of arrays and objects. Updated PropVal.js to pass its props to child elements in case of arrays and objects Jun 28, 2017
@Li0liQ Li0liQ changed the title Updated PropVal.js to pass its props to child elements in case of arrays and objects Update PropVal to pass its props to child elements in case of arrays and objects Jun 28, 2017
@Li0liQ Li0liQ changed the title Update PropVal to pass its props to child elements in case of arrays and objects Fixed PropVal prop type validation errors in info addon Jun 28, 2017
@Li0liQ Li0liQ changed the title Fixed PropVal prop type validation errors in info addon Fixed prop type validation errors in info addon Jun 28, 2017
@codecov
Copy link

codecov bot commented Jun 28, 2017

Codecov Report

Merging #1374 into release/3.3 will decrease coverage by 1.72%.
The diff coverage is 100%.

Impacted file tree graph

@@               Coverage Diff               @@
##           release/3.3    #1374      +/-   ##
===============================================
- Coverage        23.47%   21.75%   -1.73%     
===============================================
  Files              311      355      +44     
  Lines             6538     7069     +531     
  Branches           827      914      +87     
===============================================
+ Hits              1535     1538       +3     
- Misses            4399     4842     +443     
- Partials           604      689      +85
Impacted Files Coverage Δ
addons/info/src/components/PropVal.js 42.05% <100%> (+0.64%) ⬆️
app/react/src/server/utils.js 0% <0%> (-53.58%) ⬇️
addons/knobs/src/components/types/Text.js 50% <0%> (-16.67%) ⬇️
addons/knobs/src/components/types/Array.js 84.61% <0%> (-8.72%) ⬇️
addons/info/src/components/Props.js 36.36% <0%> (-0.85%) ⬇️
addons/knobs/src/components/types/Object.js 16.66% <0%> (-0.58%) ⬇️
addons/info/src/components/Story.js 76.19% <0%> (-0.29%) ⬇️
addons/knobs/src/components/types/Color.js 8.1% <0%> (-0.12%) ⬇️
addons/storyshots/src/test-bodies.js 19.14% <0%> (ø) ⬆️
... and 116 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 8d92d0c...0fede88. Read the comment docs.

@Li0liQ
Copy link
Contributor Author

Li0liQ commented Jun 28, 2017

Making eslint happy makes codebeat unhappy.
Should we say that previewArray and previewObject are functional react components, add prop types validation for them and start passing all the arguments as a single props argument?

@ndelangen ndelangen changed the base branch from master to release/3.3 August 25, 2017 22:06
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.

@shilman @usulpro Can you verify this PR is still valid considering #1501 ?

@ndelangen ndelangen added addon: info maintenance User-facing maintenance tasks labels Aug 25, 2017
@usulpro
Copy link
Member

usulpro commented Aug 26, 2017

I'd prefer to completely change our approach for source generation.
We discuss possible solution in #1602

@danielduan
Copy link
Member

danielduan commented Oct 30, 2017

This seems like a good bug fix that we should be merging into master? Doesn't seem like our Info refactor is gonna happen for 3.3.

edit: just kidding, I hate how we do our branch merges. It makes switching branches almost impossible. Let's get this into 3.3.

@danielduan danielduan changed the base branch from release/3.3 to master October 30, 2017 15:26
@danielduan danielduan changed the base branch from master to release/3.3 October 30, 2017 15:27
@@ -51,13 +52,19 @@ function previewArray(val, maxPropArrayLength) {
return <span style={valueStyles.array}>[{createFragment(items)}]</span>;
}

function previewObject(val, maxPropObjectKeys) {
previewArray.propTypes = {
Copy link
Member

Choose a reason for hiding this comment

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

previewArray is never rendered as a component, it's only called as a function. So looks like those propTypes aren't really checked at any point. You should either remove them, or capitalize the name (PreviewArray) and replace previewArray(props) below with <PreviewArray {...props} />.

If you decide to do the latter, please note that val should rather be PropTypes.array, and don't disable all of the eslint rules — only the particular ones (// eslint-disable react/forbid-prop-types)

@codecov-io
Copy link

Codecov Report

Merging #1374 into release/3.3 will increase coverage by 0.76%.
The diff coverage is 100%.

Impacted file tree graph

@@               Coverage Diff               @@
##           release/3.3    #1374      +/-   ##
===============================================
+ Coverage        22.74%   23.51%   +0.76%     
===============================================
  Files              326      311      -15     
  Lines             6750     6546     -204     
  Branches           847      824      -23     
===============================================
+ Hits              1535     1539       +4     
+ Misses            4603     4409     -194     
+ Partials           612      598      -14
Impacted Files Coverage Δ
addons/info/src/components/PropVal.js 42.05% <100%> (+0.64%) ⬆️
addons/links/src/react/components/link.js 16.66% <0%> (-67.95%) ⬇️
app/vue/src/server/utils.js 0% <0%> (-53.58%) ⬇️
addons/knobs/src/react/WrapStory.js 19.54% <0%> (-29.04%) ⬇️
...codemod/src/transforms/update-organisation-name.js 40.62% <0%> (ø) ⬆️
lib/ui/src/modules/ui/components/layout/usplit.js 38.7% <0%> (ø) ⬆️
addons/knobs/src/components/PropForm.js 9.09% <0%> (ø) ⬆️
addons/knobs/src/components/types/Number.js 8.06% <0%> (ø) ⬆️
addons/info/src/components/types/Signature.js 16.66% <0%> (ø) ⬆️
... and 40 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 5c34ccf...9090186. Read the comment docs.

@danielduan
Copy link
Member

Gonna close this PR due to inactivity. Feel free to ping us if you'd like to keep working on it.

@danielduan danielduan closed this Dec 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addon: info maintenance User-facing maintenance tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants