Skip to content

Commit

Permalink
removes variant prop
Browse files Browse the repository at this point in the history
  • Loading branch information
@mperrotti
mperrotti committed Feb 8, 2022
1 parent 90033a8 commit 260ca68
Show file tree
Hide file tree
Showing 4 changed files with 27 additions and 61 deletions.
28 changes: 10 additions & 18 deletions docs/content/FormControl.mdx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -82,8 +82,6 @@ render(DifferentInputs)

### With checkbox and radio inputs

Use `variant="choice"` to position checkbox and radio inputs to the left of the label.

```javascript live noinline
const DifferentInputs = () => {
const [tokens, setTokens] = React.useState([
Expand All @@ -97,16 +95,16 @@ const DifferentInputs = () => {

return (
<Box display="grid" gridGap={1}>
<FormControl variant="choice">
<FormControl>
<FormControl.Label>Checkbox option one</FormControl.Label>
<Checkbox />
</FormControl>
<fieldset style={{marginLeft: 0, marginRight: 0, padding: 0, border: 0}}>
<FormControl variant="choice">
<FormControl>
<FormControl.Label>Radio option one</FormControl.Label>
<Radio name="radioExample" value="one" />
</FormControl>
<FormControl variant="choice">
<FormControl>
<FormControl.Label>Radio option two</FormControl.Label>
<Radio name="radioExample" value="two" />
</FormControl>
Expand Down Expand Up @@ -141,7 +139,7 @@ render(DifferentInputs)
<TextInput />
</FormControl>

<FormControl variant="choice" disabled>
<FormControl disabled>
<FormControl.Label>Checkbox option</FormControl.Label>
<Checkbox />
</FormControl>
Expand All @@ -152,7 +150,7 @@ render(DifferentInputs)

<Note>

We encourage using `FormControl` alongside all standalone form components like [`TextInput`](/TextInput), as every input must have a corresponding label to be accessible to assistive technology.
We encourage using `FormControl` alongside all standalone form components like [`TextInput`](/TextInput), as every input must have a corresponding label to be accessible to assistive technology.
``

`FormControl` also provides an interface for showing a hint text caption and a validation message, and associating those with the input for assistive technology.
Expand All @@ -165,7 +163,7 @@ We encourage using `FormControl` alongside all standalone form components like [
<FormControl.Label visuallyHidden>Name</FormControl.Label>
<TextInput />
</FormControl>
<FormControl variant="choice">
<FormControl>
<FormControl.Label visuallyHidden>Checkbox option</FormControl.Label>
<Checkbox />
</FormControl>
Expand All @@ -181,7 +179,7 @@ We encourage using `FormControl` alongside all standalone form components like [
<TextInput />
<FormControl.Caption>Hint: your first name</FormControl.Caption>
</FormControl>
<FormControl variant="choice">
<FormControl>
<FormControl.Label>Option one</FormControl.Label>
<Checkbox />
<FormControl.Caption>Hint: the first and only option</FormControl.Caption>
Expand Down Expand Up @@ -232,15 +230,15 @@ render(ValidationExample)

```jsx live
<>
<FormControl variant="choice">
<FormControl>
<FormControl.Label>Option one</FormControl.Label>
<FormControl.LeadingVisual>
<MarkGithubIcon />
</FormControl.LeadingVisual>
<Checkbox />
</FormControl>

<FormControl variant="choice">
<FormControl>
<FormControl.Label>Option two</FormControl.Label>
<FormControl.LeadingVisual>
<MarkGithubIcon />
Expand All @@ -263,12 +261,6 @@ The container that handles the layout and passes the relevant IDs and ARIA attri
type="FormControl.Label | FormControl.Caption | FormControl.Validation | Autocomplete | TextInput | TextInputWithTokens | Select"
required
/>
<PropsTableRow
name="variant"
defaultValue="'stack'"
type="'stack' | 'choice'"
description="Changes the layout of the form control"
/>
<PropsTableRow
name="disabled"
type="boolean"
Expand Down Expand Up @@ -320,7 +312,7 @@ A `FormControl.Label` must be passed for the field to be accessible to assistive

<Note>
Validation messages should not be shown for an individual checkbox or radio form control, so `FormControl.Validation`
will not be rendered when `variant="choice"` is passed to the parent `FormControl`. Validation messages for checkbox
will not be rendered when a `Checkbox` or `Radio` is not a child of `FormControl`. Validation messages for checkbox
and radio selections should only apply to the entire group of inputs.
</Note>

Expand Down
22 changes: 5 additions & 17 deletions src/FormControl/FormControl.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,18 +23,14 @@ export type FormControlProps = {
* Whether this field must have a value for the user to complete their task
*/
required?: boolean
/**
* Changes the layout of the form control
*/
variant?: 'stack' | 'choice' // TODO: come up with a better name for 'stack'
}

export interface FormControlContext extends Pick<FormControlProps, 'disabled' | 'id' | 'required'> {
captionId: string
validationMessageId: string
}

const FormControl = ({children, disabled, id: idProp, required, variant}: FormControlProps) => {
const FormControl = ({children, disabled, id: idProp, required}: FormControlProps) => {
const expectedInputComponents = [Autocomplete, Checkbox, Radio, Select, TextInput, TextInputWithTokens, Textarea]
const id = useSSRSafeId(idProp)
const validationChild = React.Children.toArray(children).find(child =>
Expand All @@ -53,6 +49,8 @@ const FormControl = ({children, disabled, id: idProp, required, variant}: FormCo
expectedInputComponents.some(inputComponent => React.isValidElement(child) && child.type === inputComponent)
)
const inputProps = React.isValidElement(InputComponent) ? InputComponent.props : undefined
const isChoiceInput =
React.isValidElement(InputComponent) && (InputComponent.type === Checkbox || InputComponent.type === Radio)

if (!InputComponent) {
// eslint-disable-next-line no-console
Expand Down Expand Up @@ -95,7 +93,7 @@ const FormControl = ({children, disabled, id: idProp, required, variant}: FormCo
)
}

if (variant === 'choice') {
if (isChoiceInput) {
if (validationChild) {
// eslint-disable-next-line no-console
console.warn(
Expand All @@ -108,12 +106,6 @@ const FormControl = ({children, disabled, id: idProp, required, variant}: FormCo
console.warn('An individual checkbox or radio cannot be a required field.')
}
} else {
// TODO: reconsider if we even need this check. The UI will just look like something is wrong
if (React.isValidElement(InputComponent) && (InputComponent.type === Checkbox || InputComponent.type === Radio)) {
// eslint-disable-next-line no-console
console.warn('The Checkbox or Radio components are only intended to be used with the "choice" variant')
}

if (
React.Children.toArray(children).find(
child => React.isValidElement(child) && child.type === FormControlLeadingVisual
Expand All @@ -139,7 +131,7 @@ const FormControl = ({children, disabled, id: idProp, required, variant}: FormCo
{slots => {
const isLabelHidden = React.isValidElement(slots.Label) && slots.Label.props.visuallyHidden

return variant === 'choice' ? (
return isChoiceInput ? (
<Box display="flex" alignItems={slots.LeadingVisual ? 'center' : undefined}>
<Box sx={{'> input': {marginLeft: 0, marginRight: 0}}}>
{React.isValidElement(InputComponent) &&
Expand Down Expand Up @@ -217,10 +209,6 @@ const FormControl = ({children, disabled, id: idProp, required, variant}: FormCo
)
}

FormControl.defaultProps = {
variant: 'stack'
}

export default Object.assign(FormControl, {
Caption: FormControlCaption,
Label: FormControlLabel,
Expand Down
30 changes: 8 additions & 22 deletions src/__tests__/FormControl.test.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ describe('FormControl', () => {
jest.clearAllMocks()
})

describe('default variant', () => {
describe('vertically stacked layout (default)', () => {
describe('rendering', () => {
it('renders with a hidden label', () => {
const {getByLabelText, getByText} = render(
Expand Down Expand Up @@ -365,12 +365,12 @@ describe('FormControl', () => {
})
})

describe('variant="choice"', () => {
describe('checkbox and radio layout', () => {
describe('rendering', () => {
it('renders with a LeadingVisual', () => {
const {getByLabelText} = render(
<SSRProvider>
<FormControl variant="choice">
<FormControl>
<FormControl.Label>{LABEL_TEXT}</FormControl.Label>
<Checkbox />
<FormControl.LeadingVisual>
Expand All @@ -387,25 +387,11 @@ describe('FormControl', () => {
})

describe('warnings', () => {
it('should warn users if they try to render anything except a Checkbox or Radio when using variant="choice"', async () => {
const consoleSpy = jest.spyOn(global.console, 'warn')
render(
<SSRProvider>
<FormControl variant="choice">
<FormControl.Label>{LABEL_TEXT}</FormControl.Label>
<TextInput />
<FormControl.Caption>{CAPTION_TEXT}</FormControl.Caption>
</FormControl>
</SSRProvider>
)

expect(consoleSpy).toHaveBeenCalled()
})
it('should warn users if they try to render a validation message when using variant="choice"', async () => {
it('should warn users if they try to render a validation message when using a checkbox or radio', async () => {
const consoleSpy = jest.spyOn(global.console, 'warn')
render(
<SSRProvider>
<FormControl variant="choice">
<FormControl>
<FormControl.Label>{LABEL_TEXT}</FormControl.Label>
<Checkbox />
<FormControl.Validation appearance="error">Some error</FormControl.Validation>
Expand All @@ -416,11 +402,11 @@ describe('FormControl', () => {

expect(consoleSpy).toHaveBeenCalled()
})
it('should warn users if they pass `required` to variant="choice"', async () => {
it('should warn users if they pass `required` to a checkbox or radio', async () => {
const consoleSpy = jest.spyOn(global.console, 'warn')
render(
<SSRProvider>
<FormControl variant="choice" required>
<FormControl required>
<FormControl.Label>{LABEL_TEXT}</FormControl.Label>
<Checkbox required />
<FormControl.Caption>{CAPTION_TEXT}</FormControl.Caption>
Expand All @@ -434,7 +420,7 @@ describe('FormControl', () => {
it('should have no axe violations', async () => {
const {container} = render(
<SSRProvider>
<FormControl variant="choice">
<FormControl>
<FormControl.Label>{LABEL_TEXT}</FormControl.Label>
<Checkbox />
<FormControl.Caption>{CAPTION_TEXT}</FormControl.Caption>
Expand Down
8 changes: 4 additions & 4 deletions src/stories/FormControl.stories.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -126,21 +126,21 @@ export const UsingSelectInput = (args: Args) => (
UsingTextInputWithTokens.storyName = 'Using TextInputWithTokens Input'

export const UsingCheckboxInput = (args: Args) => (
<FormControl variant="choice" {...args}>
<FormControl {...args}>
<Checkbox />
<FormControl.Label>Selectable choice</FormControl.Label>
</FormControl>
)

export const UsingRadioInput = (args: Args) => (
<FormControl variant="choice" {...args}>
<FormControl {...args}>
<Radio name="radioInput" value="choice" />
<FormControl.Label>Selectable choice</FormControl.Label>
</FormControl>
)

export const WithLeadingVisual = (args: Args) => (
<FormControl variant="choice" {...args}>
<FormControl {...args}>
<FormControl.Label>Selectable choice</FormControl.Label>
<FormControl.LeadingVisual>
<MarkGithubIcon />
Expand All @@ -151,7 +151,7 @@ export const WithLeadingVisual = (args: Args) => (
WithLeadingVisual.storyName = 'With LeadingVisual'

export const WithCaptionAndLeadingVisual = (args: Args) => (
<FormControl variant="choice" {...args}>
<FormControl {...args}>
<FormControl.Label>Selectable choice</FormControl.Label>
<FormControl.LeadingVisual>
<MarkGithubIcon />
Expand Down

0 comments on commit 260ca68

Please sign in to comment.