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

Advanced prop type definitions #105

Merged
merged 49 commits into from
Jun 9, 2020
Merged

Advanced prop type definitions #105

merged 49 commits into from
Jun 9, 2020

Conversation

jerelmiller
Copy link
Contributor

Description

Adds deprecation messages, additional info about complex prop types, and prop type examples.

Reviewer Notes

There is 1 minor inconsistency with the SDK docs for the EntityCountQuery filters prop type. You will notice the value is oneOf, but nothing is rendered for the constants. We will need to address this on the SDK itself.

Related Issue(s) / Ticket(s)

If there are any related GitHub Issues or JIRA tickets, add links to them here.

Screenshot(s)

Screen Shot 2020-06-08 at 9 14 07 AM

Screen Shot 2020-06-08 at 9 13 51 AM

Screen Shot 2020-06-08 at 3 55 42 PM

Screen Shot 2020-06-08 at 3 55 54 PM

Screen Shot 2020-06-08 at 9 13 32 AM

<div>{'>'}</div>
</div>
) : (
<PropTypeInfo type={itemTypes} />
Copy link
Contributor

Choose a reason for hiding this comment

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

recursive react! never thought about doing this but this is super cool

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes! Its not often that you need it (I can only think of 1 other time I've done this), but super useful when you do need it. It helped me so much here.

@@ -45,8 +137,15 @@ PropList.propTypes = {
PropTypes.shape({
name: PropTypes.string.isRequired,
description: PropTypes.string,
deprecation: PropTypes.shape({
Copy link
Contributor

Choose a reason for hiding this comment

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

if a PropType doesn't take the shape value (for example there's no deprecation) will this value be null/undefined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is correct! You can see it set explicitly to null here: https://github.com/newrelic/developer-website/pull/105/files#diff-0645d45905de3e0b87864beb8891fb71R182

I don't set the isRequired on this shape since I know that it can be null.

propName,
type.isOneOf,
staticName
return Object.entries(component.propTypes || {}).map(([name, propType]) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

if this is component.propTypes, wouldn't it just run the map on an empty array? like the same as
component.propTypes && Object.entries.map(([name,propType)=>...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason why I opted not to do something like component.propTypes && Object.entries is because of the return value I wanted here. In the event that component.propTypes is falsey (undefined, null, 0, false, empty string), the left hand side of the boolean operation is returned (i.e. null && Object.entries(...) === null). This is not something I wanted, but rather I wanted to return an empty array in case there were no prop types: Object.entries({}).map(...) => []

];

const getArgs = (propType) =>
propType.__reflect__.find(({ args }) => args)?.args;
Copy link
Contributor

Choose a reason for hiding this comment

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

what is __reflect___?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This must be something that the tool generating the docs in the SDK sets for our use. I'm not actually sure of the mechanics of it, but we are using it to get access to the underlying structures for the prop types (for example, the args).

Let's say the prop type is set to the following:

space: PropTypes.oneOf(['sm', 'md', 'lg'])

The __reflect__ property gets me access to things like the name of the prop type used (oneOf), the args passed to it (['sm', 'md', 'lg']), etc. Does that help? I know Java has a way of introspecting underlying type info and stuff in a process called Reflection. Maybe the name comes from that?

@zstix zstix added the enhancement New feature or request label Jun 9, 2020
@jerelmiller jerelmiller requested a review from zstix June 9, 2020 16:38
<div>{'<One of'}</div>
<div className={styles.arg}>
{type.meta.constants.map((constant) => (
<div key={constant}>{constant},</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

We might want to trim off the , on the last item in this list. Maybe we could do a .join(',) on an array of divs and then slice or something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only reason I didn't do that was only because One Core has trailing commas:

Screen Shot 2020-06-09 at 9 51 04 AM

This isn't to say this is our desired result for the new dev site, but just the reason I did this to begin with. Should we do the same? Or remove the last comma?

FYI, .join() unfortunately doesn't work on the <div /> call since the return value of <div /> is an object (since its calling React.createElement under the hood). Calling .join unfortunately results in an [object Object] rendered to the screen. That being said, we can use indexes to determine whether to render the comma or or not depending on if its the last item.

@jerelmiller jerelmiller merged commit d01b3df into master Jun 9, 2020
@jerelmiller jerelmiller deleted the jerel/advanced-props branch June 9, 2020 17:05
@nr-opensource-bot
Copy link
Contributor

🎉 This PR is included in version 1.0.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants