Adds CheckboxGroup and RadioGroup components to replace ChoiceFieldset

[mperrotti]

Closes https://github.com/github/primer/issues/694

Screenshots

Merge checklist

• Added/updated tests
• Added/updated documentation
• Tested in Chrome
• Tested in Firefox
• Tested in Safari
• Tested in Edge

Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.

[changeset-bot]

🦋 Changeset detected

Latest commit: 6e0c553

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@primer/react	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[mperrotti]

Alternative API to consider

I think the API works as it is, but I think it could be improved by splitting this into CheckboxGroup and RadioGroup.

Check out the Loom for a more detailed explanation: https://www.loom.com/share/3cf3340ebfd54b469750eaf8debe1d23

Current API

<ChoiceGroup>
  <ChoiceGroup.Label>Radios</ChoiceGroup.Label>
  <ChoiceGroup.Caption>These are some radio inputs</ChoiceGroup.Caption>
  <FormControl>
    <Radio name="radioChoices" value="radioOne" onChange={handleRadioChange} />
    <FormControl.Label>Radio one</FormControl.Label>
  </FormControl>
  <FormControl>
    <Radio name="radioChoices" value="radioTwo" onChange={handleRadioChange} />
    <FormControl.Label>Radio two</FormControl.Label>
  </FormControl>
  <FormControl>
    <Radio name="radioChoices" value="radioThree" onChange={handleRadioChange} />
    <FormControl.Label>Radio three</FormControl.Label>
  </FormControl>

  {hasError && <ChoiceGroup.Validation>Some error message</ChoiceGroup.Validation>}
</ChoiceGroup>

PROS:

• 1 component to handle a checkbox group and a radio group
• Passing onChange to each Radio is consistent with ActionList.Item where we have to pass onSelect to each ActionList.Item
• It feels less "magic" to manually pass name to each Radio

CONS:

• You have to pass the same onChange to each Checkbox or Radio
• You have to pass the same name to each Radio
• Component name isn't as obvious

---

Alternative API: CheckboxGroup / RadioGroup

RadioGroup

type RadioGroupProps = {
    name: string
    onChange?: (value: string) => void
}

// ⬇️ usage example ⬇️

<RadioGroup onChange={(value) => { setSelectedRadioValue(value) }} name="radioInputGroup">
    <RadioGroup.Label>Radios</RadioGroup.Label>
    <RadioGroup.Caption>These are some radio inputs</RadioGroup.Caption>
    <FormControl>
        <Radio value="choiceOne" onChange={(e) => { console.log('input data:', e.currentTarget) }} />
        <FormControl.Label>Selectable choice</FormControl.Label>
    </FormControl>
    <FormControl>
        <Radio value="choiceTwo" />
        <FormControl.Label>Selectable choice</FormControl.Label>
    </FormControl>
    <FormControl>
        <Radio value="choiceThree" />
        <FormControl.Label>Selectable choice</FormControl.Label>
    </FormControl>
    {hasError && <RadioGroup.Validation>Some error message</RadioGroup.Validation>}
</RadioGroup>

CheckboxGroup

type CheckboxGroupProps = {
    onChange?: (values: string[]) => void
}

// ⬇️ usage example ⬇️

<CheckboxGroup onChange={(values) => { setSelectedCheckboxValues(values) }}>
    <CheckboxGroup.Label>Checkboxes</CheckboxGroup.Label>
    <CheckboxGroup.Caption>These are some checkbox inputs</CheckboxGroup.Caption>
    <FormControl>
        <Checkbox value="choiceOne" onChange={(e) => { console.log('input data:', e.currentTarget) }} />
        <FormControl.Label>Selectable choice</FormControl.Label>
    </FormControl>
    <FormControl>
        <Checkbox value="choiceTwo" />
        <FormControl.Label>Selectable choice</FormControl.Label>
    </FormControl>
    <FormControl>
        <Checkbox value="choiceThree" />
        <FormControl.Label>Selectable choice</FormControl.Label>
    </FormControl>
    {hasError && <CheckboxGroup.Validation>Some error message</CheckboxGroup.Validation>}
</CheckboxGroup>

PROS:

• If variants are introduced to radio groups that aren't available to checkbox groups (or vice versa), it will be easier to introduce them without breaking anything
• Don't have to pass the same onChange to each Checkbox or Radio (but you could if you wanted)
• Don't have to pass the same name to each Radio
• Component names are more obvious

CONS:

• 2 components instead of 1
• Passing onChange to the parent is inconsistent with ActionList which only lets you pass onSelect to each ActionList.Item
• Would require adding context in Radio and Checkbox components
• name prop would no longer be required on Radio

[github-actions]

size-limit report 📦

Path	Size
dist/browser.esm.js	64.18 KB (+2.23% 🔺)
dist/browser.umd.js	64.54 KB (+2.17% 🔺)

[rezrah]

Check out the Loom for a more detailed explanation: https://www.loom.com/share/3cf3340ebfd54b469750eaf8debe1d23

❤️ the video walkthrough @mperrotti

Here are some initial thoughts...

Naming

Agree on naming concerns. As an alternative suggestion, how about FormControlGroup with a prop to let you enable optimial layout of any type of form input? While separate components are nice, it could arguably lessen the cognitive burden to have one component that can handle all group layouts. I also like that this would also let you infer the relationship to FormControl children immediately via naming, so making it feel more natural wrap around n FormControls. We can also extend behaviour to support layouts for other form components. This could be the go to component for grouped inputs of any type.

<FormControlGroup variant="radio">
  <FormControlGroup.Label>Radios</FormControlGroup.Label>
  <FormControlGroup.Caption>These are some radio inputs</FormControlGroup.Caption>
  <FormControl>
    <Radio name="radioChoices" value="radioOne" onChange={handleRadioChange} />
    <FormControl.Label>Radio one</FormControl.Label>
  </FormControl>
  <FormControl>
    <Radio name="radioChoices" value="radioTwo" onChange={handleRadioChange} />
    <FormControl.Label>Radio two</FormControl.Label>
  </FormControl>
  <FormControl>
    <Radio name="radioChoices" value="radioThree" onChange={handleRadioChange} />
    <FormControl.Label>Radio three</FormControl.Label>
  </FormControl>

  {hasError && <FormControlGroup.Validation>Some error message</FormControlGroup.Validation>}
</FormControlGroup>

Repeating props like onChange

Given we've opted for less magic under-the-hood in other places, it would be more consistent to continue with this more declarative and explicit pattern despite it being more verbose.

I loved the idea that you can override a one handler in particular, but keep the others inheriting from the parent. One thing that makes me nervous though, is using Context to enable this behaviour. It feels like a very expensive in-memory solution to a problem that only really provides ergonomic benefits? I’d love to understand the trade-offs a bit more though.

Splitting into two separate components

I like the idea, but the reasons you mentioned felt like we were engineering for hypothetical, future scenarios which might not play out? Would it be better to wait for those new patterns to land first?

[mperrotti]

As an alternative suggestion, how about FormControlGroup with a prop to let you enable optimial layout of any type of form input?

• I'm not sure we're going to have a component for grouping other kinds of inputs. If we do, they might have different functionality and require different sets of props.
• We explicitly got rid of variant="choice | stack" on FormControl`, so I'm surprised to see that as a suggestion here.

---

I like the idea, but the reasons you mentioned felt like we were engineering for hypothetical, future scenarios which might not play out? Would it be better to wait for those new patterns to land first?

I agree—we can wait for those new patterns to land first.

---

Given we've opted for less magic under-the-hood in other places, it would be more consistent to continue with this more declarative and explicit pattern despite it being more verbose.

I'm fine with this. We can always add an onChange to the parent later if we need to. It's easier to add than take away :)

[rezrah]

I'm not sure we're going to have a component for grouping other kinds of inputs

👍 . I'm struggling with a better name suggestion that only applies to checkboxes and radio's. I'm also coming round to CheckboxGroup and RadioGroup a bit more now tbh as they are at least obvious about their usage.

You listed context as a con if you did that, is Context needed? Can you use local state instead for example? You also listed onChange inconsistencies and name props as cons in the 2 components approach. Does it need to do all of that? If the CheckboxGroup and RadioGroup are just handling layout for example, and not doing much magic internally, those cons aren't cons anymore.

We explicitly got rid of variant="choice | stack" on FormControl`, so I'm surprised to see that as a suggestion here

In that example I'm suggesting radio, checkbox, toggles as values instead of choice and stack to help FormControlGroup decide on suitable layout depending on type on inputs.

If I recall, we also dropped it previously as we don't support custom input components in FormControl, but if we did, this type of prop might be needed.

[rezrah]

Shall we update the FormControl radio and checkbox example here to use this component? These examples feel very similar now. Maybe adding a Flash or internal link pointing to ChoiceGroup so users know it exists.

[mperrotti]

Yes, definitely! Good catch.

[rezrah]

Should this one be merged into the basic example to appear alongside it? It feels like a 'basic' example to but quite tucked away in the examples.

[mperrotti]

Yea, good suggestion.

[rezrah]

FYI some cursor inconsistencies here

Screen.Recording.2022-02-16.at.20.33.26.mov

[mperrotti]

Good catch. I'll fix that.

[rezrah]

Would we ever want to allow the colour of the asterisk to be changed? For example, on some forms you'll see it in red for extra prominence.

[mperrotti]

As of now, we do not. The forms as they exist today are totally custom and not following any kind of system.

[rezrah]

Although it's visually hidden, could this become an issue for both screen readers and crawlers that you're duplicating the validation message markup here?

For screen-readers specifically, it's probably worth adding an aria-hidden here.

☝️ Just noticed you added it to the one at the bottom

[rezrah]

Also, could this be guarded? So it doesn't generate an empty tag if there's no validation message yet?

[mperrotti]

Good call about making this guarded. Will do 👍

[mperrotti]

I'm also coming round to CheckboxGroup and RadioGroup a bit more now tbh as they are at least obvious about their usage.

In that case, I think I might give it a try.

---

You listed context as a con if you did that, is Context needed? Can you use local state instead for example?

I could attempt to do it with local state instead of Context, but I'm not sure if it will be possible. We need a way to get the onChange handler from the parent to the Radio and/or Checkbox child.

---

You also listed onChange inconsistencies and name props as cons in the 2 components approach. Does it need to do all of that? If the CheckboxGroup and RadioGroup are just handling layout for example, and not doing much magic internally, those cons aren't cons anymore.

The only reason I think that onChange on the parent is a con is because it's inconsistent with ActionList. I actually think this would be a nice addition to the ActionList API cc @siddharthkp

It doesn't need to do that extra stuff, but I think it would be a shame not to.

[colebemis]

I'm liking this API. Nice work, @mperrotti!

I skimmed the docs and the code and everything looks good to me 👍