Skip to content
This repository was archived by the owner on May 24, 2024. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions packages/terra-props-table/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,9 @@ Changelog

Unreleased
----------
### Removed
* Implicit inferring of an `enum`'s type based on its first child.

### Changed
* Formatting changes per eslint v4 update

Expand Down
8 changes: 1 addition & 7 deletions packages/terra-props-table/src/PropsTable.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -26,14 +26,8 @@ function formatShape(shape) {
function determineType(type) {
let typeName = type.name;

// Pull the first value off and use that as type.
// This assumes all enumerable values are the same type.
if (typeName === 'enum') {
if (Number.isNaN(Number(type.value[0].value))) {
typeName = typeof type.value[0].value;
} else {
typeName = 'number';
}
typeName = 'enum';
Copy link
Contributor

Choose a reason for hiding this comment

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

Ohh I think I miss understood what you meant when you had asked me about this change. enum would not be the correct prop type in most cases, I thought you meant you would parse the first value to determine the type. Looking in the terra-repos,oneOf is usually an enum of strings options... so this maps to a string prop essentially.

If we only indicate an enum, this would still require one to look in the code to verify the enum options.

Additionally, to me this indicated a more 'rigid' prop value than what will be required by most props.

Copy link
Contributor

Choose a reason for hiding this comment

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

What was the reason this was displayed as undefined? I know this occurs because we want to pass Object.keys(DEFAULTS), but what information is given as the type value?

Copy link
Contributor

Choose a reason for hiding this comment

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

If we only indicate an enum, this would still require one to look in the code to verify the enum options.

Listing the available enum keys in the code comment above the prop that generates the doc could help with this

Additionally, to me this indicated a more 'rigid' prop value than what will be required by most props.

In previous meetings we've talked about not using enums as propTypes but still exporting them so people could use them if they wanted. For example, in the card props image, we'd make the propType a string and list out the values we accept while also exporting an enum that's values were strings. Users could type the string directly or use the enum.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've dug into the react-docgen code a bit more and it looks like they may handle the case #1463 was logged for.

We've opted to do custom type checking, so we miss out on this feature from react-docgen, but I'd recommend taking a look at this PR and this PR which seem to have added support in react-docgen for resolving Object.keys() in PropType.oneOf() and potentially resolving spreads. Here is a util they've made to help resolve enums.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What was the reason this was displayed as undefined

This was displaying as undefined because when we would try to examine typeof type.value[0].value, we would in fact be trying to call typeof on Object.values(<whatever the name of your enumerated object was>), rather than on [/*the actual array that Object.values() returns */] – essentially, we'd be making the call before the Object.values was evaluated, and hence before the objects values were converted into an array. So trying to access the first item in type.value and look at it's value would return undefined, so typeName was inadvertently getting set to undefined here.

Copy link
Contributor

Choose a reason for hiding this comment

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

@dylan-sftwr @bjankord Ah I see what you are saying the issue is. ok. LGTM.

} else if (typeName === 'arrayOf') {
if (type.value.name === 'shape') {
typeName = <span> array of objects structured like: <pre className={cx('props-table-pre')}> {formatShape(type.value.value)} </pre></span>;
Expand Down
124 changes: 124 additions & 0 deletions packages/terra-props-table/src/PropsTable.jsx.orig
Original file line number Diff line number Diff line change
@@ -0,0 +1,124 @@
/* eslint-disable import/no-extraneous-dependencies */
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure whats up with this file, looks like this wasn't meant to be checked into version control

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops! looks like that was left over from my diff, f85e7b2

import React from 'react';
import PropTypes from 'prop-types';
import { parse } from 'react-docgen';
import Markdown from 'terra-markdown';
import classNames from 'classnames/bind';
import styles from './PropsTable.scss';

const cx = classNames.bind(styles);

const propTypes = {
/**
* Title of component
*/
componentName: PropTypes.string,
/**
* Markdown source file
*/
src: PropTypes.string.isRequired,
};

function formatShape(shape) {
return JSON.stringify(shape, null, 1);
}

function determineType(type) {
let typeName = type.name;

if (typeName === 'enum') {
<<<<<<< HEAD
if (Number.isNaN(Number(type.value[0].value))) {
typeName = typeof type.value[0].value;
} else {
typeName = 'number';
}
=======
typeName = 'enum';
>>>>>>> c55554b1... ISSUE-1463: Fixed undefined prop type error
} else if (typeName === 'arrayOf') {
if (type.value.name === 'shape') {
typeName = <span> array of objects structured like: <pre className={cx('props-table-pre')}> {formatShape(type.value.value)} </pre></span>;
} else {
typeName = `array of ${type.value.name}s`;
}
} else if (typeName === 'union') {
const options = type.value.map((option) => {
const name = option.name === 'shape' ? ((
<span key={option.value}> an object structured like:
<pre className={cx('props-table-pre')}> {formatShape(option.value)} </pre>
</span>
)) : (<span key={option.name}> {option.name}</span>);
return name;
});
typeName = options.reduce((curr, next) => [curr, <span key={`${curr.value}-${next.value}`}> or </span>, next]);
} else if (typeName === 'shape') {
typeName = <span> an object structured like: <pre className={cx('props-table-pre')}> {formatShape(type.value)} </pre></span>;
}

return typeName;
}

/**
* Renders a table view for the props metadata of a react component generated by react-docgen
*/
const PropsTable = ({ componentName, src, ...customProps }) => {
/**
* Runs component source code through react-docgen
* @type {Object}
*/
const componentMetaData = parse(src);

/**
* Alias for props object from componentMetaData
* @type {Object}
*/
const componentProps = componentMetaData.props;

const tableRowClass = cx('prop-table-row');
const tableClassNames = cx([
'props-table',
customProps.className,
]);

return (
<div dir="ltr" className="markdown-body">
<h2>{componentName} Props</h2>
<table {...customProps} className={tableClassNames}>
<thead>
<tr>
<th className={cx('prop-table-name')}>Prop Name</th>
<th className={cx('prop-table-type')}>Type</th>
<th className={cx('prop-table-required')}>Is Required</th>
<th className={cx('prop-table-default')}>Default Value</th>
<th className={cx('prop-table-description')}>Description</th>
</tr>
</thead>
<tbody>
{Object.keys(componentProps).map((key) => {
const prop = componentProps[key];
const type = determineType(prop.type);

return (
<tr className={tableRowClass} key={key} style={{ fontSize: '90%' }}>
<td style={{ fontWeight: 'bold' }}>{key}</td>
<td>{(prop.type ? type : '')}</td>
{(prop.required ?
<td style={{ color: '#d53700' }}>required</td>
: <td style={{ color: '#444' }}>optional</td>)}
{(prop.defaultValue ?
<td style={{ fontWeight: 'bold' }}>{prop.defaultValue.value}</td>
: <td style={{ color: '#444' }}>none</td>)}
<td><Markdown src={prop.description} /></td>
</tr>
);
})}
</tbody>
</table>
</div>
);
};

PropsTable.propTypes = propTypes;

export default PropsTable;