Skip to content
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

fix(FormControl): allow required check boxes #5027

5 changes: 5 additions & 0 deletions .changeset/wild-maps-grow.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@primer/react": patch
---

fix(FormControl): allow required check boxes in CheckboxGroup
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ Playground.argTypes = {
export const Default = () => (
<CheckboxGroup>
<CheckboxGroup.Label>Choices</CheckboxGroup.Label>
<FormControl>
<FormControl required>
<Checkbox value="one" defaultChecked />
<FormControl.Label>Choice one</FormControl.Label>
</FormControl>
Expand Down
63 changes: 29 additions & 34 deletions packages/react/src/FormControl/FormControl.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import FormControlLabel from './_FormControlLabel'
import FormControlLeadingVisual from './_FormControlLeadingVisual'
import FormControlValidation from './_FormControlValidation'
import {FormControlContextProvider} from './_FormControlContext'
import {warning} from '../utils/warning'

export type FormControlProps = {
children?: React.ReactNode
Expand Down Expand Up @@ -62,26 +63,21 @@ const FormControl = React.forwardRef<HTMLDivElement, FormControlProps>(
const inputProps = React.isValidElement(InputComponent) && InputComponent.props
const isChoiceInput =
React.isValidElement(InputComponent) && (InputComponent.type === Checkbox || InputComponent.type === Radio)
const isRadioInput = React.isValidElement(InputComponent) && InputComponent.type === Radio

if (InputComponent) {
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) {
// 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) {
// 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>`,
)
}
warning(
inputProps?.id,
`instead of passing the 'id' prop directly to the input component, it should be passed to the parent component, <FormControl>`,
)
warning(
inputProps?.disabled,
`instead of passing the 'disabled' prop directly to the input component, it should be passed to the parent component, <FormControl>`,
)
warning(
inputProps?.required,
`instead of passing the 'required' prop directly to the input component, it should be passed to the parent component, <FormControl>`,
)
}

if (!slots.label) {
Expand All @@ -92,24 +88,20 @@ const FormControl = React.forwardRef<HTMLDivElement, FormControlProps>(
}

if (isChoiceInput) {
if (slots.validation) {
// eslint-disable-next-line no-console
console.warn(
'Validation messages are not rendered for an individual checkbox or radio. The validation message should be shown for all options.',
)
}
warning(
!!slots.validation,
'Validation messages are not rendered for an individual checkbox or radio. The validation message should be shown for all options.',
)

if (childrenWithoutSlots.find(child => React.isValidElement(child) && child.props?.required)) {
// eslint-disable-next-line no-console
console.warn('An individual checkbox or radio cannot be a required field.')
}
warning(
isRadioInput && childrenWithoutSlots.find(child => React.isValidElement(child) && child.props?.required),
'An individual radio cannot be a required field.',
)
} else {
if (slots.leadingVisual) {
// eslint-disable-next-line no-console
console.warn(
'A leading visual is only rendered for a checkbox or radio form control. If you want to render a leading visual inside of your input, check if your input supports a leading visual.',
)
}
warning(
!!slots.leadingVisual,
'A leading visual is only rendered for a checkbox or radio form control. If you want to render a leading visual inside of your input, check if your input supports a leading visual.',
)
}

const isLabelHidden = slots.label?.props.visuallyHidden
Expand Down Expand Up @@ -138,11 +130,14 @@ const FormControl = React.forwardRef<HTMLDivElement, FormControlProps>(
InputComponent as React.ReactElement<{
id: string
disabled: boolean
required: boolean
['aria-describedby']: string
}>,
{
id,
disabled,
// allow checkboxes to be required
required: required && !isRadioInput,
['aria-describedby']: captionId as string,
},
)}
Expand Down
53 changes: 51 additions & 2 deletions packages/react/src/__tests__/FormControl.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,9 @@ import axe from 'axe-core'
import {
Autocomplete,
Checkbox,
CheckboxGroup,
FormControl,
Radio,
Select,
Textarea,
TextInput,
Expand Down Expand Up @@ -376,20 +378,67 @@ describe('FormControl', () => {
consoleSpy.mockRestore()
})

it('should warn users if they pass `required` to a checkbox or radio', async () => {
it('should warn users if they pass `required` to a radio', async () => {
const consoleSpy = jest.spyOn(global.console, 'warn').mockImplementation()

render(
<FormControl required>
<FormControl.Label>{LABEL_TEXT}</FormControl.Label>
<Checkbox required />
<Radio value="radio" name="radio" required />
<FormControl.Caption>{CAPTION_TEXT}</FormControl.Caption>
</FormControl>,
)

expect(consoleSpy).toHaveBeenCalled()
consoleSpy.mockRestore()
})

it('should allow required prop to individual checkbox', async () => {
const {getByRole} = render(
<FormControl required>
broccolinisoup marked this conversation as resolved.
Show resolved Hide resolved
<FormControl.Label>{LABEL_TEXT}</FormControl.Label>
<Checkbox />
<FormControl.Caption>{CAPTION_TEXT}</FormControl.Caption>
</FormControl>,
)

expect(getByRole('checkbox')).toBeRequired()
})

it('should not add required prop to individual radio', async () => {
const {getByRole} = render(
<FormControl required>
<FormControl.Label>{LABEL_TEXT}</FormControl.Label>
<Radio value="radio" name="radio" />
<FormControl.Caption>{CAPTION_TEXT}</FormControl.Caption>
</FormControl>,
)

expect(getByRole('radio')).not.toBeRequired()
})

it('should allow required prop on checkbox if part of CheckboxGroup', async () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add a test for "should allow required prop on radio button if part of RadioButtonGroup"?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's my understanding from this that a radio cannot be required, not even when within a RadioButtonGroup, just the group itself can be required (at the top level). I can add a test for "does not allow required prop on radio button when part of RadioButtonGroup" if you think that'd be worth it

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No worries, all good!

const {getByTestId} = render(
<CheckboxGroup>
<CheckboxGroup.Label>Checkboxes</CheckboxGroup.Label>
<FormControl required>
<Checkbox value="checkOne" data-testid="checkbox-1" />
<FormControl.Label>Checkbox one</FormControl.Label>
</FormControl>
<FormControl>
<Checkbox value="checkTwo" data-testid="checkbox-2" />
<FormControl.Label>Checkbox two</FormControl.Label>
</FormControl>
<FormControl>
<Checkbox value="checkThree" />
<FormControl.Label>Checkbox three</FormControl.Label>
</FormControl>
</CheckboxGroup>,
)

expect(getByTestId('checkbox-1')).toBeRequired()
expect(getByTestId('checkbox-2')).not.toBeRequired()
})
})

it('should have no axe violations', async () => {
Expand Down
Loading