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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: form to accept react component #1511

Merged
merged 1 commit into from Nov 20, 2019
Merged

fix: form to accept react component #1511

merged 1 commit into from Nov 20, 2019

Conversation

chanceaclark
Copy link
Contributor

@chanceaclark chanceaclark commented Nov 12, 2019

Reasons for making this change

In development, we weren't able to pass a react component into the tagName prop. This is valid in React but was unable to test in development due to PropTypes.

It also looked like some packages weren't properly locked, so I've updated those.

Side Note: We may want to look into migrating to yarn. Just a 馃挱

Checklist

  • I'm adding or updating code
    • I've added and/or updated tests
    • I've updated docs if needed

@@ -428,7 +428,7 @@ if (process.env.NODE_ENV !== "production") {
onSubmit: PropTypes.func,
id: PropTypes.string,
className: PropTypes.string,
tagName: PropTypes.string,
tagName: PropTypes.oneOfType([PropTypes.string, PropTypes.elementType]),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I should be able to make this just PropTypes.elementType 馃

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, does PropTypes.elementType include strings?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it's a ReactComponentLike:

export type ReactComponentLike =
    | string
    | ((props: any, context?: any) => any)
    | (new (props: any, context?: any) => any);

Copy link
Member

@epicfaace epicfaace left a comment

Choose a reason for hiding this comment

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

Looks good, thanks. I wonder if tagName is now a bit misleading though since it really refers to a component name...

@chanceaclark
Copy link
Contributor Author

chanceaclark commented Nov 18, 2019

@epicfaace I agree, but if we wanted to change it, it would be a BREAKING CHANGE. I guess since v2 is in alpha it would be fine to add a BC?

And since rjsf is converting to TypeScript, I think it might be more beneficial for development to restrict the prop via typings vs. prop-types.

@epicfaace
Copy link
Member

Yeah, true. I'll go ahead and merge this for now, though. Do you have any suggestions for what a better prop name might be?

@epicfaace epicfaace merged commit 561dff1 into rjsf-team:master Nov 20, 2019
@chanceaclark
Copy link
Contributor Author

We could go for the as prop. It's used in some libraries, like styled-components. Material-ui and Atlaskit uses the component prop, which IMO, prefer.

@chanceaclark chanceaclark deleted the fix/custom-form-comp branch November 20, 2019 21:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants