Update "Example form.js" document #212
Conversation
|
Thanks for working on this, @erikphansen! I've added @annekainicUSDS and @dmethvin-gov as reviewers here. |
| @@ -16,95 +29,107 @@ Use this example `form.js` file to build a basic form. For more information abou | |||
| confirmation: ConfirmationComponent, | |||
|
|
|||
| // The prefix for Google Analytics events that are sent for different form actions. | |||
| trackingPrefix: '', | |||
| trackingPrefix: 'unique-ga-id-', | |||
There was a problem hiding this comment.
I have a PR in #196 that moves this out to the app, in which case trackingPrefix wouldn't be used.
|
|
||
| // Since field IDs are repeated in the form config and must be unique throughout the form, | ||
| // it's safer and more convenient to store them in an object | ||
| const formFields = { |
There was a problem hiding this comment.
I agree with the diagnosis that repeating names across all these objects is confusing and error prone. Ideally we should be grouping most of this information in a single object, rather than using the field name as a key to unite far-flung objects. That's related to design changes though, so perhaps this is the best we can do with the current formConfig design.
There was a problem hiding this comment.
This is an interesting idea. I agree with what @dmethvin-gov said, that we might be making further design changes to the form config that would change how some of this works. However, in general this change seems a bit prescriptive. It's not necessary for the library to work to put the field names in a separate object, it just potentially makes it less error prone. But I'm not sure if that recommendation belongs in the scope of this library, or should we just leave it up to individual developers to write their form config in whatever way makes most sense to them? If this is the new way Vets.gov is going to be writing their form configs, should this prescription belong in the Vets.gov codebase instead of here?
|
|
||
| // The title of the chapter. | ||
| title: '', | ||
| // The title of the chapter. Rendered near the top of the page, under the segmented controller |
There was a problem hiding this comment.
I would change segmented controller to segmented progress bar, because that's how we refer to it elsewhere.
| }, | ||
| 'view:artificialGroup'{ | ||
|
|
||
| // Specifies that a page will turn its schema into a page for each item in an array, such as an array of children with a page |
There was a problem hiding this comment.
Maybe we should add a header comment that clarifies that the following group of things are used for array fields only. The additional path reference is a bit confusing because we already specified the path earlier, so perhaps that header will make it more clear that this is only necessary for arrays.
| type: 'object', | ||
| properties: { | ||
| // `view:artificialGroup` is flattened. `subField1` and `subField2` are siblings of `[formFields.fieldOne]` when sent to the API. | ||
| subField1: { |
There was a problem hiding this comment.
If we want to stick with putting all fields in an object, subField1 and subField2 need to be added.
|
|
||
| // Since field IDs are repeated in the form config and must be unique throughout the form, | ||
| // it's safer and more convenient to store them in an object | ||
| const formFields = { |
There was a problem hiding this comment.
This is an interesting idea. I agree with what @dmethvin-gov said, that we might be making further design changes to the form config that would change how some of this works. However, in general this change seems a bit prescriptive. It's not necessary for the library to work to put the field names in a separate object, it just potentially makes it less error prone. But I'm not sure if that recommendation belongs in the scope of this library, or should we just leave it up to individual developers to write their form config in whatever way makes most sense to them? If this is the new way Vets.gov is going to be writing their form configs, should this prescription belong in the Vets.gov codebase instead of here?
|
@erikphansen Thank you so much for submitting this PR! After reviewing with our team, we're not ready to merge these changes into the library. We think the suggestions for how to structure the form config are potentially very useful to users of the library, but we're not sure these suggestions belong as instructions for how to do things. Instead, we think this suggestion is more appropriate to demonstrate in example forms to show how the library can be used, which we're planning on building out in the next month. For now, I will create a ticket to revisit this idea, and close this PR in the meantime. Let us know if you have any additional thoughts or questions on this! |
|
@annekainicUSDS Totally understandable why you wouldn't want to merge the PR as I wrote it. I'd like to make a much smaller PR that simply fixes a few errors in the example code without injecting my own opinion on how things should be done, which rightly doesn't belong in this repo. |
Description
I'm getting up to speed with the US Forms System for my work on Vets.gov with Ad Hoc. I found a small number of errors and areas where clarity was lacking in the documentation for the form.js config object. This PR addresses them.
Additional information
I also included a change that reflects an improved/safer naming strategy for both field keys and page ID keys on the config object. It's a change that makes large config objects easier for a human to parse makes them less error prone. Using the same string all over the place is not very safe and leads to errors. Also, using array-access notation to reference the strings that are stored in an object makes it clear that the repeated keys actually are related and are not named the same coincidentally.