-
-
Notifications
You must be signed in to change notification settings - Fork 502
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
Conversation
View your CI Pipeline Execution ↗ for commit c09f683
☁️ 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.55% 90.35% +0.80%
==========================================
Files 34 36 +2
Lines 1493 1617 +124
Branches 370 384 +14
==========================================
+ Hits 1337 1461 +124
Misses 139 139
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. |
users are able to do conditional rendering, but TS generally doesn't pick up on it. Therefore, while it is less type safe, it allows users to use field groups in more locations than previously possible.
@LeCarbonator I think there's a couple ways that could maybe work. The prop could be just named group that could either accept an object or an object of properties mapped to form properties. This would probably require a form subscribe.
Alternatively, you could add a new option to the createFormHook called groupComponents. They could be called similar to field components, but their name prop could take a string or an object of key values similar to how the withFieldGroup works. Then it could use a useGroupContext. This is all completely brainstorming so it may not be possible. But you could call it like
The current approach still works, though, and if that makes more sense to other users, I'm ok being told I'm wrong. I'm just throwing out some ideas. I really like this library. It took me a minute to get it, but once I did it's very intuitive. I just need this last group feature to be able to use it. |
@MPiland Alright, here's my thoughts on the snippets: <form.Subscribe select={(state) => state.values)}>
{(values) => {
<Auth group={values.auth} form={form} />
}
</form.Subscribe> I'm heavily against this implementation. The reason is that field groups aren't just displaying values, they map fields called within to the form outside. This would force it to be a one-way street which goes against a lot of the features other users want. As for /* Do you want granular control? */
<form.AppField name="firstName">
{field => <>
<label>First Name</label>
<field.TextInput />
<field.Feedback />
</>}
</form.AppField>
/* Or wrap it all in a component? */
<form.AppField name="firstName">
{field => <field.TextInput showFeedback label="First Name" />}
</form.AppField> Field groups don't really fit that category, as they're meant to do the work themselves. Once you have the field group, there's really only one way to use it, and that is like you showed: /* Combinations don't really exist. Either you use the field group or you don't */
<form.AppGroup name='auth'>
{({ AuthGroup }) => (
<AuthGroup {...props} />
)} />
</form.AppGroup> That's why I prefer the HOC implementation. Hopefully it's clear what I'm trying to convey. |
@LeCarbonator That all makes sense to me! I will defer to your expertise. I think the original way works as well. I appreciate you entertaining my thoughts! |
Will this feature work if multiple forms share the same UI but have different fields? Example: |
Not with its current implementation, no. The intent of this feature is to group multiple fields together so they can all be reused. The Section B you describe is not consistent across forms, but is instead a collection of a An example usage of this field group would be: Section: calories, elevationGain, distance
|
…m into form-group-api
the reset is already accessible through group.form.reset.
It goes for all createFormHook extracts. |
Good catch! I'll add those annotations tomorrow (alongside fixing the failing unit tests at the moment) |
As discussed on Discord, it appears to have been because of pnpm cache instead of a problem with the PR. |
@LeCarbonator Firstly, thanks so much for putting this together. I've been really looking forward to this feature! 🙌 Just wondering if there's anything blocking this from being merged, or if any help is needed to move it forward. Also curious if there's any idea of when it might be released (no pressure, just excited to try it out!). |
@AfrazHussain not particularly. Things have slowed down a notch since it is the middle of summer, but apart from reviewing the revised version, there's no blocking part of this PR. The PR is installable using the scripts provided by this comment. Of course it's not part of the main branch yet, but it's testable. I'm sure it will be released soon! If you notice a problem in the current code, feel free to leave a review comment so it can be addressed. |
Ah, that is quite helpful. Thanks a lot! I'll try it out and then report back if there's any feedback/issues. |
Did you manage to figure it out? I don't understand why you were ignored, just to stumble across the test boxes. |
@ArtemFH The discussion was continued on Discord, so it looks like it was lost here.
We'll fix it by exporting the types very soon, but in case you need a current workaround, that configuration should be turned off. |
What's the recommended workaround for those that do need to emit declarations? If there's none, it's ok. I'll wait for the types to be exported. |
No pretty ones apart from going into TanStack Form's declaration files and exporting the reported types @zhouzi |
@LeCarbonator you mentioned this api is intended to be used when fields are closely related but that's still not so clear to me. Could you give examples of use cases? Also, could that be used for a multi-step form? |
@Harukisatoh Here's an example of fields that depend on each other and benefit greatly from field groups: Example: I have 5 forms that all include intervals or date ranges. For that, I want a start date and an end date, and ensure that the end date cannot be set before the start date. Since the user can set the time, I would also like to format the fields on blur so that Field groups allow me to not only make a group of both fields, but also use 'references' to other fields - Which means I can decide how granular those groups should be // Case 1: my field group creates fields for Start and End times.
<IntervalFields form={form} fields={{
startDate: 'my.nested.start.date',
endDate: 'my.nested.end.date'
}} /> Notice how I now forced my fields to be together. What if I want to make them stack vertically instead of horizontally? What if something is supposed to go between them? With field groups, I can go for this structure instead: // Case 2: Two field groups, one for each field
<IntervalStartField form={form} fields={{
// will create the field
startDate: 'my.nested.start.date',
// won't be a field, but I can reference it for setting / reading / subscribing values
endDateReference: 'my.nested.end.date'
}} />
<AnythingIWant />
<IntervalEndField form={form} fields={{
// will create the field
endDate: 'my.nested.end.date',
// won't be a field, but I can reference it for setting / reading / subscribing values
startDateReference: 'my.nested.start.date'
}} /> Does that clear it up? |
Thanks for #1661 @LeCarbonator 🙏 I upgraded to v1.15.3 and the migration from |
Closes #1296
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: