refactor(toggle-group): split into composable components and generalise

[HendrikThePendric]

Based on the excellent #381 (comment) @varl placed on my previous PR which had been merged already, I've come up with a much more composable solution in this PR.

Some notes:

1. I quickly realised that this way of implementing it is much more flexible, and would allow us to reuse this component for grouping Checkboxes and Switches too. As such I've renamed everything that was radio to toggle, which is the most descriptive word I could come up with to describe radio/checkbox/switch inputs.
2. We really need to have a label of some kind to put above the set of 'toggles'. I found it hard to come up with a suitable name, and also wasn't sure if this would be something we would want to expose individually. So as an alternative, this is now exposed as ToggleGroup.Label. This is the best I could come up with, but still isn't perfect, because the ToggleGroup.Label is actually above the ToggleGroup and not inside of it...

Looking forward to finding out what you think of this second attempt.

[varl]

Much better. 👍

Could this be so general as to accommodate other field components? In that case we could rename the ToggleGroup to e.g. FieldSet and implement it as a fieldset component under the hood.

2. We really need to have a label of some kind to put above the set of 'toggles'. I found it hard to come up with a suitable name, and also wasn't sure if this would be something we would want to expose individually. So as an alternative, this is now exposed as ToggleGroup.Label. This is the best I could come up with, but still isn't perfect, because the ToggleGroup.Label is actually above the ToggleGroup and not inside of it...

This sounds like a HTML <fieldset>, + a HTML <legend>.

We can provide our own styled component pair: FieldSet and Legend. They would be exposed individually and could technically be used with other fields as well. E.g.

<FieldSet stacked>
  <Legend>Toggle fields</Legend>
  <Radio />
  <Switch />
</FieldSet>

<FieldSet>
  <Legend>Input fields</Legend>
  <InputField />
  <SelectField />
</FieldSet>

fieldset elements can be nested as well: https://codepen.io/varl/pen/pozRWxb

[varl]

Since removing an exposed component is a BREAKING CHANGE we should consider if we should rename InputField to Input and SelectField to Select to be more consistent with the other form elements e.g. Switch, Radio, Checkbox, that do not have the -Field suffix.

[Mohammer5]

Since removing an exposed component is a BREAKING CHANGE we should consider if we should rename InputField to Input and SelectField to Select to be more consistent with the other form elements e.g. Switch, Radio, Checkbox, that do not have the -Field suffix.

A very welcomed change on my side, these name annoyed me ever since I encountered them the first time

---

This sounds like a HTML <fieldset>, + a HTML <legend>.

I wouldn't mind using these elements, we just have to be aware, that we'll need an extra wrapper for the input elements if we wanted them to be flexed:

Note: as of this writing, there are bugs in Microsoft Edge and Google Chrome which prevent flexbox and grid layouts from being used inside a <fieldset>. This GitHub issue provides bug tracking links.

[varl]

This sounds like a HTML <fieldset>, + a HTML <legend>.

I wouldn't mind using these elements, we just have to be aware, that we'll need an extra wrapper for the input elements if we wanted them to be flexed:

Note: as of this writing, there are bugs in Microsoft Edge and Google Chrome which prevent flexbox and grid layouts from being used inside a <fieldset>. This GitHub issue provides bug tracking links.

Nice catch, we do need to consider that. 🤔

[varl]

A very welcomed change on my side, these name annoyed me ever since I encountered them the first time

#389

[HendrikThePendric]

This PR can now be reviewed again.

One note:
Because we are now rendering the <legend> from the FieldSet, this component now also needs to know about statuses (error, warning, valid) + required.... Previously, when it was ToggleGroup it was nicely separated. So this is a minor drawback, but overall I think this implementation is good....

[varl]

@cooper-joe could you have a look at the statuses of the FieldSet?

[Mohammer5]

Always forwarding the statuses is not good in my opinion.
A fieldset can also be used to group logical inputs that don't have the same name attribute, like this:

<fieldset>
	<legend>Personal details</legend>
	<label for="first-name">First name</label>
	<input name="first-name" id="first-name" type="text">
	<label for="family-name">Family name</label>
	<input name="family-name" id="family-name" type="text">
	<label for="email">Email</label>
	<input name="email" id="email" type="text">
</fieldset>

(source: http://usability.com.au/2013/04/accessible-forms-1-labels-and-identification/)

---

I'd propose a forwardStatus prop that defaults to false (or no default, which is falsy)

[HendrikThePendric]

Good that you bring this up, because I was having second thoughts about this too. Upon reflection, I actually don't think we should forward these props at all. By forwarding these props we could be creating unexpected behaviour:

• suppose the error prop of the FieldSet is set to true
• and the error prop of a child Radio is set to false
• because of the way the component prop are merged with the object being passed as a second argument of cloneElement, the Radio will now have prop error set to true
• So effectively the parent props are overriding the props set directly on the child. This is not good...

So, I will stop forwarding these props, and in cloneElement I will only add the fieldset-input to the classNames. This way we have a clearer separation of responsibility.

[varl]

disabled too perhaps?

[varl]

I think using the four-value sequence is preferable from a expressive perspective: 0 0 ${spacers.dp12} 0

[varl]

Option:

fieldset.valid > legend { ... }

I prefer yours though.

[varl]

It is awkward that the FieldSet adds spacing to form components.

If we have some components within a FieldSet and some outside, the fields are going to have different margins solely based on the position in the hierarchy.

[Mohammer5]

It could also happen the, due to a different specificity, the margin applied here will be ignored.
(I didn't think about that yesterday, when @HendrikThePendric and I were talking about this)

But specificity is another good reason not to interfere with a component's style but wrap it with another element instead (of course we could still have collapsing margins, so if the inner element has a bigger margin, that one will win, be we won't have 0-margin values anymore)

[varl]

Although the name FormControl might be a bit clunky, the function of it is perfect for this. Field would be an excellent name:

  <Field>
    <Input
      label="text"
    />
    <Help>text</Help>
  </Field>

[Mohammer5]

I think that component would be an internal wrapper inside FieldSet anyways,
so I have no objections to Field, but I think FieldWrap is more accurate.

[varl]

I think that component would be an internal wrapper inside FieldSet anyways,
so I have no objections to Field, but I think FieldWrap is more accurate.

That doesn't address the problem that components inside FieldSet are rendered different solely due to the fact that they are put inside a FieldSet component:

<>
    <Input value="This value is valid" valid label="A valid thing" />
    <Help valid>Foobar</Help>

    <FieldSet label="Choose something">
        <Radio
            onChange={onChange}
            name="radio"
            value="first"
            label="First"
        />
        <Radio
            onChange={onChange}
            name="radio"
            value="second"
            label="Second"
            checked
        />
        <Radio
            onChange={onChange}
            name="radio"
            value="third"
            label="Third"
        />
    </FieldSet>
    <Input value="This value is invalid" error label="An invalid thing" />
    <Help valid>Foobar</Help>
</>

We could wrap every input in a FieldSet:

<FieldSet>
    <Input value="This value is invalid" error label="An invalid thing" />
    <Help valid>Foobar</Help>
</FieldSet>

But that looks a bit funky unless we add stacked:

With stacked:

Still not perfect...

If we use the FormControl component though:

<>
    <FormControl>
        <Input value="This value is valid" valid label="A valid thing" />
        <Help valid>Foobar</Help>
    </FormControl>

    <FieldSet label="Choose something" stacked>
        <FormControl>
            <Radio
                onChange={onChange}
                name="radio"
                value="first"
                label="First"
            />
        </FormControl>

        <FormControl>
            <Radio
                onChange={onChange}
                name="radio"
                value="second"
                label="Second"
                checked
            />
        </FormControl>

        <FormControl>
            <Radio
                onChange={onChange}
                name="radio"
                value="third"
                label="Third"
            />
        </FormControl>
    </FieldSet>

    <FormControl>
        <Input value="This value is invalid" error label="An invalid thing" />
        <Help valid>Foobar</Help>
    </FormControl>
</>

If we go from stacked on the group of radio controls to inline, then the margin-right is gone too, so that would need to be sorted.

It is very explicit, it gets rid of the React.Children.map and React.cloneElement and just renders the children as they are defined:

<fieldset className={cx(className)}>
    {label && (
        <legend className={cx({ valid, warning, error, required })}>
            {label}
        </legend>
    )}
    <div className={cx({ stacked })}>
        {children}
    </div>
    <style jsx>{styles}</style>
</fieldset>

These are very low-level components, and will not be used directly in "real" forms, but rather always use a molecule perhaps defined in ui-forms, so a realistic form could look like:

<InputField label="" help="" value="" />
<InputField label="" help="" value="" valid />

<FieldSet stacked>
  <RadioField label="" value="" />
  <RadioField label="" value="" />
  <RadioField label="" value="" />
</FieldSet>

<InputField label="" help="" value="" error />
<CheckboxField label="" value="" help="" />

[HendrikThePendric]

It is awkward that the FieldSet adds spacing to form components.

I think this is quite a good point, however, this thread is making me think that we actually need to stop looking at the code and the components we have at this point in time. Instead we should have a broader discussion about the components we need to a) layout form-fields, and b) build inputs that are composed of several components.

To explain why I think this, I'll give a short summary on how we got here in the first place:

1. I was working on the RadioGroup component in ui-forms, and I realised it needed some styled elements:
   1. Some sort of text label to go above the radio inputs, with some margin
   2. Some spacing between the Radios, and support for stacked and inline
   3. I needed some sort of wrapper component, so the each form component in ui-forms would have a root component with some spacing.
2. We decided ui-forms should contain no styling, so we should introduce helper components in ui-core. Still fully on board with that.
3. Some of these helper components have gone through a full-on Pokemon evolution by now. We started with a RadioGroup (which was meh), which then evolved into a ToggleGroup (better, but still had quite a narrow scope and the ToggleGroup.Label component was awkward), and that then evolved into the FieldSet component.
4. This FieldSet component makes a lot of sense, and is nice and generic. And the fact that such a generic component currently adds spacing to form components is indeed fishy.... So I would agree we should probably remove this behaviour.....
5. However.... Now we've gotten to a point where we have a nice new FieldSet component, which makes a lot of sense conceptually, but I don't have a use-case for at the moment. And at the same time, I am still not able to compose the ui-forms RadioGroup from a bunch of ui-core components, which was how this all started. So we have sort of come full circle...

I think the summary above highlights that we are currently still working out what we need. So perhaps we'd better focus on that first, and continue building stuff later.

Do you agree @varl @Mohammer5 ?

[Mohammer5]

Now we've gotten to a point where we have a nice new FieldSet component, which makes a lot of sense conceptually, but I don't have a use-case for at the moment.

fielset itself is a very useful component: It enables accessibility in forms.
See: http://usability.com.au/2013/04/accessible-forms-1-labels-and-identification/

So I think we need

1. FieldSet (which removes the default fieldset and legend styles)
2. FormControl (which wraps 1 field row for vertical spacing; could be 1 or more fields)
3. StackControl (which wraps 2 or more components; can be vertical or horizontal)

Edit:
The FormControl will allow any layout, while the StackControl will not

[varl]

Agreed, taking a step back and thinking about how to lay out the components is a good idea. My notes above are intended to hammer home the API design with concrete examples so we don't dig the same hole over and over again.

[varl]

5. However.... Now we've gotten to a point where we have a nice new FieldSet component, which makes a lot of sense conceptually, but I don't have a use-case for at the moment. And at the same time, I am still not able to compose the ui-forms RadioGroup from a bunch of ui-core components, which was how this all started. So we have sort of come full circle...

Does not FieldSet + FormControl give you ability to compose the ui-forms::RadioGroup component?

[Mohammer5]

Does not FieldSet + FormControl give you ability to compose the ui-forms::RadioGroup component?

I'll interfere her:
It would probably, yes. But then we couldn't use the FormControl for other inputs in ui-forms to group inputs from ui-core with Help from u-core, which is why I proposed the 3 components above.

With those 3 you could also have multiple radio groups together in 1 FieldSet, each RadioGroup separated by a FormControl

[cooper-joe]

@cooper-joe could you have a look at the statuses of the FieldSet?

Could I get some clarifcation on how the statuses work?

1. I see the status text is appended to the label, are we unable to add the standard 'error' text (under the toggle controls)?
2. Are we able to maniuplate the background color of the FieldSet? (I'm thinking light red bg-color for error etc)

Once I get this info I will take a quick look into how to best present the statuses and provide specs, thanks!

[varl]

1. I see the status text is appended to the label, are we unable to add the standard 'error' text (under the toggle controls)?

<FormControl>
    <Radio
        onChange={onChange}
        name="radio"
        value="second"
        label="Second"
        checked
        error
    />
    <Help error>Foobar!</Help>
</FormControl>

Like so, or under the entire set?

2. Are we able to maniuplate the background color of the FieldSet? (I'm thinking light red bg-color for error etc)

Yes, but need to fix some problems with the legend being rendered slightly too high, outside the background color.

[cooper-joe]

1. I see the status text is appended to the label, are we unable to add the standard 'error' text (under the toggle controls)?

<FormControl>
    <Radio
        onChange={onChange}
        name="radio"
        value="second"
        label="Second"
        checked
        error
    />
    <Help error>Foobar!</Help>
</FormControl>

Like so, or under the entire set?

Under the entire set, ideally.

[varl]

@cooper-joe Yeah.

Inside or outside is possible:

In:

Out:

Also shows the legend bug and background-color.

[varl]

Related to: dhis2/ui-forms#25

Missing form components in the ui-forms issue:

• FieldSet: logical grouping of a set of fields with an optional label

  💭 Do statuses really make sense on a FieldSet?

• Field: a wrapper for form components to compose a complete field molecule with spacing

• Radio / Checkbox / Switch: could these always stack? Getting multiple columns of those elements can be solved by the app or ui-forms by using a flex container with two columns instead of using a prop to the FieldSet.

[cooper-joe]

* `FieldSet`: logical grouping of a set of fields with an optional label
  > 💭 Do statuses really make sense on a `FieldSet`?

Working on the status specs has highlighted the issues with applying a status to a FieldSet. See the example below. The status/error makes a lot of sense when using the FieldSet as a grouping for toggle controls like radio buttons. However, using a FieldSet to group multiple text inputs is a different conceptual model, now the FieldSet wraps multiple inputs that all have their own validation status. This may be confusing for users if they are presented with both of these types of FieldSets in a single form.

In this example, only one of the text inputs inside the FieldSet is returning an error, but is it immediately obvious which one? I haven't tested, but I think many users would assume that all controls inside the red/bordered area were invalid. This may be a case of adjusting the design to accomodate this flexibility, but I think it's important that we make a concious decision to allow this. Is this really what we intend with the FieldSet? I am not so sure myself, and I'm leaning towards this component being a more rigid ToggleGroup.

[varl]

@cooper-joe I agree with that assessment up to the ToggleGroup.

The FieldSet does not need to indicate any status, each component could do that on it's own:

This is what I expect from a generic FieldSet component.

If we want to highlight a field-set that has a problem, we could do that with a softer gray background. Or go for the red background. That could also be added as a feature later on when it is needed.

edit: What I'm proposing is that the FieldSet can be expanded on later with statuses etc. (and in a different PR), and for now it would only be for spacing and adding a label to a group of fields.

[varl]

@HendrikThePendric @cooper-joe @Mohammer5 check out this diff for what I mean.

I split out the Storybook for it for readability.

This is what I want from FieldSet right now, as simple as possible.

From here we can improve it and implement statuses, etc. in a controlled way to give some time & space for designing and implementing it at a slower pace.

[netlify]

Deploy preview for dhis2-ui-core ready!

Built with commit 0c6cbbf

https://deploy-preview-388--dhis2-ui-core.netlify.com

[cooper-joe]

@HendrikThePendric @cooper-joe @Mohammer5 check out this diff for what I mean.

I split out the Storybook for it for readability.

This is what I want from FieldSet right now, as simple as possible.

From here we can improve it and implement statuses, etc. in a controlled way to give some time & space for designing and implementing it at a slower pace.

I agree with this conclusion, the FieldSet should start as simple as possible while stilll being useful. The full design/usage spec needs to be completed before decisions about component composition can be tackled. Spacing and grouping are useful right now, so I vote that we start there.

[HendrikThePendric]

This PR should now be ready for final review. @Mohammer5 your review status is still "Changes requested". Could have a look if this looks OK now, and if so approve?

[varl]

Looks good to me! 👍

Thanks for bearing with the back and forth, next time we should start with the design spec. 💃

[HendrikThePendric]

Thanks for bearing with the back and forth, next time we should start with the design spec. 💃

Yeah, quite a lengthy process this way... But I am very happy with the outcome and that is much more important to me. Will merge

[dhis2-bot]

🎉 This PR is included in version 3.9.1 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀