Implement Terra Forms #305

JakeLaCombe · 2017-05-04T05:37:35Z

Summary

There are the base components used for building forms with Terra.

Additional Details

This is bare bones forms. Features such as validation will come at a later date.

StephenEsser · 2017-05-08T14:13:57Z

packages/terra-form/docs/ChoiceField.md

@@ -0,0 +1,22 @@
+# Choice Field


What was the reasoning for naming this a ChoiceField, as opposed to something like RadioGroup?

I think the main decision was to keep in consistent naming with the textareaField and textField components. I'm not opposed to renaming it radio group though. Perhaps it should be RadioGroupField to indicate that it utilizes a field component as well?

StephenEsser · 2017-05-08T14:17:50Z

packages/terra-form/src/components/ChoiceField.jsx

+
+const propTypes = {
+  /**
+   * Choices to populate radio buttosn for


Typo on buttons

StephenEsser · 2017-05-08T14:24:31Z

packages/terra-form/src/components/ChoiceField.jsx

+const defaultProps = {
+  choices: [],
+  choiceName: 'name',
+  choiceValue: 'value',


From a form submission standpoint I think defaulting these values could be really dangerous, accidentally submitting incorrect information to a server or selecting unintentional fields as the default.

Edit: I might be confusing choiceName / choiceValue from name / value

It looks like these define the key value pairs in the choices prop hash array, this feels like a really confusing API.

correct. the choiceName is the label of the input, and choiceValue is the value of the input. I like the idea you posted down with the children. I'll see what other developers think of it as well

what about nameKey and valueKey ?

I don't know if that's better though. I see a need to pass in additional attributes to the individual nodes as well. I think we should just require the same api structure as that of the actual choice input, meaning we just rely on the name and value attributes straight up. Otherwise, this api will be very confusing for users to consume.

bjankord · 2017-05-08T14:27:15Z

packages/terra-form/package.json

+  },
+  "peerDependencies": {
+    "react": "^15.4.2",
+    "react-dom": "^15.4.2"


Need to add "terra-base": "^0.x", to peerDependencies and dependencies.

StephenEsser · 2017-05-08T15:06:12Z

packages/terra-form/src/components/ChoiceField.jsx

+  /**
+   * Choices to populate radio buttosn for
+   */
+  choices: PropTypes.array,


I believe Brett called out on the original Tech Design that this prop might make more sense being called options.

I personally don't think we should use a prop API that consists of an Array of hashes with key value pairs. The React form libraries I've worked with use children and I think it makes it a lot easier for the consumer.

<RadioGroup onChange={this.onChange} value={this.state.value}> <Radio value={1}>A</Radio> <Radio value={2}>B</Radio> <Radio value={3}>C</Radio> <Radio value={4}>D</Radio> </RadioGroup>

https://ant.design/components/radio/#components-radio-demo-radiogroup

+1 to that idea. I think that looks more clear than passing in choiceName and choiceValue as well.

The React form libraries I've worked with use children and I think it makes it a lot easier for the consumer

I think this depends on if you are dealing with static choices that are coded or dynamic choices populated from the server.

These options aren't mutually exclusive. Technically what you're describing can already be done use the current design.. ChoiceField is just a higher order component that does that for you. It prevents consumers from having to do the boilerplate code of listing our the radio fields or looping over them and creating them.

I wish these conversations would have happened on the design as it would have been easier to follow... but the Field components are convenience molecules to make it easy to create the proper inputs to capture the proper data. Nothing prevents anyone from generating forms in a style that leaks the implementation details of HTML

Ultimately, OCS from what I hear is going to specify rules that dictate when you use radios/checkboxes vs select vs search. That will rule out using a static approach of rendering radios such as this because it should flex depending on how many choices there are.

I propose enable what you're suggesting, but also create choiceField which will be the abstraction to flip impl between select and radios depending on how many choices there are.

@mhemesath

I've added this topic to the War Room agenda, will you be able to attend on Friday?

War room discussion output was that we create the <RadioGroup /> component that uses children API and a <ChoiceField /> that uses a hash API.

StephenEsser · 2017-05-08T15:13:02Z

packages/terra-form/docs/ChoiceField.md

+  error="This field is required"
+  help="State Locations help determine what campus to place you at"
+  required={true}
+  attrs={{ className: 'healtheintent-application' }}


Looking at the ChoiceField.jsx below I don't think attrs is actually used in this component.

StephenEsser · 2017-05-08T15:25:05Z

packages/terra-form/src/components/ChoiceField.jsx

@@ -0,0 +1,103 @@
+/* eslint-disable jsx-a11y/label-has-for */


What is the error being thrown for on this?

StephenEsser · 2017-05-08T15:27:10Z

packages/terra-form/src/components/Control.jsx

+  /**
+   * Additional attributes for the input object
+   */
+  attrs: PropTypes.object,


Does anyone have strong opinions on this? Something like inputAttributes seems more clear to me

I think some of us agreed to switch it to that. I'll make the change

inputAttributes seems extremely verbose.... FWIW, this follows the pattern of what Django does which uses attrs and has worked there. I'd rather have to check the documentation once, learn what it is and then use that going forward over having to type inputAttributes every time I set this...

https://docs.djangoproject.com/en/1.11/ref/forms/widgets/#django.forms.Widget.attrs

In the war room, we came to a census for inputAttrs

StephenEsser · 2017-05-08T15:29:52Z

packages/terra-form/src/components/Control.jsx

+   * Initial Value of the input
+   */
+  value: PropTypes.string,
+};


Should there be a first class prop for defaulting the value to be checked?

To that comment, wondering if control would be simplified if it simply had an input property and was explicitly passed the input (rather than attempting to wrap it)

StephenEsser · 2017-05-08T15:39:06Z

packages/terra-form/docs/Field.md

+  value="children_present"
+  error="This field is required"
+  help="Families are eligible for family package plans"
+  required={true}


Does the Field accept the required prop?

StephenEsser · 2017-05-08T15:45:06Z

packages/terra-form/src/components/Input.jsx

+  /**
+   * Functional to be called when the input is changed.
+   */
+  onChange: PropTypes.func,


I think the description meant to have function

not sure its worth calling this out but we can. Technically other events are needed like keydown, focus, etc. But they are all available to be set via custom props

In my opinion, I think we should keep just onChange as first class, as that's the most critical one for dealing with controlled inputs. There are several others we could add, but that could make this list very large.

StephenEsser · 2017-05-08T15:59:13Z

packages/terra-form/docs/Input.md

+import { Input } from 'terra-form';
+
+<Input
+  required={false}


Nit: Our Booleans are getting a little awkward. This is kind of up to preference, this value defaults to false, so passing in an explicit false does the same thing. There are also examples in these docs that use required={true} when just passing required in should do the same thing. Do we have a preference or standard on how we recommend using booleans? The two generic paths being:

<Input required />

or

<Input required={true} />

The current prop required also breaks our is prefix, but it matches the html API, so I don't know which one is more user friendly

Our linter prefers us to use the top approach you have when the boolean value is known to be true. Looks like I forgot to update the documentation to reflect the change. I'll get the MD fixes.

As for the is prefix, I feel like the solution is situational. required is a standard html attribute, so for a variable like required, I think keeping it as it would be the ideal approach. For something like isInline, inline isn't a mapped html attribute, so I feel that the prefix would be needed there.

In our early meetings I thought we had discussed not using html attributes as prop names to avoid conflicts, but I could be remembering that conversation incorrectly, it was a while back.

I did see these standards where they wanted to mirror the DOM whenever it was possible. https://github.cerner.com/terra/conventions/blob/master/react.md#mirror-dom-wherever-possible

I'll look through war room agendas to see if we had updates.

It was most likely when there were conflicting props, like using title as a prop to display text, when title would otherwise be spread onto the dom element and perform a different role. When the roles are the same it probably makes more sense to mirror.

StephenEsser · 2017-05-08T16:23:23Z

packages/terra-form/src/components/Textarea.jsx

@@ -0,0 +1,76 @@
+import React, { PropTypes } from 'react';


Is this the file that was causing all the case sensitive issues? Should this be TextArea?

I debated that myself. I mainly stick to Textarea to keep the same naming convention of the html node textarea. I'm not picky on this though, and if you'd like it changed, I have no problem doing so.

bjankord · 2017-05-08T18:01:12Z

packages/terra-form/docs/ChoiceField.md

+<ChoiceField
+  label="What State do you live in?"
+  name="children_present"
+  value="children_present"


I think the values used in the name and value props need updated, they don't seem to make sense in the ChoiceField example.

bjankord · 2017-05-08T18:03:36Z

packages/terra-form/docs/README.md

+
+To use these components, create a React component that contains a form element, and then insert the Terra form components into that form.
+
+The recommended approach for building forms is to stick with the field components that are build for you (ChoiceField, MultiChoiceField, NumberField, Textarea, TextareaField, Field). If you need to create a custom field, the Field, Control, Input, and Textarea components are available for use.


typo: build => built

bjankord · 2017-05-08T18:05:10Z

packages/terra-form/docs/README.md

+
+The recommended approach for building forms is to stick with the field components that are build for you (ChoiceField, MultiChoiceField, NumberField, Textarea, TextareaField, Field). If you need to create a custom field, the Field, Control, Input, and Textarea components are available for use.
+
+When using input fields, React has two ways of using inputs: controlled or uncontrolled. Controlled maps the value of the input to the state of the component, while uncontrolled does not (https://facebook.github.io/react/docs/uncontrolled-components.html). If you would like to use a controlled input (ideal for forms that need to do validations with JavaScript and send ajax requests), provide a value and an onChange function to the component. If you would like to use an uncontrolled input (ideal for forms that do straight backend requests), provide just a defaultValue into the input **Note: If you provide both a value and defaultValue to the input, React will be confused as to whether you are working with an uncontrolled input or a controlled one. Provide either a value or a defaultValue to the input, but not both.**


Let's link the first reference of controlled in this paragraph to https://facebook.github.io/react/docs/forms.html#controlled-components and link the first reference on uncontrolled to https://facebook.github.io/react/docs/uncontrolled-components.html

nit: drop the word just in this phrase: provide just a defaultValue into => provide a defaultValue into

… inputs

mhemesath · 2017-05-09T13:24:17Z

packages/terra-form/src/Form.scss

+  border-radius: 0.25em;
+  font-size: 16px;
+  line-height: 1.428em;
+  padding: 0.178em;


can these reference variables ?

mhemesath · 2017-05-09T13:26:15Z

packages/terra-form/src/components/ChoiceField.jsx

+const defaultProps = {
+  choices: [],
+  choiceName: 'name',
+  choiceValue: 'value',


what about nameKey and valueKey ?

mhemesath · 2017-05-09T13:34:23Z

packages/terra-form/src/components/Control.jsx

+   * Initial Value of the input
+   */
+  value: PropTypes.string,
+};


To that comment, wondering if control would be simplified if it simply had an input property and was explicitly passed the input (rather than attempting to wrap it)

mhemesath · 2017-05-09T13:38:18Z

packages/terra-form/src/components/Input.jsx

+  /**
+   * Functional to be called when the input is changed.
+   */
+  onChange: PropTypes.func,


not sure its worth calling this out but we can. Technically other events are needed like keydown, focus, etc. But they are all available to be set via custom props

emilyrohrbough · 2017-05-12T13:19:15Z

packages/terra-form/README.md

+[![NPM version](http://img.shields.io/npm/v/terra-form.svg)](https://www.npmjs.org/package/terra-form)
+[![Build Status](https://travis-ci.org/cerner/terra-core.svg?branch=master)](https://travis-ci.org/cerner/terra-core)
+
+Components for building forms


Should we have a more detailed description of what components are available for building a form? I see the other read me doc goes into more depth into it.

In my opinion, we should provide links to the packages that this module contains as opposed to describing them here. Otherwise, we could risk this page getting rather large when folders like validations are later included.

emilyrohrbough · 2017-05-12T13:35:12Z

packages/terra-form/package.json

+  },
+  "scripts": {
+    "compile": "npm run compile:clean && npm run compile:build",
+    "compile:clean": "rm -rf lib",


"compile:clean": "$(cd ..; npm bin)/rimraf lib",

emilyrohrbough · 2017-05-12T13:40:36Z

packages/terra-form/src/components/ChoiceField.jsx

@@ -0,0 +1,106 @@
+/* eslint-disable jsx-a11y/label-has-for */
+
+import React, { PropTypes } from 'react';


Once Matt's PR for React 15.5 compatibility is merged in, PropTypes will need imported from the prop-types package. Should this be done before his is merged in so there are less changes in the future?

emilyrohrbough · 2017-05-12T14:00:49Z

packages/terra-form/src/components/Input.jsx

+const defaultProps = {
+  defaultValue: undefined,
+  name: null,
+  onChange: () => {},


We are inconsistent between packages on how to handle default onChange/onClick/onKeyDown functions. Button has no default defined, ActionHeader defaults to null, Table & List defaults to undefined, and here it defaults to an empty function. Do we need to use a standard?

Let's bring that up on the war room as well today

why is onChange defaulted?

We have the need to support both controlled and uncontrolled inputs, so we can't always expect the developer to provide an onChange function.

One option is to just put it on the inputAttrs attribute for forms, and delegate it that way to the input element.

emilyrohrbough · 2017-05-12T14:07:41Z

packages/terra-form/src/components/MultiChoiceField.jsx

+// eslint-disable react/jsx-boolean-value
+
+import React, { PropTypes } from 'react';
+


Missing import 'terra-base/lib/baseStyles';

Or did you not include it because its using the Field Component? If that is the case, then you added import 'terra-base/lib/baseStyles' to the ChoiceFeild component

StephenEsser · 2017-05-12T14:21:36Z

packages/terra-form/docs/components/MultiChoiceField.md

+  choices=[{ name: 'C', value: 'C' }, { name: 'java', value: 'Java' }]
+  isInline={false}
+  required={false}
+/>


Is it possible to pre select choices with this API?

I feel like the original tech design called in for passing those in through a values array, but I don't feel like that's the best API. The place that would belong most likely is on this hash

@StephenEsser thats what the values is for.

emilyrohrbough · 2017-05-12T14:25:41Z

Will there be any css styling on ChoiceField, MultiChoiceField, NumberField,TextField, or TextAreaField? There was nothing specific to these components in Field.scss so just wondering how that worked or if they weren't special.

emilyrohrbough · 2017-05-12T15:08:15Z

packages/terra-site/src/TestLinks.jsx

@@ -11,6 +11,7 @@ const TestLinks = () => (
      <li><Link to="/tests/button-tests">Button Tests</Link></li>
      <li><Link to="/tests/button-group-tests">Button Group Tests</Link></li>
      <li><Link to="/tests/date-picker-tests">DatePicker Tests</Link></li>
+      <li><Link to="/tests/form-tests">DatePicker Tests</Link></li>


Oops, you forgot to change this to Form Tests!

emilyrohrbough · 2017-05-12T15:15:45Z

After looking at the site examples, it appears the consumer needs to declare <form> before using the form components.

Would it be reasonable to create a wrapper component called Form that the user can drop these form components into as children?Then each component could be aliased via the Form component so the consumer would only need to import terra-form and use dot notation.

JakeLaCombe · 2017-05-12T15:36:15Z

Terra form actually will not be providing a base component. Consumers would have to provide the form node theirselves, and then use the components listed here to build that form. This is similar to the approach react-form uses. You can view more details here. https://jira2.cerner.com/browse/TERRAUI-573?focusedCommentId=2634665&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-2634665

JakeLaCombe · 2017-05-12T15:38:27Z

@emilyrohrbough The styling for components such as TextField and NumberField will come from the base field. They will use the styles that a Field uses for it's components.

Discussed in War Room.

JakeLaCombe · 2017-05-15T14:48:56Z

This pull request is too lengthy. I'll split up the forms into multiple pull requests.

Jake LaCombe and others added 3 commits May 4, 2017 00:35

Implement Terra Forms

9e12da5

Fix issue with file names on travis build

06346fd

Merge branch 'master' into Terra-Forms

202542b

bjankord requested review from mhemesath, mjhenkes, tbiethman, ryanthemanuel and benbcai May 5, 2017 20:16

bjankord temporarily deployed to terra-core-feature-pr-305 May 5, 2017 20:20 Inactive

bjankord temporarily deployed to terra-core-deployed-pr-305 May 5, 2017 20:40 Inactive

bjankord temporarily deployed to terra-core-deployed-pr-305 May 5, 2017 20:48 Inactive

bjankord added the form label May 8, 2017

Merge branch 'master' into Terra-Forms

1c8948d

bjankord temporarily deployed to terra-core-deployed-pr-305 May 8, 2017 13:55 Inactive

StephenEsser reviewed May 8, 2017

View reviewed changes

bjankord reviewed May 8, 2017

View reviewed changes

StephenEsser reviewed May 8, 2017

View reviewed changes

bjankord reviewed May 8, 2017

View reviewed changes

bjankord temporarily deployed to terra-core-deployed-pr-305 May 9, 2017 19:41 Inactive

Use mixin for the end margins for inline components

ad5b732

bjankord temporarily deployed to terra-core-deployed-pr-305 May 9, 2017 19:51 Inactive

Convert px to em

ea8cbab

bjankord temporarily deployed to terra-core-deployed-pr-305 May 9, 2017 19:58 Inactive

Update the documentation to be consistent between default and regular…

8e50914

… inputs

bjankord temporarily deployed to terra-core-deployed-pr-305 May 9, 2017 20:22 Inactive

Merge branch 'master' into Terra-Forms

0815b45

bjankord temporarily deployed to terra-core-deployed-pr-305 May 10, 2017 19:11 Inactive

mhemesath reviewed May 10, 2017

View reviewed changes

Add required class and prop for fields

14a9635

bjankord temporarily deployed to terra-core-deployed-pr-305 May 11, 2017 16:06 Inactive

emilyrohrbough reviewed May 12, 2017

View reviewed changes

emilyrohrbough previously requested changes May 12, 2017

View reviewed changes

emilyrohrbough reviewed May 12, 2017

View reviewed changes

StephenEsser reviewed May 12, 2017

View reviewed changes

emilyrohrbough reviewed May 12, 2017

View reviewed changes

Add Fieldset and Radiogroups

2e3eb0c

bjankord deployed to terra-core-deployed-pr-305 May 14, 2017 20:30 View deployment

JakeLaCombe closed this May 15, 2017

JakeLaCombe deleted the Terra-Forms branch July 31, 2017 21:38

StephenEsser mentioned this pull request Jul 9, 2019

Create Terra Dropdown Button #2487

Merged

		@@ -0,0 +1,103 @@
		/* eslint-disable jsx-a11y/label-has-for */


		To use these components, create a React component that contains a form element, and then insert the Terra form components into that form.

		The recommended approach for building forms is to stick with the field components that are build for you (ChoiceField, MultiChoiceField, NumberField, Textarea, TextareaField, Field). If you need to create a custom field, the Field, Control, Input, and Textarea components are available for use.


		The recommended approach for building forms is to stick with the field components that are build for you (ChoiceField, MultiChoiceField, NumberField, Textarea, TextareaField, Field). If you need to create a custom field, the Field, Control, Input, and Textarea components are available for use.

		When using input fields, React has two ways of using inputs: controlled or uncontrolled. Controlled maps the value of the input to the state of the component, while uncontrolled does not (https://facebook.github.io/react/docs/uncontrolled-components.html). If you would like to use a controlled input (ideal for forms that need to do validations with JavaScript and send ajax requests), provide a value and an onChange function to the component. If you would like to use an uncontrolled input (ideal for forms that do straight backend requests), provide just a defaultValue into the input Note: If you provide both a value and defaultValue to the input, React will be confused as to whether you are working with an uncontrolled input or a controlled one. Provide either a value or a defaultValue to the input, but not both.

		@@ -0,0 +1,106 @@
		/* eslint-disable jsx-a11y/label-has-for */

		import React, { PropTypes } from 'react';

		// eslint-disable react/jsx-boolean-value

		import React, { PropTypes } from 'react';

Implement Terra Forms #305

Implement Terra Forms #305

Conversation

JakeLaCombe commented May 4, 2017

Summary

Additional Details

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StephenEsser May 8, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StephenEsser May 8, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StephenEsser May 8, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StephenEsser May 8, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StephenEsser May 8, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bjankord May 8, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emilyrohrbough May 12, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emilyrohrbough commented May 12, 2017

Choose a reason for hiding this comment

emilyrohrbough commented May 12, 2017

JakeLaCombe commented May 12, 2017

JakeLaCombe commented May 12, 2017

JakeLaCombe commented May 15, 2017

StephenEsser May 8, 2017 •

edited

StephenEsser May 8, 2017 •

edited

StephenEsser May 8, 2017 •

edited

StephenEsser May 8, 2017 •

edited

StephenEsser May 8, 2017 •

edited

bjankord May 8, 2017 •

edited

emilyrohrbough May 12, 2017 •

edited