Add FormControl component

[mperrotti]

The FormControl component combines the functionality of InputField and ChoiceInputField, and will replace FormGroup, InputField, and ChoiceInputField.

Relates to https://github.com/github/primer/issues/679

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

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

[github-actions]

size-limit report 📦

Path	Size
dist/browser.esm.js	62.5 KB (+1.4% 🔺)
dist/browser.umd.js	62.88 KB (+1.42% 🔺)

[rezrah]

@mperrotti - not sure if it's just me but on the preview url I can't see any labels. Any ideas why?

https://primer-react-efgapobji-primer.vercel.app/react/FormControl

[mperrotti]

Should we just go with "stack" for the vertically stacked label+input+caption/validation? Or does anybody have an idea for a better name?

[rezrah]

Could it be more abstract like vertical and horizontal with prop called alignment instead?

Taking that further, do we need a variant prop at all?

E.g.

      <FormControl variant="stack">
        <FormControl.Label>Checkbox option one</FormControl.Label>
        <Checkbox />
      </FormControl>

☝️ This is incorrect usage for checkboxes and breakslayout, but is what happens by default unless the user manually changes to choice.

Can we read children via React.Children to see if there's a type of Checkbox or Radio, which automatically applies choice for them without needing it via prop?

[colebemis]

@rezrah We discussed that after you left one of our meetings. We decided that reading from React.Children felt like a little too much magic and would prevent people from using custom checkboxes/radios

[rezrah]

Thanks @colebemis. Should the values be more explicit to the type of child instead then? It's not immediately clear (to me at least) which values (stacked/choice) to use for variant, even though there are restrictions on types of input to variant. E.g. You can't use stacked with checkbox children.

Example of explicit values...

      <FormControl variant="checkbox">
        <FormControl.Label>Checkbox option one</FormControl.Label>
        <Checkbox />
      </FormControl>

Although I personally find for even more readable...

      <FormControl for="checkbox">
        <FormControl.Label>Checkbox option one</FormControl.Label>
        <Checkbox />
      </FormControl>

[mperrotti]

@colebemis - I went with Reza's suggestion to remove variant. I won't merge this until you either say this is ok, or we come up with a solution everybody is comfortable with.

[rezrah]

Given this component only supports internal form inputs, I think it makes sense to remove variant. Less customisation for the user to worry about.

[siddharthkp]

Given this component only supports internal form inputs, I think it makes sense to remove variant.

When we were looking at the API, we talked about supporting custom form inputs and that's where an explicit layout variant came from, because there is no non-hacky way for the user to get a vertical layout on their own.

If we are choosing to not support custom input elements for now, we can remove the variant prop. I'm always up for a little magic. I imagine the need for custom inputs will come up at some point and then we can bring back the variant prop, it's nice that we already have a solution.

[colebemis]

This is excellent! ✨

I left a lot of comments but they're mostly nitpicks or questions. I didn't notice any serious blockers

[colebemis]

Not a blocker for this PR:

@mperrotti and @siddharthkp how do y'all feel about not capitalizing slot names (slots.leadingVisual instead of slots.LeadingVisual)? The PascalCase makes me think I can do something like this <slots.LeadingVisual> (which wouldn't work)

[siddharthkp]

that's a good point, I'm not sure 🤔

I called it slots.LeadingVisual in ActionList because I wanted to call it the exact same name from the component API. <ActionList.LeadingVisual> → slots.LeadingVisual

const {Slots, Slot} = createSlots(['LeadingVisual', 'InlineDescription', 'BlockDescription', 'TrailingVisual'])

https://github.com/primer/react/blob/main/src/ActionList2/Item.tsx#L79

Side note: It would be really cool if we could actually call the render the slot with <Slots.LeadingVisual props="too"> but I couldn't figure out how to make that work

[mperrotti]

I'm cool with that change, but I'd rather make it across all components instead of just this one.

[siddharthkp]

I'd like to keep slots.LeadingVisual until we know what changes in #1690 in the hopes we can actually have <Slots.LeadingVisual props="too"> 😅

[colebemis]

Does this implementation prevent people from passing custom input components?

[mperrotti]

Yes. We can open it up later if we need to.

[rezrah]

❤️