Skip to content
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

Form #800

Closed
Tracked by #780
tomdye opened this issue Sep 24, 2019 · 6 comments · Fixed by #807
Closed
Tracked by #780

Form #800

tomdye opened this issue Sep 24, 2019 · 6 comments · Fixed by #807
Assignees

Comments

@tomdye
Copy link
Member

tomdye commented Sep 24, 2019

We should create a Form pattern that can be used to easily set and get values from forms.
It should handle things like dirty state and validation.
This could be implemented as a widget or as a middleware that can be used in applications.

  • The Form should use the name / setValue / getValue properties of the form fields created within it.
  • The Form should handle the dirty state of it's fields
  • The form should handle field validation
  • The form should have it's own onValue callback which reports back the form value.
@tomdye tomdye mentioned this issue Sep 24, 2019
47 tasks
@KaneFreeman KaneFreeman self-assigned this Oct 1, 2019
@KaneFreeman
Copy link
Member

Based on an initial version from @matt-gadd, I have put together a poc for the form middleware. We could also create a similar meta to allow it to be used with class based widgets.

POC: https://codesandbox.io/s/github/KaneFreeman/form-middleware-poc/tree/master/

@KaneFreeman
Copy link
Member

POC updated with meta

@samends
Copy link

samends commented Oct 8, 2019

I think it looks good Dan! What your missing in your POC is a dirty state. Also it could be helpful to add a method that would take an object so that you can set multiple form states at once. Something like:

	function getter(name?: string): ReturnType<typeof singleField>;
	function getter<K extends Record<string, any>>(
		defaults: K
	): Record<keyof K, ReturnType<typeof singleField>>;
	function getter<K extends Record<string, any>>(name?: string | K) {
		if (typeof name === 'object') {
			return Object.keys(name).reduce(
				(result, key) => {
					const field = singleField(key);

					if (field.value() === undefined) {
						field.value(name[key]);
					}

					return {
						...result,
						[key]: field
					};
				},
				{} as Record<keyof K, ReturnType<typeof singleField>>
			);
		} else {
			return singleField(name);
		}
	}

	return getter;

So that it could be used like this

const { arrivalDate, arrivalTime, facility, committedBy, bookingOfficer } = form({
		arrivalDate: '',
		arrivalTime: '',
		facility: '',
		committedBy: '',
		bookingOfficer: ''
	});

Would you want to put in a PR for your POC?

@matt-gadd
Copy link
Contributor

matt-gadd commented Oct 8, 2019

I think there is some strong overlap here conceptually with #803, in terms of a group of fields. It would be nice to align the patterns, for instance the CheckboxGroup exposes a widget that wraps the middleware (which makes it nicer to use than just the middleware and would negate us having to implement a meta for legacy reasons too). It also explores using the widget uncontrolled, which might also be useful on the form.

@KaneFreeman
Copy link
Member

What your missing in your POC is a dirty state.

A lot of the form widgets handle their own dirty state internally. Example: https://github.com/dojo/widgets/blob/master/src/text-input/index.ts#L27

To handle this at the form level we would need to expose dirty as a property and force the user to handle it/hook it up to the form. I am not against this idea as it would allow for a proper form reset, but not sure that is the path we want to take.

Would you want to put in a PR for your POC

Will be doing so soon. Working on a few other items with it. Also have tests still to write, but wanted some feedback before i dove into those too much.

@KaneFreeman
Copy link
Member

Also it could be helpful to add a method that would take an object so that you can set multiple form states at once.

The POC has been updated with this (using it to fill the form). Based on discussion with @matt-gadd removed the Meta and added a Form widget wrapper for the middleware similar to #803.

https://codesandbox.io/s/github/KaneFreeman/form-middleware-poc/tree/master/

I'll work on getting a PR opened up for this, then start in on the tests.

@KaneFreeman KaneFreeman mentioned this issue Oct 10, 2019
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants