Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 677
Make FormControl externally extensible (via HOC)#3621
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Uh oh!
There was an error while loading. Please reload this page.
Changes from all commits
4588ced1a491498c3822385ac7ebbbac1d264fc0d742de932544c1a6bac790dd10c6310c29e6529f40f3File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| --- | ||
| "@primer/react": patch | ||
| --- | ||
| Allow consumers to mark components as compatible with `FormControl` autowiring by wrapping them in `FormControl.autowirable` | ||
| <!-- Changed components: FormControl --> |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,23 +1,18 @@ | ||
| import React, {useContext} from 'react' | ||
| import Autocomplete from '../Autocomplete' | ||
| import Box from '../Box' | ||
| import Checkbox from '../Checkbox' | ||
| import Radio from '../Radio' | ||
| import Select from '../Select' | ||
| import TextInput from '../TextInput' | ||
| import TextInputWithTokens from '../TextInputWithTokens' | ||
| import Textarea from '../Textarea' | ||
| import {CheckboxOrRadioGroupContext} from '../internal/components/CheckboxOrRadioGroup' | ||
| import ValidationAnimationContainer from '../internal/components/ValidationAnimationContainer' | ||
| import {get} from '../constants' | ||
| import InlineAutocomplete from '../drafts/InlineAutocomplete' | ||
| import {useSlots} from '../hooks/useSlots' | ||
| import {CheckboxOrRadioGroupContext} from '../internal/components/CheckboxOrRadioGroup' | ||
| import ValidationAnimationContainer from '../internal/components/ValidationAnimationContainer' | ||
| import {SxProp} from '../sx' | ||
| import {useSSRSafeId} from '../utils/ssr' | ||
| import FormControlCaption from './_FormControlCaption' | ||
| import FormControlLabel from './_FormControlLabel' | ||
| import FormControlLeadingVisual from './_FormControlLeadingVisual' | ||
| import FormControlValidation from './_FormControlValidation' | ||
| import {autowirable, isValidAutowirableElement} from './autowirable' | ||
| export type FormControlProps = { | ||
| children?: React.ReactNode | ||
| @@ -55,43 +50,33 @@ const FormControl = React.forwardRef<HTMLDivElement, FormControlProps>( | ||
| leadingVisual: FormControlLeadingVisual, | ||
| validation: FormControlValidation, | ||
| }) | ||
| const expectedInputComponents = [ | ||
| Autocomplete, | ||
| Checkbox, | ||
| Radio, | ||
| Select, | ||
| TextInput, | ||
| TextInputWithTokens, | ||
| Textarea, | ||
| InlineAutocomplete, | ||
| ] | ||
| const choiceGroupContext = useContext(CheckboxOrRadioGroupContext) | ||
| const disabled = choiceGroupContext.disabled || disabledProp | ||
| const id = useSSRSafeId(idProp) | ||
| const validationMessageId = slots.validation ? `${id}-validationMessage` : undefined | ||
| const captionId = slots.caption ? `${id}-caption` : undefined | ||
| const validationStatus = slots.validation?.props.variant | ||
| const InputComponent = childrenWithoutSlots.find(child => | ||
| expectedInputComponents.some(inputComponent => React.isValidElement(child) && child.type === inputComponent), | ||
| ) | ||
| const inputProps = React.isValidElement(InputComponent) && InputComponent.props | ||
| const InputComponent = childrenWithoutSlots.find(isValidAutowirableElement) | ||
| // we don't make the mark symbol public, so TS thinks it's impossible that the types could overlap | ||
| const isChoiceInput = | ||
| React.isValidElement(InputComponent) && (InputComponent.type === Checkbox || InputComponent.type === Radio) | ||
| InputComponent && ((InputComponent.type as unknown) === Checkbox || (InputComponent.type as unknown) === Radio) | ||
| if (InputComponent) { | ||
| if (inputProps?.id) { | ||
| const inputProps = InputComponent.props | ||
| if (inputProps.id) { | ||
| // eslint-disable-next-line no-console | ||
| console.warn( | ||
| `instead of passing the 'id' prop directly to the input component, it should be passed to the parent component, <FormControl>`, | ||
| ) | ||
| } | ||
| if (inputProps?.disabled) { | ||
| if (inputProps.disabled) { | ||
| // eslint-disable-next-line no-console | ||
| console.warn( | ||
| `instead of passing the 'disabled' prop directly to the input component, it should be passed to the parent component, <FormControl>`, | ||
| ) | ||
| } | ||
| if (inputProps?.required) { | ||
| if (inputProps.required) { | ||
| // eslint-disable-next-line no-console | ||
| console.warn( | ||
| `instead of passing the 'required' prop directly to the input component, it should be passed to the parent component, <FormControl>`, | ||
| @@ -197,25 +182,16 @@ const FormControl = React.forwardRef<HTMLDivElement, FormControlProps>( | ||
| sx={{...(isLabelHidden ? {'> *:not(label) + *': {marginTop: 1}} : {'> * + *': {marginTop: 1}}), ...sx}} | ||
| > | ||
| {slots.label} | ||
| {React.isValidElement(InputComponent) && | ||
mattcosta7 marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| React.cloneElement( | ||
| InputComponent, | ||
| Object.assign( | ||
| { | ||
| id, | ||
| required, | ||
| disabled, | ||
| validationStatus, | ||
| ['aria-describedby']: [validationMessageId, captionId].filter(Boolean).join(' '), | ||
| }, | ||
| InputComponent.props, | ||
| ), | ||
| )} | ||
| {childrenWithoutSlots.filter( | ||
| child => | ||
| React.isValidElement(child) && | ||
mattcosta7 marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| !expectedInputComponents.some(inputComponent => child.type === inputComponent), | ||
| )} | ||
| {InputComponent && | ||
| React.cloneElement(InputComponent, { | ||
| id, | ||
| required, | ||
| disabled, | ||
| validationStatus, | ||
| ['aria-describedby']: [validationMessageId, captionId].filter(Boolean).join(' '), | ||
| ...InputComponent.props, | ||
| })} | ||
| {childrenWithoutSlots.filter(child => child !== InputComponent)} | ||
Comment on lines
+185
to
+194
ContributorAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. An advantage of this approach is that it's internally much easier to typecheck, allowing for significantly reduced boilerplate to satisfy the compiler. This is because the | ||
| {slots.validation ? ( | ||
| <ValidationAnimationContainer show>{slots.validation}</ValidationAnimationContainer> | ||
| ) : null} | ||
| @@ -232,4 +208,5 @@ export default Object.assign(FormControl, { | ||
| Label: FormControlLabel, | ||
| LeadingVisual: FormControlLeadingVisual, | ||
| Validation: FormControlValidation, | ||
| autowirable, | ||
| }) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,41 @@ | ||
| import {ComponentType, FC, ReactElement, isValidElement} from 'react' | ||
| import {FormValidationStatus} from '../utils/types/FormValidationStatus' | ||
| /** | ||
| * Presence of this symbol on a component indicates that it supports `FormControl``s auto-wiring features. | ||
| * By not exporting this symbol we force consumers to use the HOC wrapper function, ensuring that the prop types align. | ||
| */ | ||
| const supportsAutowiring = Symbol('FormControl.supportsAutowiring') | ||
| /** | ||
| * When a supported component is used as a child of `FormControl`, it will be cloned and these props will be | ||
| * overidden to auto-wire accessibility features. These props should be forwarded to the underlying form control, | ||
| * typically a Primer component. | ||
| */ | ||
| export interface FormControlForwardedProps { | ||
| id?: string | ||
| required?: boolean | ||
| disabled?: boolean | ||
| ['aria-describedby']?: string | ||
| validationStatus?: FormValidationStatus | ||
| } | ||
| type AutowirableComponent = FC<FormControlForwardedProps> & { | ||
| [supportsAutowiring]: unknown | ||
| } | ||
| type AutowirableElement = ReactElement<FormControlForwardedProps, AutowirableComponent> | ||
| /** | ||
| * Mark a component to indicate that it supports `FormControl` autowiring by forwarding the props in | ||
| * `FormControlAutowireComponentProps` to an underlying form element or component. This is useful when | ||
| * wrapping/extending Primer form controls to make them easier to use. | ||
| */ | ||
| // use explicit return type to hide the `supportsAutoWiring` symbol from the public API | ||
| export function autowirable<P extends FormControlForwardedProps>(component: ComponentType<P>): ComponentType<P> { | ||
| return Object.assign(component, {[supportsAutowiring]: true}) | ||
| } | ||
| export function isValidAutowirableElement(object: Parameters<typeof isValidElement>[0]): object is AutowirableElement { | ||
| return isValidElement(object) && Object.getOwnPropertySymbols(object.type).includes(supportsAutowiring) | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1,2 @@ | ||
| export {default} from './FormControl' | ||
| export type {FormControlForwardedProps} from './autowirable' |
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This issue was already present - I just noticed it while working here and decided to call it out explicitly