-
-
Notifications
You must be signed in to change notification settings - Fork 493
Add Form component FormData generic typing #426
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 Form component FormData generic typing #426
Conversation
Codecov Report
@@ Coverage Diff @@
## master #426 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 10 10
Lines 207 207
Branches 61 61
=====================================
Hits 207 207Continue to review full report at Codecov.
|
|
This pull request introduces 1 alert when merging 657125c into 768a476 - view on LGTM.com new alerts:
Comment posted by LGTM.com |
|
|
|
@wtgtybhertgeghgtwtg I think that would require changing the type definitions inside |
|
I'd guess make them generic with a default in a minor release for |
|
@michalwarda I'd accept the PR for the FF changes, and then we can merge this RFF change, just like @wtgtybhertgeghgtwtg said. @wtgtybhertgeghgtwtg I'm curious about the sentence that your username is abbreviating. |
|
It's not an abbreviation, I just mashed the keyboard. |
|
@erikras I'll do it in few hours :) |
|
Are the changes going into FF the blocker currently for this or is it something else? |
| RenderableProps<FormRenderProps> { | ||
| subscription?: FormSubscription | ||
| decorators?: Decorator[] | ||
| initialValuesEqual?: (a?: object, b?: object) => boolean |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't these also be FormData?
|
|
||
| export var Field: React.ComponentType<FieldProps> | ||
| export var Form: React.ComponentType<FormProps> | ||
| export class Form<FormData = object> extends React.Component< |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Form is actually implemented as a function, not as a class, so this type could be misleading couldn't it?
It would suggest I can create instances a la const form = new Form()?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been doing this if it helps.
<FormData = object>(
props: FormProps<FormData>,
) => React.ReactElement<FormProps<FormData>>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| subscription?: FormSubscription | ||
| } | ||
|
|
||
| export var Field: React.ComponentType<FieldProps> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice for Field to get the same treatment.
|
Can you resubmit this PR (or resolve the conflicts)? |
|
Thank you for this contribution! It provided guidance and impetus to getting this feature into the library. I think #516 did what this PR did, so I think this one is obsolete now. |
|
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
In TypeScript you can use generic syntax inside of JSX code to allow typing components.
react-final-formexposes a way to typeConfigFormDatathroughcreateFormbut there is no way to access it inreact-final-form.This pull request exposes the possibility to pass
FormDatatyping toFormcomponent.I've also added typings tests for that.