Skip to content

Form 292#293

Closed
mikemfleming wants to merge 7 commits into
cision:masterfrom
mikemfleming:FORM-292
Closed

Form 292#293
mikemfleming wants to merge 7 commits into
cision:masterfrom
mikemfleming:FORM-292

Conversation

@mikemfleming
Copy link
Copy Markdown
Contributor

@mikemfleming mikemfleming commented Dec 16, 2020

What's in this pr?

This resolves #292 by adding a Form component that provides state management helpers. It's my first real pr to rover-ui so be brutal!

☝ Concerns

What I'm proposing is a home brewed state management system. Would it be better to introduce a lib that's dedicated to the task?

Formik is a really good one IMO but if this is sufficient then we can leave it at that.

Comment thread example/src/App.js
Comment thread src/components/Form/Form.tsx Outdated
Comment thread src/components/Form/Form.tsx Outdated
Comment thread src/components/Form/Form.tsx Outdated
Comment thread src/components/Form/Form.tsx Outdated
validationSchema={{
nameInput: {
message: 'No empty strings',
validate: (value) => value !== '',
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could the validate function return a string for the message, or falsey if no error?
Something like validate: value => value === '' && 'Required'

Maybe that wouldn't be as legible, but it would allow for the user to provide different messages for different error conditions.

I've also seen validation configs with multiple keys for different validation errors, like "invalid email", "required", "too long", etc. If you want to explore that, the component should probably return an array of error keys and let the consumer handle building a final, user-friendly string in a target language.

{
  nameInput: {
    required: {
      message: 'No empty strings',
      validate: value => value !== '',
    }
  }
}

'Overview',
() => (
<Form
initialValues={{ nameInput: 'james r. cat' }}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a thought: What should happen if no initial values are provided? Might there be a good way to default to "empty" values?
I guess this could be tough if we don't know up front whether an input will be text, radio, etc.

Comment thread src/components/Form/story.tsx Outdated
<Form
initialValues={{ nameInput: 'james r. cat' }}
// eslint-disable-next-line no-alert
onSubmit={(formValues) => alert(JSON.stringify(formValues))}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is where, if no submit handler is provided, I think falling back to default form behavior would make sense.

Comment thread src/components/Form/story.tsx Outdated
formValues,
handleChange,
handleBlur,
handleCustom,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name handleCustom is confusing to me. Is it a change handler, blur handler, submit handler?
Maybe something along the lines of processFieldValue?

Would it be better to have it take and update the entire form state? I'm thinking of cases where fields would depend on each other, like if a user chose "February" and we had to reset or modify a date from 31 to 28.

Comment thread src/components/Form/story.tsx Outdated
<input
type="text"
name="nameInput"
value={formValues.nameInput}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be crazy:
Having the user enter name="foo" and formValues.foo feels redundant.
Would it make sense to iterate the children/dom elements and apply values and event handlers that way? I'm not sure it would be feasible or fast, but the interface would sure be nice if you could just add <input type="text" name="myInput" /> and everything else was automatic.

We might be able to do something like that with an EasyInput component and the React Context API.

Copy link
Copy Markdown
Contributor

@pixelbandito pixelbandito Jan 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

<EasyForm initialValues={{
  foo: ""
}}>
  <EasyInput name="foo" type="text" />
</EasyForm>

Comment thread src/components/Form/story.tsx Outdated
}}
>
{({
formState,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you combined some parts of formState and formValues, could you do something like:

<input
  value={form.email.value}
  onChange={form.email.onChange}
  data-touched={form.email.touched}  
  data-error-messages={form.email.errorMessage}  
/>

Or, if you wanted to extract some properties in the function as child argument, you could get as simple as:

<Form ... />
  {
    ({ form: { touched, errorMessage, ...email } }) => {
      <input {...email}  />
    }
  }
</Form>

@pixelbandito
Copy link
Copy Markdown
Contributor

Arg, I've been meaning to get to this, but it's complex enough I want to run it locally. Will do Monday at the latest. Sorry, @wendigolabs!

@pixelbandito
Copy link
Copy Markdown
Contributor

Changes so far look good! I just resolved a handful of conversations.

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 this pull request may close these issues.

4 participants