Update docs for definitions #236

bernars-usa · 2018-08-27T14:29:56Z

Closes #231, which is related to #225.

This PR:

Updates the conceptual information about definitions
Updates the individual descriptions of the definitions that were changed in Make definitions consistent #225 (everything except Autocomplete and File Upload)
Updates the code sample for how to call the definitions based on the sample form.js in https://github.com/usds/us-forms-system-starter-app/blob/test-new-schema-additions-to-usfs/js/config/form.js
Adds images of each field to show how they're rendered

This needs a thorough review of the code samples, as well as the descriptions of the definitions.

bernars-usa · 2018-08-27T14:42:30Z

docs/building-a-form/common-definitions.md

 ```

-Then, call it to add all the `uiSchema` definitions. In this example, the definition is a function that takes the title for that field, which is used to populate the 'ui:title' property in uiSchema:
+Then, call it to add the `schema` and `uiSchema` functions. The definition is a function that takes the title for that field, which is used to populate the 'ui:title' property in uiSchema:


🤔 I can't remember making this change... it might have been a half implementation of another thought. Let me know if it needs changing.

annekainicUSDS

Good first pass! A few comments for edits.

annekainicUSDS · 2018-08-27T14:37:21Z

docs/building-a-form/common-definitions.md


 ### Using definitions

 To use a definition, import it near the top of the file you want to reference it in:

 ```js
-import currencyUI from 'us-forms-system/lib/js/definitions/currency';
+import currencyConfig from 'us-forms-system/lib/js/definitions/currency';


This needs to be imported using curly brackets now: import { currencyConfig } from .... The reason is because before we were using default exports in the definitions file, and now we're not (basically changing how we export/import things).

annekainicUSDS · 2018-08-27T14:41:29Z

docs/building-a-form/common-definitions.md

 ```

-Then, call it to add all the `uiSchema` definitions. In this example, the definition is a function that takes the title for that field, which is used to populate the 'ui:title' property in uiSchema:
+Then, call it to add the `schema` and `uiSchema` functions. The definition is a function that takes the title for that field, which is used to populate the 'ui:title' property in uiSchema:


I think we need to change this language a little bit. It's still pretty specific to uiSchema. Here's my recommended edit:

Then, use the definition by calling the schema and uiSchema functions on it where needed. Sometimes the functions take arguments to configure how the definitions are used. In this particular case, the uiSchema for currency takes an optional argument for the title of the field. In this example we want to set the title to "Currency", so we pass that as a string to the uiSchema function.

annekainicUSDS · 2018-08-27T14:43:07Z

docs/building-a-form/common-definitions.md

@@ -45,22 +47,25 @@ Available definitions are:
 ### Autosuggest

 A common type-ahead widget that lets a user type in values and narrow down a longer list of options. It is most commonly used with an `enum` of the available options as shown here. Define the uiSchema by calling the function that you import. You can pass an object with additional uiSchema properties.
+
 ```js
 import { uiSchema as autosuggestUI } from 'us-forms-system/lib/js/definitions/autosuggest';


This import statement needs to change to import { autosuggestConfig } from 'us-forms-system/lib/js/definitions/autosuggest';.

annekainicUSDS · 2018-08-27T14:47:22Z

docs/building-a-form/common-definitions.md

-        'New York',
-        'Chicago'
-      ]
+function schema() {


I think what these code samples are trying to show is what this would look like in the form config, not what the definition code is. So I think this would actually remain the way it was before, where the schema is an object in the form config. You can just revert this change for the schema.

This particular definition is a little confusing because here we're not actually using the schema from the definition, but instead writing our own schema into the form config, so I think we should clean that up as a separate issue. I'll create a ticket for that.

annekainicUSDS · 2018-08-27T14:49:14Z

docs/building-a-form/common-definitions.md

+          'New York',
+          'Chicago'
+        ]
+      }
    }
  }
 },


The usage for the uiSchema below also needs to change, where instead it would look like this:

uiSchema: { officeLocation: autosuggestConfig.uiSchema( ... ) }

(what is contained in the ... is exactly as it is now (lines 74-81), it's just where the uiSchema function is called that needs to change)

annekainicUSDS · 2018-08-27T14:50:12Z

docs/building-a-form/common-definitions.md

@@ -81,63 +86,105 @@ Source: [/src/js/definitions/autosuggest.js](../../src/js/definitions/autosugges

 ### Currency

-Formats and validates a US currency field. The display includes a leading `$` character. Call this exported function and pass it the label to be used on the field.
+Formats and validates a US currency field that includes a leading `$` character. You can pass this function a label to be used on the field.


I would change the language in the second sentence to be: "You can pass the uiSchema function a label to be used on the field".

annekainicUSDS · 2018-08-27T14:51:18Z

docs/building-a-form/common-definitions.md

 }
 ```
 Source: [/src/js/definitions/currency.js](../../src/js/definitions/currency.js)

 ### Current or past dates

-The common date field with current or past validation set (i.e., dates in the future are not valid). Call this exported function and pass it the label to be used on the field.
+The common date field with validation warning that dates in the past or future are not valid. You can pass this function a label to be used on the field. Dates must be on or before January 1, 1900.


Anywhere where it says "You can pass this function a label", I would specify to be "You can pass the uiSchema function a label" because there are now 2 functions that are exported by a definition: the schema() and the uiSchema(). It's important to specify which of the two takes the additional argument.

annekainicUSDS · 2018-08-27T14:52:04Z

docs/building-a-form/common-definitions.md


 uiSchema: {
-  birthdate: currentOrPastDate('Date of Birth')
+  currentOrPastDate: currentOrPastDateConfig.uiSchema()


Do we want to show how this takes an additional argument for the label like we used to? So:

currentOrPastDate: currentOrPastDateConfig.uiSchema('Date of Birth')

I'd keep the birthdate as well since it's a good example of when you might want a date not in the future (assuming the person filling out the form is giving their own birthdate 🤔).

Ah yes, good point @dmethvin-gov!

annekainicUSDS · 2018-08-27T14:52:25Z

docs/building-a-form/common-definitions.md


 uiSchema: {
-  lastContact: currentOrPastMonthYear('Last Contact')
+  currentOrPastMonthYear: currentOrPastMonthYearConfig.uiSchema()


Same thing about showing the label that can be passed to it

The lastContact was similarly trying to give an example of when you might want this type of validated date.

dmethvin-gov · 2018-08-27T16:24:43Z

docs/building-a-form/common-definitions.md


 uiSchema: {
-  birthdate: currentOrPastDate('Date of Birth')
+  currentOrPastDate: currentOrPastDateConfig.uiSchema()


I'd keep the birthdate as well since it's a good example of when you might want a date not in the future (assuming the person filling out the form is giving their own birthdate 🤔).

dmethvin-gov · 2018-08-27T16:25:32Z

docs/building-a-form/common-definitions.md


 uiSchema: {
-  lastContact: currentOrPastMonthYear('Last Contact')
+  currentOrPastMonthYear: currentOrPastMonthYearConfig.uiSchema()


The lastContact was similarly trying to give an example of when you might want this type of validated date.

I added some example labels in the `uiSchemas` - let me know what you think.

annekainicUSDS

A few small nits to clean up! Thanks for taking this on, I know this was probably hard to do because it's so technical and nit-picky!

annekainicUSDS · 2018-08-27T19:19:58Z

docs/building-a-form/common-definitions.md

@@ -61,83 +66,119 @@ schema: {
        'New York',
        'Chicago'
      ]
+      }


I think this one last } should be deleted!

It didn't look right but I couldn't figure out how to check it. Thanks!

annekainicUSDS · 2018-08-27T19:21:59Z

docs/building-a-form/common-definitions.md

-      }
-    }
+  officeLocation: autosuggestConfig.uiSchema(
+    ...


Oops, sorry! I think my previous feedback was confusing. This is what the code should look like here:

uiSchema: { officeLocation: autosuggestConfig.uiSchema( 'Preferred Office Location', // field title null, // Promise to get options (optional) { // Additional uiSchema options 'ui:options': { // When labels are not provided, it uses enumNames labels: { } } } ) }

Many thanks!

annekainicUSDS · 2018-08-27T19:23:43Z

docs/building-a-form/common-definitions.md

+schema: {
+  type: 'object'
+  properties: {
+    currentOrPastDate: currentOrPastDateConfig.schema()


This key also needs to be birthdate, so:

birthdate: currentOrPastDateConfig.schema()

annekainicUSDS · 2018-08-27T19:24:04Z

docs/building-a-form/common-definitions.md

+schema: {
+  type: 'object',
+  properties: {
+    currentOrPastMonthYear: currentOrPastMonthYearConfig.schema()


This key also needs to be lastContact, the same as above.

annekainicUSDS · 2018-08-27T19:24:22Z

docs/building-a-form/common-definitions.md

+schema: {
+  type: 'object',
+  properties: {
+    date: dateConfig.schema()


startDate here instead of date

annekainicUSDS · 2018-08-27T19:24:34Z

docs/building-a-form/common-definitions.md

+schema: {
+  type: 'object',
+  properties: {
+    dateRange: dateRangeConfig.schema()


servicePeriod instead of dateRange

annekainicUSDS · 2018-08-27T19:25:13Z

docs/building-a-form/common-definitions.md

+schema: {
+  type: 'object',
+  properties: {
+    monthYear: monthYearConfig.schema()


serviceStart instead of monthYear

annekainicUSDS · 2018-08-27T19:25:31Z

docs/building-a-form/common-definitions.md

+schema: {
+  type: 'object',
+  properties: {
+    monthYearRange: monthYearRangeConfig.schema()


serviceRange instead of monthYearRange

annekainicUSDS · 2018-08-27T19:26:18Z

docs/building-a-form/common-definitions.md

+schema: {
+  type: 'object',
+  properties: {
+    year: yearConfig.schema()


taxYear instead of year

Updates since the last review

bernars-usa · 2018-08-27T20:42:26Z

Thanks for the reviews, @annekainicUSDS and @dmethvin-gov! This is ready for a final check.

dmethvin-gov · 2018-08-27T21:16:05Z

Not sure why this failed in CircleCI the first time but I re-ran it and it's okay.

bernars-usa added 4 commits August 21, 2018 17:05

First pass at updating definitions and code samples

21776f4

Swap out schema and uiSchema

6d870f1

Add and reference images for the rendered definition fields

f464fc4

Update descriptions

8b0c1c9

bernars-usa requested review from annekainicUSDS and dmethvin-gov August 27, 2018 14:29

bernars-usa commented Aug 27, 2018

View reviewed changes

Complete the currency fields

f1e086f

annekainicUSDS reviewed Aug 27, 2018

View reviewed changes

dmethvin-gov previously requested changes Aug 27, 2018

View reviewed changes

Incorporate feedback from @annekainicUSDS and @dmethvin-gov

21eb051

I added some example labels in the `uiSchemas` - let me know what you think.

annekainicUSDS reviewed Aug 27, 2018

View reviewed changes

Fix bad js, code sample cc @annekainicUSDS

0b0cfa8

Small fix from Anne

d619fd4

dmethvin-gov approved these changes Aug 27, 2018

View reviewed changes

bernars-usa merged commit 44bf861 into master Aug 28, 2018

bernars-usa deleted the definitions-functions branch August 28, 2018 15:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update docs for definitions #236

Update docs for definitions #236

bernars-usa commented Aug 27, 2018

bernars-usa Aug 27, 2018

annekainicUSDS left a comment

annekainicUSDS Aug 27, 2018

annekainicUSDS Aug 27, 2018

bernars-usa Aug 27, 2018

annekainicUSDS Aug 27, 2018

annekainicUSDS Aug 27, 2018

annekainicUSDS Aug 27, 2018

annekainicUSDS Aug 27, 2018

annekainicUSDS Aug 27, 2018

annekainicUSDS Aug 27, 2018

dmethvin-gov Aug 27, 2018

annekainicUSDS Aug 27, 2018

annekainicUSDS Aug 27, 2018

dmethvin-gov Aug 27, 2018

dmethvin-gov Aug 27, 2018

dmethvin-gov Aug 27, 2018

annekainicUSDS left a comment

annekainicUSDS Aug 27, 2018

bernars-usa Aug 27, 2018

annekainicUSDS Aug 27, 2018

bernars-usa Aug 27, 2018

annekainicUSDS Aug 27, 2018

annekainicUSDS Aug 27, 2018

annekainicUSDS Aug 27, 2018

annekainicUSDS Aug 27, 2018

annekainicUSDS Aug 27, 2018

annekainicUSDS Aug 27, 2018

annekainicUSDS Aug 27, 2018

bernars-usa commented Aug 27, 2018

dmethvin-gov commented Aug 27, 2018

Update docs for definitions #236

Update docs for definitions #236

Conversation

bernars-usa commented Aug 27, 2018

Choose a reason for hiding this comment

annekainicUSDS left a 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

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

annekainicUSDS left a 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

bernars-usa commented Aug 27, 2018

dmethvin-gov commented Aug 27, 2018