-
-
Notifications
You must be signed in to change notification settings - Fork 479
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. |
…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.
@LeCarbonator I've been toying this PR (I know it's not done) because it's exactly what I need. I'm not an experienced developer by any means, but I am a product manager by trade and so I'm used to thinking about user experience or in this case developer experience. Can I suggest that instead a fields prop that either takes a string or an object of key value pairs where you map a form property to the group property, could we instead have a group prop and we pass the actual form object to a group prop or map the destination property to the form property directly? That could be more intuitive since we pass the form prop to the withForm hook and this would feel similar in experience. I don't know the feasibility with that and maintaining type safety, though. |
@MPiland could you provide a code snippet of what that structure may look like? It doesn't need to be feasible, just a snippet of what you envisioned the API to be |
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
|
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: