-
-
Notifications
You must be signed in to change notification settings - Fork 465
feat(react-form): Add withFieldGroup
#1469
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
base: main
Are you sure you want to change the base?
Conversation
View your CI Pipeline Execution ↗ for commit 8ce40fe.
☁️ Nx Cloud last updated this comment at |
Issues that could be addressed:
This has now been addressed. If onSubmitMeta is unset, any value will do. If it is set, you must match it exactly. |
While the previous separate implementation was compatible with AppForm, users wouldn't have been able to use any field/form components in the render itself. This commit allows them to do that, at the expense of not being compatible with useForm.
The unit tests should be reusable in case this isn't the desired approach. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1469 +/- ##
==========================================
+ Coverage 89.24% 90.10% +0.86%
==========================================
Files 31 33 +2
Lines 1432 1557 +125
Branches 366 380 +14
==========================================
+ Hits 1278 1403 +125
Misses 137 137
Partials 17 17 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Related PR: #1334 |
Something to consider:
|
This type will help with a lens API for forms so that only names leading to the subset data are allowed.
#1475 sounded like an interesting idea, so I'll try to tinker with that and come up with unit tests. The note at the top of the PR still applies. |
…k-form into form-group-api
Strange, the derived state from the form lens isn't updating ... Issue: React input unfocuses when changing input. Unsure why. |
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.
DUDE - this is awesome. I'm genuinely so excited about it. I know I've gushed about this API in our private maintainer's channel, but want to share that excitement here as well.
Outside of these 7 items I think there's one big one:
I want to change this API's name to fieldGroup
, where the lens
property becomes group
and the withFormLens
becomes withFieldGroup
.
I think the proposed name change feels more genuine and less CS-focused.
this.store = new Derived({ | ||
deps: [this.form.store], | ||
fn: ({ currDepVals }) => { | ||
const currFormStore = currDepVals[0] | ||
const { | ||
errors: formErrors, | ||
errorMap: formErrorMap, | ||
values: _, | ||
...rest | ||
} = currFormStore | ||
|
||
const { errorMap: lensErrorMap, errors: lensErrors } = | ||
this.form.getFieldMeta(this.name) ?? { | ||
...defaultFieldMeta, | ||
} | ||
|
||
return { | ||
...rest, | ||
lensErrorMap, | ||
lensErrors, | ||
formErrorMap, | ||
formErrors, | ||
values: this.form.getFieldValue(this.name) as TLensData, | ||
} | ||
}, | ||
}) |
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.
This is a pretty broad design question that I don't know the answer to yet, so I want you and other @TanStack/form-maintainers to weigh in:
Should we:
- Keep the derived state inside of the
FormLens
class? - Register a
FormLens
instance to theForm
class and compute the derived state inside of theForm
class?
We currently do 2 for Field
s, but lenses might be sufficiently different from what we want to do.
The reason I loosely lean towards 2 and why I bring it up at all:
The current implementation will cause more re-renders than needed to due lack of caching values from prevDepVals
; a pattern we follow for fields (but is tricky to get right).
We can fix this in the current implementation, but feels strange to have fields and lenses behave differently in how they compute their derived state
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.
The thing is, the lens itself should ideally not exist. It should merely be a translator between fields and a form using some form of mapping.
With this in mind, i think I will remove most state values as they are not related to it and can be accessed differently (submissionAttempts getting fetched from group.form.store
instead of directly)
However, values
would be convenient to have derived somehow for easier useStore
and Subscribe
. I'm just not sure how to go about it.
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 would lean towards 2 myself, it feels odd to have a separate state outside the form. Though it seems we're all in agreement
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.
The revised version tries to keep as little store as possible in the field group, since it's not its true purpose. Thoughts? @harry-whorlow @crutchcorn
After some discussion with @LeCarbonator - we thought of a feature addition: const Parent = () => {
// Docs change: Prefix all `withFieldGroup` components with `FieldGroup` to avoid confusion
const FieldGroupPassword = withFieldGroup({
defaultValues: {
firstName: '',
lastName: '',
},
render: ({ group, title }) => {
// ...
},
})
const form = useAppForm({
...formOpts,
})
// This is actually pretty type-safe since we'll get an error if the shape of the `fields` doesn't match
// the form name type.
export const fields = {
"password": 'password',
"confirm_password": 'confirm_password',
} as const;
return (
<FieldGroupPassword
form={form}
{/* Rename the `name`*/}
{/* Either */}
fields="person"
{/* OR */}
fields={fields}
{/* Via an overload */}
/>
)
} |
…m into form-group-api
withFormLens
withFieldGroup
the method appears to be a helper function for form.reset, which is accessible from the field group API. There does not seem to be a good reason to port this method from FormApi.
Todos
This PR implements a variation of
withForm
that can be used to create form groups. This form group allows extendingdefaultValues
and has no expectations of form level validators.This distinguishes it both from
withForm
as well as instantiations of forms.Here's an extract of the documentation:
Reusing groups of fields in multiple forms
Sometimes, a pair of fields are so closely related that it makes sense to group and reuse them — like the password example listed in the linked fields guide. Instead of repeating this logic across multiple forms, you can utilize the
withFieldGroup
higher-order component.Rewriting the passwords example using
withFieldGroup
would look like this:We can now use these grouped fields in any form that implements the default values:
Mapping field group values to a different field
You may want to keep the password fields on the top level of your form, or rename the properties for clarity. You can map field group values
to their true location by changing the
field
property:Important
Due to TypeScript limitations, field mapping is only allowed for objects. You can use records or arrays at the top level of a field group,
but you will not be able to map the fields.
If you expect your fields to always be at the top level of your form, you can create a quick map
of your field groups using a helper function: