WIP: Allow Input component to render child components passed to it

[kevinkuszyk]

@Tomdango as you know from discussions with @SatnamSinghUK we have a requirement to render input boxes with a hyperlink underneath, all inside a form group. Something like this:

I had a stab this afternoon at enabling the Input component to take child components. The usage is something like this:

<Input label="Question One" width="10">
    <a href="#">Don't know the Question One?</a>
</Input>

It's quick and dirty, but it enables us to hit a deadline this week. Unfortunately, it also has a number of issues, not least, I'm not sure how accessible it will be.

So, would you accept this contribution as is?

If no, are you ok with this approach, but would like some changes (like tests?)?, or would you prefer if we went down the route you and @SatnamSinghUK have discussed? Something more like this:

<InputWithLink>
    <InputWithLink.Link href=”/some-location”>Example Text</InputWithLink.Link>
</InputWithLink>

[Tomdango]

I think the issue here is more around allowing components to be passed inside of the <FormGroup> component rather than just the Input itself - perhaps something could be done to allow passing in a custom render function to the Input to allow components inside the FormGroup?

[kevinkuszyk]

@Tomdango that was actually my starting point, but I hit two snags:

1. The <FormGroup> isn't exported from the lib.
2. I didn't want to explicitly create a <FormGroup> in my code and then have to re-implement all the good stuff in the components to make labels validation styles etc work.

Are you thinking something like this might work better:

<FormGroup>
    <Input label="Question One" width="10" />
    <div>
        <a href="#">Don't know the Question One?</a>
    </div>
</FormGroup>

[Tomdango]

The annoying bit is the way that the FormGroup works - it's not just a wrapper for <div class="nhsuk-form-group">, it takes a function as a child, then passes props to that child that automatically contains generated IDs and references for labels and hints - generally just ensures that we stay accessible throughout. I was thinking that children could be passed through as a prop to the FormGroup, something like formGroupChildren?

[kevinkuszyk]

The annoying bit is the way that the FormGroup works - it's not just a wrapper for <div class="nhsuk-form-group">, it takes a function as a child

Yes, I spotted that. Do you think it's ripe for a refactor to make the usage more like the example I posted earlier?

then passes props to that child that automatically contains generated IDs and references for labels and hints - generally just ensures that we stay accessible throughout

Yes, this is the good stuff I didn't want to lose.

I was thinking that children could be passed through as a prop to the FormGroup, something like formGroupChildren?

I'm not sure about this, as it feels like a workaround on how the component is built, rather than doing it the react way?

Do you know the background as to why it takes a function, rather than child components in the usual react way?

As you know, I'm new to react, but I think we should aim for something that is as simple a possible for consumers and follows the usual react patterns for composition etc.

Thoughts?

[Tomdango]

Do you know the background as to why it takes a function, rather than child components in the usual react way?

When any Form component is used, all of the props are passed to the FormGroup component, rather than the Input component itself - this is how it performs it's logic around labels, hints, errors and other fun stuff. These modified props are then passed to it's children, which takes a render function instead of a standard component so that the new props are always directly passed from the FormGroup to the Input. The only other way we could pass in additional props in this way is to do some React.Children.map trickery which can be flaky at best. It also allows us to maintain good typing throughout, through the use of generics in the FormGroup itself. The FormGroup component was never designed to be a public component, just as a helper component to make the implementation of accessible form labels a lot easier and more solid.

I think we could go in the middle here:

We could implement a new FormGroup component that doesn't contain any of the other logic, and just the wrapper classes. This could expose a FormGroupContext that tells any subsequent form components that it is already wrapped inside of a FormGroup and that no additional <div class="nhsuk-form-group"> components would need to be rendered. This would allow you to have code that would work 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>
);

Which would output something along the lines of:

<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>

This seems like how this should work - we don't lose any functionality when not using a FormGroup directly (IDs should still pass through etc) - and the Input will still be wrapped in the nhsuk-form-group div if none is already supplied. We'd also have to rename the internal FormGroup component. How does that sound?

[kevinkuszyk]

@Tomdango that sounds perfect - to consumers it's a standard react, but inside we get all the accessibility benefits etc.

@SatnamSinghUK how do you feel about implementing this?

I'm guessing we're going to need to apply this refactor to all the form components in one go?

[SatnamSinghUK]

@Tomdango That looks like the way to go with this

@kevinkuszyk Yep, let's start to pull it into our elaboration/sprints sessions

[kevinkuszyk]

@Tomdango we have some feature work which is ahead of this on our priority list, so it might be a sprint or two before we get to this.