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

Add excludedPropTypes as an option to info addon #3468

Conversation

andyo729
Copy link
Contributor

@andyo729 andyo729 commented Apr 21, 2018

Issue: #3408

What I did

I updated the info addon to include an excludedPropTypes option to exclude those prop types from the PropTable.

How to test

There are two new tests in PropTable, one for the default behavior and another for the new filtering behavior.

}

return propDefinitions.filter(
propDefinition => !excludedPropTypes.includes(propDefinition.property)
Copy link
Member

Choose a reason for hiding this comment

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

You need to import a polyfill on top of this file:

import 'core-js/fn/array/includes';

See https://babeljs.io/docs/plugins/transform-runtime/

NOTE: Instance methods such as "foobar".includes("foo") will not work since that would require modification of existing built-ins

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pulled the polyfill into the PropTable

@Hypnosphi
Copy link
Member

Please add a usage example to examples/official-storybook

@@ -331,7 +331,7 @@ class Story extends React.Component {
const array = Array.from(types.keys());
array.sort((a, b) => getName(a) > getName(b));

const { maxPropObjectKeys, maxPropArrayLength, maxPropStringLength } = this.props;
const { maxPropObjectKeys, maxPropArrayLength, maxPropStringLength, excludedPropTypes } = this.props;
Copy link
Member

Choose a reason for hiding this comment

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

Do we have a place to document this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added this new option to the README

@codecov
Copy link

codecov bot commented Apr 22, 2018

Codecov Report

Merging #3468 into master will increase coverage by 0.14%.
The diff coverage is 92.3%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3468      +/-   ##
==========================================
+ Coverage   37.42%   37.56%   +0.14%     
==========================================
  Files         457      457              
  Lines       10304    10320      +16     
  Branches      906      907       +1     
==========================================
+ Hits         3856     3877      +21     
+ Misses       5910     5904       -6     
- Partials      538      539       +1
Impacted Files Coverage Δ
addons/info/src/index.js 78.57% <ø> (ø) ⬆️
addons/info/src/components/Story.js 88.58% <100%> (+0.56%) ⬆️
addons/info/src/components/PropTable.js 88.52% <90.9%> (+3.41%) ⬆️
addons/links/src/react/components/link.js 76.56% <0%> (ø) ⬆️
addons/knobs/src/components/types/Files.js 14.28% <0%> (ø) ⬆️
lib/core/src/client/preview/syncUrlWithStore.js 40.9% <0%> (ø) ⬆️
lib/core/src/server/build-dev.js 20.15% <0%> (ø) ⬆️
addons/actions/src/lib/types/function/index.js 77.77% <0%> (ø) ⬆️
lib/ui/src/modules/api/actions/api.js 51.85% <0%> (ø) ⬆️
addons/jest/src/hoc/provideJestResult.js 0% <0%> (ø) ⬆️
... and 70 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 1df59f7...e3acf75. Read the comment docs.

@@ -59,6 +59,14 @@ storiesOf('Addons|Info.Options.inline', module).add(
})(() => <BaseButton label="Button" />)
);

storiesOf('Addons|Info.Options.excludedPropTypes').add(
Copy link
Member

Choose a reason for hiding this comment

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

please don't forget adding second module parameter, this is needed for HMR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍Added back in

@@ -1,5 +1,6 @@
import PropTypes from 'prop-types';
import React from 'react';
import 'core-js/fn/array/includes';
Copy link
Member

Choose a reason for hiding this comment

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

You need to add core-js as dependency to addons/info/package.json

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added

yarn.lock Outdated
@@ -4058,6 +4058,10 @@ copy-webpack-plugin@~4.4.1:
p-limit "^1.0.0"
serialize-javascript "^1.4.0"

core-js@2.5.5:
version "2.5.5"
resolved "http://npm.kroger.com/core-js/-/core-js-2.5.5.tgz#b14dde936c640c0579a6b50cabcc132dd6127e3b"
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this is your private registry and other people can't install from there. You might need to add an empty .npmrc to project root (without commiting it), and run yarn again

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing that out. This has been updated.

@andyo729 andyo729 force-pushed the feature/add-excluded-proptypes-to-with-info branch from e4600cd to 14a2888 Compare April 24, 2018 23:04
@andyo729 andyo729 force-pushed the feature/add-excluded-proptypes-to-with-info branch from 14a2888 to 6286f12 Compare April 24, 2018 23:47
Copy link
Member

@Hypnosphi Hypnosphi left a comment

Choose a reason for hiding this comment

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

…d-proptypes-to-with-info

# Conflicts:
#	examples/official-storybook/stories/__snapshots__/addon-info.stories.storyshot
@Hypnosphi Hypnosphi merged commit f9635a0 into storybookjs:master Apr 27, 2018
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.

4 participants