Make FormControl externally extensible (via hook)

[iansan5653]

Followup from discussion in #3621

Motivation

FormControl supports an 'auto-wiring' functionality where it automatically applies certain props to the wrapped input in order to make accessibility easier. For example, it automatically applies an id prop (if none is provided) to link the wrapped input to the Label.

At present, this is only supported for a limited subset of components, defined internally:

react/src/FormControl/FormControl.tsx

Lines 58 to 67 in 65b888c

	const expectedInputComponents = [
	Autocomplete,
	Checkbox,
	Radio,
	Select,
	TextInput,
	TextInputWithTokens,
	Textarea,
	InlineAutocomplete,
	]

Consumers of the library cannot add to this list, so it's impossible for consumers to create their own components that support integration with FormControl.

As I work on moving the InlineAutocomplete component out of this repository, we either need to make this functionality externally extensible or we have to break the InlineAutocomplete component and require it to be manually wired up whenever used. I think the first one is much more desirable as it's generally useful for all component authoring.

Solution

I exposed a new hook on the FormControl API: FormControl.useForwardedProps. This hook is given the external props of the component and handles merging them together and building the aria-describedby attribute.

I honestly think this is much cleaner a far preferable solution vs the one proposed in #3621. There are few downsides, and it's much more idiomatic React. We can avoid using the Children API and the relationship between FormControl and child feels much more natural and robust.

A huge advantage of this approach is that we don't need to forward props down through children - since it's context-based, supported components can be deeply nested.

The only downsides I can think of are:

1. It's a little less readable to have to wrap the props in a hook. This is pretty minor though and it's not something most users will have to care about at all
2. We have no way of preventing the id from getting assigned to multiple components. We really want to error if two components in one FormControl are both calling the hook, but I can't think of a clean way to do that. However, this is a pretty rare edge case and I don't think most users will actually try rendering multiple components in a single FormControl

[changeset-bot]

🦋 Changeset detected

Latest commit: 51b5e83

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	100.49 KB (-3.95% 🔽)
dist/browser.umd.js	101.04 KB (-3.93% 🔽)

[iansan5653]

There's no good way to detect if the wrapped component is a Checkbox or Radio without referencing them directly. I don't like this since it means we still have to have this backwards data flow from children to parent, but I don't have an elegant solution to that problem.

We can't handle this inside the hook (ie, by adding an isChoiceInput parameter) because the hook doesn't have access to the slots these checks refer to.

[joshblack]

Is the slot info something that could be passed along in the context value, potentially? Or does that not really work?

[iansan5653]

I just tried this and honestly I don't think it makes it any cleaner. We still have to look through the children to figure out if it's a choice input, so all that changes is that we pass the slots info down. I think since these checks don't reference props, it makes sense to leave them here for now.

[iansan5653]

This change also fixes an apparently previously overlooked bug where horizontal-layout FormControl would not forward the validationStatus or required props

[mperrotti]

This was not a bug, this was intentional. You can't require an individual Radio or Checkbox, and they can't have their own validation status.

Instead, you handle those on the CheckboxGroup or RadioGroup level.

See the following documentation for more info:

• https://primer.style/design/ui-patterns/forms/overview#validation-message
• https://primer.style/design/ui-patterns/forms/overview#validation-message:~:text=An%20individual%20checkbox%20or%20radio%20button%20cannot%20be%20marked%20as%20required.

[iansan5653]

One downside of this is that if two components inside the same FormControl call this hook, they could have the same ID. But that's an invalid structure anyway - FormControl is singular so this shouldn't be a problem.

[iansan5653]

It's debatable whether this should even be publicly exposed with this approach:

On one hand, this is almost never necessary. Prior to this change, wrapping a Primer form input in another component (ie, if you want to make a NumericTextInput component) would have broken the FormControl wiring. But with context, this doesn't happen. As long as the underlying component is a Primer form input, it doesn't matter how deeply nested it is. If we don't expose this hook, we can push users to always build around Primer components and not HTML from scratch.

On the other hand, publicly exposing it allows consumers to build custom form inputs from plain HTML. Maybe that's not something we want to prevent. For example, they could build a new RangeInput component, which Primer doesn't have yet.

[iansan5653]

I feel more strongly about exposing this after looking into the ongoing work around Filter and QueryBuilder - more complex inputs that definitely could benefit from being able to directly access this info.

[joshblack]

I'm definitely a fan of making this public, depending on how stable this ends up being maybe it makes sense in the drafts/experimental entrypoint until it's further validated?

I think something that was surprising initially was seeing this on the component namespace. Could hooks be something that is just accessed from the entrypoint instead of tying it to a specific component? e.g. import { useFormControl } from '@primer/react'; or is the namespace helpful here in the same way that it helps with sub-components?

[iansan5653]

We definitely could expose the hook directly - I just figured the namespace was an obvious place to look for it. I am good with either one

[mperrotti]

I slightly prefer exposing the hook directly. Usually the properties on a component "object" are other components.

[iansan5653]

I wonder if these warnings should be suppressible (or just removed altogether). I kept them to be consistent with current behavior, but I do think there are valid extensibility cases where you might not want these warnings.

For example, a DatePicker might have a TextInput as its anchor. The DatePicker may want to control exactly how these forwarded props are handled, since a datepicker is a much more complicated component than just a text input. So the DatePicker would override the TextInput forwarded props, causing the warning to occur because the TextInput is still calling this hook.

One possible way to suppress this would be for the DatePicker to wrap its TextInput in a FormControlContext.Provider value={null}. But then we'd have to expose FormControlContext and I don't think we want to do that.

[joshblack]

I love the example that you brought up, I added a comment on our v37 release issue to try and capture this: https://github.com/github/primer/issues/1715#issuecomment-1682938944 as this is likely something that will stick around for v35 and v36.

Feel free to edit/add anything to the comment if you feel like there are other examples or situations that really demonstrate why this change is important 👀

[iansan5653]

Do we like this naming? "Forwarded props" vs "external props"? It reminds me of "forwarded refs" which also come from the parent.

[mperrotti]

I like this naming. It's clear and feels familiar.

[iansan5653]

Forwarding isn't necessary to make it work with FormControl anymore since the underlying TextInput or Textarea will handle it, but I still forward the HTML props just for backwards compat

[iansan5653]

• Add useFormControlForwardedProps examples to Storybook

I see a few hooks in the Storybook, but they are all visual. I have a hard time seeing how I might make a story for this hook since it does not have a visual effect. Do you have anything in mind?

[mperrotti]

@iansan5653 - it's ok if there's no visual effect. I just think it would help Primer consumers discover that hook and see an example of how it could be used. I'd recommend adding something like this in the "Hooks" section of Storybook:

export default {
  title: 'Hooks/useFormControlForwardedProps',
  decorators: [
    Story => {
      return (
        <ThemeProvider>
          <BaseStyles>{Story()}</BaseStyles>
        </ThemeProvider>
      )
    },
  ],
  argTypes: {
    disabled: {
      type: 'boolean',
      defaultValue: false,
    },
    id: {
      type: 'string',
      defaultValue: 'some-id',
    },
    placeholder: {
      type: 'string',
      defaultValue: 'Placeholder',
    },
    required: {
      type: 'boolean',
      defaultValue: false,
    },
    validationStatus: {
      defaultValue: undefined,
      options: ['error', 'success', 'warning', undefined],
      control: {type: 'radio'},
    },
    // whatever else you might want to pass to useFormControlForwardedProps
  },
} as Meta

export const UseFormControlForwardedProps = args => {
  const {
    disabled,
    id,
    required,
    validationStatus,
    ['aria-describedby']: ariaDescribedBy,
    ...externalProps
  } = useFormControlForwardedProps(args)

  return (
    <>
      <label htmlFor={id}>Label {required ? '*' : undefined}</label>
      <input
        id={id}
        disabled={disabled}
        required={required}
        aria-invalid={validationStatus === 'invalid'}
        aria-describedby={ariaDescribedBy}
        {...externalProps}
      />
      <div id={ariaDescribedBy}>Some caption</div>
    </>
  )
}

or if you just want to demonstrate what gets returned by the forwarded props, you could just render the result into a table or something:

export const UseFormControlForwardedProps = args => {
  const forwardedProps = useFormControlForwardedProps(args)

  return (
    <table>
      <thead>
        <tr>
          <th>Forwarded prop</th>
          <th>Value</th>
        </tr>
      </thead>
      <tbody>
        {Object.entries(forwardedProps).map(([key, value]) => (
          <tr key={key}>
            <td>{key}</td>
            <td>{value}</td>
          </tr>
        ))}
      </tbody>
    </table>
  )
}

[iansan5653]

@mperrotti Done -- added a story, tests, and directly exposed the hook