Expose FormGroup component to consumers

[kevinkuszyk]

Following on from the discussion in #35, this issue is to clarify our intent for this.

We would like to expose a FormGroup component so that it can be used by library consumers, so that all the boiler plate and logic for error styles, hint text etc can be encapsulated and easily reused. This will help us in Forms so that we can focus on our custom components and not repeat all the boiler plate.

There is some complexity here as the existing FormGroup component was not designed to be exposed and it accepts a function instead of child components in the usual way.

The goal is to have syntax like this:

import React from "react";
import { FormGroup, Input } from "nhsuk-react-components";

const ExampleUsage = () => (
    <FormGroup>
        <Input hint="Test Hint" label="Test Label" />
        <a href="/somewhere-else">Don't have a number?</a>
    </FormGroup>
);

Emit something like this:

<div class="nhsuk-form-group">
    <label class="nhsuk-label">Test Label</label>
    <span class="nhsuk-hint">Test Hint</span>
    <input />
    <a href="/somewhere-else">Don't have a number?</a>
</div>

The new / exposed FormGroup should support at least the following props (and associated functionality):

1. hint
2. label
3. error

---

Questions

1. Should we also support disableErrorLine?
2. What else is included with labelProps, hintProps, errorProps and formGroupProps, and should we support them?

---

Fyi. Forms team, we're tracking this in Jira as WBFCD-399.

---

@Tomdango anything else to add?

@ramyas16 ping

[Tomdango]

The current FormGroup takes a function as it's children, and passes through all relevant props, i.e.

<FormGroup>
    {({value, onChange) => (
        <Input value={value} onChange={onChange}
    )}
</FormGroup> 

If the FormGroup wants to be exposed a bit more naturally, some context would need to be used and implemented in each Input component to ensure that all relevant IDs are also passed along with it.

In practice, this might look something like:

const FormGroup = () => {
    const [inputID, setInputID] = useState();
    
    return (
        <FormGroupContext.Provider value={{ setInputID }}>
            <Label htmlFor={inputID}>{label}</Label>
            {children}
        </FormGroupContext.Provider>
    )
}

const Input = ({ id }) => {
    const { setInputID } = useContext(FormGroupContext);
    
    useEffect(() => {
        setInputID(id);
        return () => setInputID(undefined);
    }, [id])
    
    return <Input id={id} />
}

Though I can see this approach breaking down if multiple Inputs are being used inside a single FormGroup, in which case the render function might work better. Both approaches could be used by doing a check on the children prop:

const isUsingRenderFunction = typeof children === "function";

[jenbutongit]

Sorry if I'm missing some context! Instead of render props, I wonder if we do checks on the children instead? The below is a "prescriptive" layout, although you could just {children} if you don't care much for the order.

//FormGroup.ts

import { Input, Label, Hint, Link } from '~components'

function FormGroup({ hasErrors = false, children }) {
  const childrenArray = React.children.toArray(children);
  const InputChild = childrenArray.find(child => child.type === Input) ?? null; // or typescript guard. You can use child.type === Input if Input is a function 
  const LabelChild = childrenArray.find(child => child.type === Label) ?? null;
  const HintChild = childrenArray.find(child => child.type === Hint) ?? null;
  const LinkChild = childrenArray.find(child => child.type === Link) ?? null;
  return (
    <div className={classNames('nhsuk-form-group', { 'nhsuk-input--error': hasError })}>
      {LabelChild}
      {InputChild}
      {HintChild}
      {LinkChild}
    </div>
  );
}

n.b. I forget if you are able to use child.type === Input if Input is an anonymous function (const Input = (..) => { }). It should work if it's defined function Input(..)

const ExampleForm = ({ children }) => {
  const [firstName, setFirstName] = useState('');
  const [hasErrors, setHasErrors] = useState(false);
  const onFirstNameChange = event => {
    // validation, or whatever else
    setFirstName(event.target.value);
  };
  function onSubmit(e) {
    e.preventDefault();
    // post request
  }
  return (
    <form onSubmit={onSubmit}>
      <FormGroup hasErrors={hasErrors}>
        <Label htmlFor="first-name">{i18n('firstName.title')}</Label>
        <Hint>{i18n('firstName.hint')}</Hint>
        <Input id="first-name" value={firstName} onChange={onFirstNameChange} />
      </FormGroup>
    </form>
  );
};

Of course, in ExampleForm you can hoist the id / htmlFor into a variable. id={inputId}. ExampleForm is how is more likely to be used. The logic for validation is most likely going to be at this level, as well as submitting, so it would make sense to define these here, instead of at FormGroup level.

Using a Context would be overkill for just an id, there's no prop drilling involved really. It would also involve refactoring the input and label to be consumers (adding useContext(..)). Contexts can make testing a pain as well. Providers need to wrap the component under test (whether or not it's shallow or a full render) or it will throw an error

[Tomdango]

Hey @jenbutongit

The approach you suggested was actually the original way the forms worked (pre-1.0) - however what we found with it in practice is it's quite restrictive with what you can use inside that element. For example, the following would break:

<FormGroup>
    <div className="custom-form-wrapper">
        <Input />
    </div>
</FormGroup>

The workaround would be to scan all potential child elements of all the children, which ends up being a lot more code and a much more expensive operation on every render than using context.

[jenbutongit]

@Tomdango would/is there be a case where you would wrap the input, label, link etc within FromGroup, but not use the FormGroup wrapper + additional classes itself? It's already a wrapping component

<FormGroup className="custom-form-wrapper"> //outputs nhs-form-group custom-form-wrapper
        <Input />
</FormGroup>