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

Global Forms #743

Merged
merged 116 commits into from
Jul 6, 2022
Merged

Global Forms #743

merged 116 commits into from
Jul 6, 2022

Conversation

Heydon
Copy link
Contributor

@Heydon Heydon commented May 5, 2022

Major reworking of global-forms following the formation of The Commitee For HTML Forms™️ (@alexkilgour, @Heydon, @sturobson, @axemonkey, @sky-jack). Design is informed by @foxintherain's work: https://www.sketch.com/s/c77d316e-76e5-403e-95b8-93021fc6c95e/p/0EB75A8C-3CC3-40C8-A48E-AE685CB267BC. Note that the button styles have not been updated to reflect the Sketch file since this is a separate concern with a broader scope.

This is an RC (release candidate) release.

From the HISTORY.md file:

## 5.0.0-rc.1 (2022-06-27)
    * BREAKING:
        * Form fields now available as a suite of templates/partials
        * Major changes to styling and styling API
        * Design tokens support

Includes comprehensive documentation.

@@ -0,0 +1,34 @@
// Generated on 30/06/2022, 16:20:58
// Source: design-tokens/componenets/global/global-forms/default.json
Copy link
Collaborator

Choose a reason for hiding this comment

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

Spelling mistake: componenets should be components
across this and springer

Copy link
Contributor

Choose a reason for hiding this comment

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

whoops!

I had a bit of a re-think on this, as an aside.

The current npm scripts for generating the Sass variables are a pretty blunt tool in generating literal and alias tokens (pretty much) every time.

I'm going to rework it so that they're either very separate tasks or the script checks for changes to disc for the.json files and only compiles things that have changed.

Moving forward, it should be the case that literal tokens very rarely get changed. alias tokens do get changed but not often, and component tokensonly get changed/added to when there are changes from thealiastokens or we are introducingcomponent` tokens to the component.

If that makes sense.

@sturobson
Copy link
Contributor

with the linting failing on shortened hex color codes (or them not being shortened) hadn’t failed when they were introduced in the brand-context - This issue should soon be irrelevant when we switch back on the output to reference the alias token Sass variables.

Copy link
Contributor

@amyhupe amyhupe left a comment

Choose a reason for hiding this comment

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

OK, here's my review. Mostly small things, aimed at:

  • increasing consistency of terminology, as much as possible
  • referencing user impact, where appropriate
  • simplifying language

I've also suggested copy changes to some of the example copy we're using. My general take on this is that example copy should be obviously an example (e.g. Monkey, Platypus etc.) or realistic, and something we'd be happy to be used in a real service.

I hope these comments make sense - let me know if anything needs clarifying. And good work - this is an absolute beast!

toolkits/global/packages/global-forms/README.md Outdated Show resolved Hide resolved

## Branding
This component comprises a number of form fields and related templates. It is designed to make constructing any variety of HTML form a relatively straightforward process.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
This component comprises a number of form fields and related templates. It is designed to make constructing any variety of HTML form a relatively straightforward process.
This component includes a number of form fields and related templates. It is designed to make it as simple as possible to create an HTML form.


The `global-forms` component currently uses the `DEFAULT` brand only.
The component does not ship with any JavaScript. States (such as an invalid/error state) are defined at a data level. Implementations using client-side processing/validation may benefit from compiling the form’s (handlebars) template in the browser.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The component does not ship with any JavaScript. States (such as an invalid/error state) are defined at a data level. Implementations using client-side processing/validation may benefit from compiling the form’s (handlebars) template in the browser.
The component does not include any JavaScript. States (such as an invalid/error state) are defined at a data level. If you're using client-side processing, you might benefit from compiling the form’s handlebars template in the browser.

There are two options for rendering form fields:

1. Compile each field independently, using its respective template from the `view/fields` folder.
2. Organize your fields into fieldsets using the data structure exemplified in `demo/context.json`. In this case, you will have to iterate over a `fieldsets` property something like this:
Copy link
Contributor

Choose a reason for hiding this comment

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

I know there's still a question mark over whether we use US or UK English in SN, but I've been using UK English in the docs so far so I'm changing it here for consistency.

Suggested change
2. Organize your fields into fieldsets using the data structure exemplified in `demo/context.json`. In this case, you will have to iterate over a `fieldsets` property something like this:
2. Organise your fields into `fieldset`s using the data structure shown in `demo/context.json`. In this case, you'll have to iterate over a `fieldsets` property something like this:

There are two options for rendering form fields:

1. Compile each field independently, using its respective template from the `view/fields` folder.
2. Organize your fields into fieldsets using the data structure exemplified in `demo/context.json`. In this case, you will have to iterate over a `fieldsets` property something like this:
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't fully understand what "iterate over a fieldsets property" means - maybe this will make sense to FEDs but just wanted to see if you think it could do with clarifying?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Putting"over a fieldsets array" to make it more explicit what the iteration refers to.

"template": "text",
"label": "Your name (with error)",
"hint": "Your name is on your passport",
"error": "That is not a name, it’s a number",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"error": "That is not a name, it’s a number",
"error": "Enter your name",

{
"template": "checkbox",
"label": "Agree to our terms (with error)",
"error": "You must agree to the terms before continuing.",
Copy link
Contributor

Choose a reason for hiding this comment

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

Updating to keep consistent with my previous suggestions:

Suggested change
"error": "You must agree to the terms before continuing.",
"error": "Agree to the terms to continue.",

{
"template": "checkbox",
"label": "Agree to our terms (with hint)",
"hint": "You should have read these. Read them, then agree to them.",
Copy link
Contributor

@amyhupe amyhupe Jul 4, 2022

Choose a reason for hiding this comment

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

Changing to something I'd be happy to see in a real service

Suggested change
"hint": "You should have read these. Read them, then agree to them.",
"hint": "You must agree to the terms to continue your application",

"template": "checkbox",
"label": "Agree to our terms (with error)",
"error": "You must agree to the terms before continuing.",
"hint": "You should have read these. Read them, then agree to them.",
Copy link
Contributor

@amyhupe amyhupe Jul 4, 2022

Choose a reason for hiding this comment

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

As before

Suggested change
"hint": "You should have read these. Read them, then agree to them.",
"hint": "You must agree to the terms to continue your application.",

@@ -13,3 +17,265 @@ The `global-forms` component currently uses the `DEFAULT` brand only.
// Include this with your other components
@import '@springernature/global-forms/scss/50-components/forms';
```

Then you will need to register the Handlebars partials in the `/view` folder.
Copy link
Contributor

Choose a reason for hiding this comment

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

For consistency:

Suggested change
Then you will need to register the Handlebars partials in the `/view` folder.
Then you will need to register the handlebars partials in the `/view` folder.

Copy link
Contributor

@sturobson sturobson Jul 4, 2022

Choose a reason for hiding this comment

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

as a long time owner of Sassnotsass.com I had a quick look at the docs for handlebars … and although the logo is in lowercase they seem to use Title Case when it's mentioned throughout the docs.

(I'm happy either way 😄)

@sturobson
Copy link
Contributor

@alexkilgour I think this PR … would be a good chance to clean up the issues list … as I spotted this last night -- #485 -- I'm assuming some of these would be nullified with this PR?

@@ -0,0 +1,6 @@
<label for="{{id}}" class="c-forms__label">
{{label}}{{#if optional}} (optional){{/if}}
Copy link
Contributor

Choose a reason for hiding this comment

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

🤔 I don't think "optional" is really helpful for the following reasons

  1. By default all text fields are "optional" i.e they dont have "required" set to true.
  2. This will not be useful if the service has to support multiple languages because the copy is not translated.

When I worked in GOV we tried to make all fields required by default (no point asking a user for data if its not useful for all) and only flag up "optional" fields in the label like what is done here but in our case the text fields are not required by default so seems to send mix messages as to when to use this option or not.

Copy link
Contributor

Choose a reason for hiding this comment

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

Instead just let the devs control the copy and if they want to use "(Optional)" they could just include it in their copy that they need for the label.

Copy link
Contributor

Choose a reason for hiding this comment

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

The determination of what is optional or not, or what copy should be used would come from agreement across dsign/ux/content that would ultimately end up in the Design System docs site?

Copy link
Contributor

Choose a reason for hiding this comment

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

Instead just let the devs control the copy and if they want to use "(Optional)" they could just include it in their copy that they need for the label.

From a content design point of view, I'd really like to introduce some standardisation if possible. I get that we're not currently offering localisation of this copy, but we're not currently doing that for the content elements of any components so this isn't a new problem. My concern is if we don't include something, people are going to fall back to using asterisks.

Copy link
Contributor

Choose a reason for hiding this comment

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

So, @Heydon has followed the Sketch designs (see screenshot). I'd have to check with James (on holiday until Monday (11/07/22) but I think all of the form designs have gone through rounds of review in SN digital and this is currently the latest agreed across discipline.

Screenshot 2022-07-05 at 11 45 12

Copy link
Contributor

Choose a reason for hiding this comment

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

regarding the issue with translation. It could be made a handlebars variable in the json, rather than hardcoded as it is now‽ So it could get switched out as needed? (pinging @Heydon)

Microcopy like (optional) here could also be tokenised -- which could further help documentation of it all

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was just following the UX. Can we discuss this after v1 is out, please? For the time being, just put required: true on all your fields if you want them all required.

Copy link
Contributor

@amyhupe amyhupe left a comment

Choose a reason for hiding this comment

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

Hello,

A few additional revisions on yesterday's revisions. These are things I think could be better clarified, on a second read, or where copy errors have been introduced in the process of suggesting & accepting changes.

}
```

If you do wish to include a legend, you can use HTML to style it and add semantic meaning. It is recommended to use a heading of the correct level as the legend content.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggesting another small revision to this to make the second sentence active, not passive.

Suggested change
If you do wish to include a legend, you can use HTML to style it and add semantic meaning. It is recommended to use a heading of the correct level as the legend content.
If you do wish to include a legend, you can use HTML to style it and add semantic meaning. In most cases, legends should be headings, with the heading level determined by the page structure.


### Fields

Fields take the `template` property to determine which of the `view/fields` template files they represent. This property along with `label`, `id`, and The `template` property sets the type of field - for example, `"template": "text"` renders a text input field. Aim to make the `template`, `label`, `id`, and `name` properties mandatory parts of your data schema.
Copy link
Contributor

Choose a reason for hiding this comment

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

Something went awry with the resolving of these edits. Fixed:

Suggested change
Fields take the `template` property to determine which of the `view/fields` template files they represent. This property along with `label`, `id`, and The `template` property sets the type of field - for example, `"template": "text"` renders a text input field. Aim to make the `template`, `label`, `id`, and `name` properties mandatory parts of your data schema.
The `template` property sets the type of field - for example, `"template": "text"` renders a text input field. Aim to make the `template`, `label`, `id`, and `name` properties mandatory parts of your data schema.


Fields take the `template` property to determine which of the `view/fields` template files they represent. This property along with `label`, `id`, and The `template` property sets the type of field - for example, `"template": "text"` renders a text input field. Aim to make the `template`, `label`, `id`, and `name` properties mandatory parts of your data schema.

This component supports a wide range of standard form field attributes. So, for example, to include a `readonly` attribute on your text input, you can include a property of the same name on the data:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
This component supports a wide range of standard form field attributes. So, for example, to include a `readonly` attribute on your text input, you can include a property of the same name on the data:
This component supports a wide range of standard form field attributes. For example, to include a `readonly` attribute on your text input, you can include a property of the same name on the data:

}
```

You can summarise errors using a top level `errorSummary` property (adjacent to the `fieldsets` property). Each error in the errors array must point to the `id` of the input it relates to and repeat its `error` message:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
You can summarise errors using a top level `errorSummary` property (adjacent to the `fieldsets` property). Each error in the errors array must point to the `id` of the input it relates to and repeat its `error` message:
You can summarise errors using a top level `errorSummary` property adjacent to the `fieldsets` property. Each error in the errors array must point to the `id` of the input it relates to and repeat its `error` message:


### Making choices

`Select` fields (using the `<select>` element) define their choices via an `options` property, which must be an array. The `selected` property is a Boolean:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's good to specify what "their choices" refer to. I know this is technical documentation, but the more we can do to contextualise it for designers now, the less we'll have to edit later when we layer in the design guidelines.

Suggested change
`Select` fields (using the `<select>` element) define their choices via an `options` property, which must be an array. The `selected` property is a Boolean:
`Select` fields (using the `<select>` element) define the user's answer choices with an `options` property, which must be an array. The `selected` property is a Boolean:


Radios are always grouped together in a `fieldset`, so the group label (“Animal”, here) renders as a `legend` not a `label`.

You might want to show users an additional field when they select a radio. For example, revealing a text input field for them to give more specific information about the option they’ve selected.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
You might want to show users an additional field when they select a radio. For example, revealing a text input field for them to give more specific information about the option they’ve selected.
You might want to show users an additional field when they select a particular radio. For example, revealing a text input field for them to give more specific information about the option they’ve selected.

### Buttons

A `template` of `buttons` defines a set of button controls, displayed inline (using Flexbox and `gap` for tidy wrapping).
The `type` property for each individual button corresponds to the `type` property/attribute. For example, here is how you would include a submit button:
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we make a choice here between property and attribute, and lose the slash? If it really could be either, can we say "or"?

Copy link
Contributor

@sturobson sturobson left a comment

Choose a reason for hiding this comment

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

"It's been a long road. Getting from there to here. It's been a long time. But the time is finally near."

I think we're now in a good place to release this as an rc1 version.

Buyer Beware! There may be dragons!

Great work @Heydon and all involved in recreating global-forms for 2022 and beyond, finding bugs, questioning and improving things so rc1 is pretty solid out of the gate.

@Heydon Heydon merged commit ab63ab5 into master Jul 6, 2022
@sturobson sturobson deleted the form-templates branch July 6, 2022 15:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants