Form component

[bytasv]

Closes #749

Drag-and-drop form component.

• Supports the 4 input types we have so far: TextField, Select, DatePicker and FilePicker
• Allow for submitting inputs as a group with custom field names
• Allow for resetting form values
• Supports default values
• Allows for validation according to input type
• Individual inputs outside forms can also validate and display errors
• Add prop categories (in this case, "validation" props)
• Customizable form controls out of the box. But form controls can also be hidden and the Button component can be used instead with button, reset and submit as options for button type

For automatically generating a form from external data I've created this separate issue: #1770

Screen.Recording.2023-03-21.at.17.42.39.mov

Screen.Recording.2023-03-21.at.17.43.35.mov

[Janpot]

From a high level review, (I didn't go in depth yet), I see potentially two problems coming up:

1. Ideally this works automatically with the component name, but component names are global and unique. This is another case where we need local scope for a feature to work properly. a local scope would allow us to hide those form controls from the global scope, and allow them to have a name that's unique to the form, not to the page.
2. What happens when you add a TextField to the page? Where there is no form context available? My initial hunch would be that we shouldn't touch the individual components, but rather the form component should wrap its children.

[bytasv]

• Ideally this works automatically with the component name, but component names are global and unique. This is another case where we need local scope for a feature to work properly. a local scope would allow us to hide those form controls from the global scope, and allow them to have a name that's unique to the form, not to the page.

What you are describe sounds more like a general problem that Toolpad has and not something that's related to form or this PR. So while I agree in theory it can bring problems, I wouldn't consider this topic to be part of this task/PR

• What happens when you add a TextField to the page? Where there is no form context available? My initial hunch would be that we shouldn't touch the individual components, but rather the form component should wrap its children.

It breaks now, but I will make sure that it does not when there is not context (or just add a global context). However as per your alternative suggestion - I did try to go that way where form only wraps regular components and detects it's changes, but it creates an issue of binding back data or controlling data from the form itself UNLESS we want to control data via hooking into our bindings/dom data model? Though still we would need way to "register" form fields somehow and current implementation basically solves all that

[Janpot]

What you are describe sounds more like a general problem that Toolpad has and not something that's related to form or this PR.

What I'm describing is a limitation of the current code base that needs to be refactored if we want to implement features like List/Form in a way that is not "hacking it in to make it look like it mostly works, covering up all the nasty side-effects". Which is certainly possible, but also it's just passing on the hot potato to the person that will handle the tickets that inevitably come in for this feature.

[bytasv]

What I'm describing is a limitation of the current code base that needs to be refactored if we want to implement features like List/Form

No it doesn't require refactoring if we want to implement the base requirements that we have today

in a way that is not "hacking it in to make it look like it mostly works, covering up all the nasty side-effects"

Working around certain limitations that are imposed on architectural decisions made in the past is not necessarily a hack. Improving code by knowing more requirements is a natural way of evolving codebase and it doesn't really matter who's the person going to be who "inevitably come in for this feature". The more important thing is to understand whether we (as a team) even want or need to solve what you call "nasty side-effects" at all, as it's also possible that nobody will even care about it until certain point of time, where it's more likely that someone (at least those who upvoted original feature) will care for not having a feature they requested.

[bytasv]

After trying external form libs work to support form component I decided to throw all that away and start from scratch. IMO this should simplify how we deal with fields that have changing values, without having to decorate them with extra logic.

@Janpot this is not ready for review, but I want to double check before proceeding whether this could be a good approach or if you have an alternative suggestion how to approach it?

1. Form becomes just a wrapper without any extra children or any logic (so far)
2. When onChangeHandler is triggered (assuming this is any component that sets value and that we would like to access via form) we check if component is placed inside form (getClosestForm). If we find a closest form then together with field change handler we set controlled binding for the form value grabbing current value and altering the value that was just changed

The issue that I'm having right now is - even if I configure form like this:

    value: {
      visible: false,
      typeDef: { type: 'object', default: {} },
      onChangeProp: 'onChange',
    },

for some reason closestForm.props.value is not defined initially (I'm expecting it would default to {}?).
I'd appreciate help to better understand how the data flows and what am I missing to make this work correctly (if this approach makes sense)

Apart from that I'm able to capture value changes and use it as form value.

CleanShot.2023-02-08.at.15.13.27.mp4

Any feedback on this approach?

[apedroferreira]

Not sure what the specific issue is for the missing default value - you'd probably have to look into the bindings logic in the runtime to figure it out - there's a useEffect there that handles default values (src/runtime/ToolpadApp, around line 349?). At least this is where i would look first...

There probably are some similarities between what you need to do here and what I did for the List component in #1527 (looking for a review there btw to see if my latest method sounds good? Also maybe it would be helpful if we merged the List first.).
What I did there was to create a local scope inside the list component by making it set a context, on the runtime (src/runtime/ToolpadApp) that wraps all of it's children. In the case of a form, this context could probably hold the form state.
The context could also have an onChange handler that every child of it could use to change values.
Anyway that's just a generic idea of it could work, without having gotten into the details, so it could be wrong about some things or not work well for this particular case.

[bytasv]

Also maybe it would be helpful if we merged the List first.

For sure, this is no where near being ready to be merged anyways so I'd need to adjust accordingly after List is merged.

The context could also have an onChange handler that every child of it could use to change values.

Could you please clarify what you mean by that? Currently it's implemented in a way that child components wouldn't have to know anything about form component

[Janpot]

How I would go about it without doing low-level work to toolpad to enable defining arbitrary scopes.

1. create shared FormValue and SetFormValue contexts to be used by both the Form and individual components. These contexts are provided by the Form component to provide its value and onChange prop downstream:

   <Paper>
     <FormValue.Provider value={value}>
       <SetFormValue.Provider
         value={(field, newValue) => onChange({ ...value, [field]: newValue })}
       >
         {children}
       </SetFormValue.Provider>
     </FormValue.Provider>
   </Paper>

   In the form components we use these contexts, and if they exist we override the value and onChange from the props:

   const formValue = React.useContext(FormValue);
   const setFormValue = React.useContext(SetFormValue);
   inveriant(!!formValue === !!setFormValue, '...');
   
   const inputValue = formValue ? formValue[name] : valueProp;
   const setInput = setFormValue ? (newValue) => setFormValue(name, newValue) : onChangeProp;
   
   return <TextField value={input} onChange={setInput} />;

2. If the above works, we have the basis, but we still need to filter out the components that are bound to a form in the bindings. The second part of 1. will be shared by every form-capable component, so we can extract it and put it in the runtime. e.g. we can add some definition to ToolpadComponentConfig like:

   argTypes: {
     /* ... */
   },
   formValue: ['value', 'onChange']

   In the runtime we can check for existence of this config when rendering the component and do the logic of 1. automatically, when preparing its properties.
   Then we can also read it while setting up the bindings and omit these properties from creating scope variables (so just remove scopePath for those)

   This is a two part explanation, the end result is 2. witha clean global scope, but try 1. first to verify that the reasoning works.

[apedroferreira]

@Janpot 's idea sounds good to me!

Could you please clarify what you mean by that? Currently it's implemented in a way that child components wouldn't have to know anything about form component

In this case this onChange handler would probably need to be passed as a prop to each component, or some other method with the same result.

I guess one of the main advantages of using context would be that you wouldn't have to "search" for the closest form manually.
As for including form logic inside components themselves, I'm fine with that if we decide it's the easiest/best way to go about it.

[bytasv]

Thanks for through explanation, it seems that this would be similar to the first approach that I tried (except that it used external form lib, which is not necessary).

1. However I'm wondering - is it necessary to decorate each "form-capable" component with extra information such as formValue: ['value', 'onChange'] if I understand correctly this should be pretty much the same for each component?

2. Am I missing something important or could we actually do the same without decorating components with special configuration and using approach similar to what this "new implementation" is doing - using getClosestForm determine if we should create global scope or not and how the value should be set. In such case if component is part of form, then it would always set value on the form values, and also read it from there, but all of that could happen without component knowing about it (unless that's the part that is not possible)?

So I'm just trying to understand if I'm missing something which would make current approach incompatible with what you defined

Also, can you confirm if I understand correctly what is meant by "omit properties from creating scope":

• If form-capabale component is part of form it would not be accessible from a global scope, like below? 👇

Thank you

[apedroferreira]

@Janpot let me know if you have any more feedback since your previous comments (see above), otherwise we can probably merge this PR.

[Janpot]

I feel like this feature could use an integration test

[apedroferreira]

I feel like this feature could use an integration test

I was gonna do it separately but I'll add it before we merge this then, even if in a separate PR.

[Janpot]

Either way is fine for me, I just don't feel like this feature shouldn't be covered at all

[apedroferreira]

I'll merge this soon if there's no more feedback, will add the integration test separately right after.

[apedroferreira]

The text field stopped being interactive in forms in my last change...
I've reverted, will merge again when I have the test ready.