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 checkbox and radio input validation positioning #728

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
5 changes: 5 additions & 0 deletions .changeset/loud-phones-exercise.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@primer/react-brand': patch
---

Updates the styling of validation messages for checkbox and radio inputs to use flex-grid to fix a bug and adds storybook examples for each scenario.
5 changes: 5 additions & 0 deletions packages/react/src/forms/Checkbox/Checkbox.module.css
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,11 @@
position: relative;
}

[class*='FormControl'] .Checkbox-wrapper,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This might be overkill but I wasn't sure if we wanted to always apply the grid-row to the wrapper for the input or only do this when its parent is a form control element?

[id*='FormControl'] .Checkbox-wrapper {
grid-row: 1;
}

.Checkbox {
align-items: center;
background-color: var(--brand-control-checkbox-bg-default);
Expand Down
17 changes: 8 additions & 9 deletions packages/react/src/forms/Checkbox/Checkbox.module.css.d.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@
declare const styles: {
readonly "Checkbox-wrapper": string;
readonly "Checkbox": string;
readonly "Checkbox-input": string;
readonly "Checkbox-checkmark": string;
readonly "Checkbox-checkmark-path": string;
readonly "Checkbox--indeterminate": string;
};
export = styles;

readonly 'Checkbox-wrapper': string
readonly Checkbox: string
readonly 'Checkbox-input': string
readonly 'Checkbox-checkmark': string
readonly 'Checkbox-checkmark-path': string
readonly 'Checkbox--indeterminate': string
}
export = styles
10 changes: 6 additions & 4 deletions packages/react/src/forms/FormControl/FormControl.module.css
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,8 @@
}

.FormControl--checkbox {
display: flex;
align-items: flex-start;
flex-direction: row-reverse;
justify-content: flex-end;
display: grid;
grid-template-columns: auto 1fr;
}

.FormControl--border {
Expand Down Expand Up @@ -116,3 +114,7 @@
transform: translateY(0);
}
}

.FormControl-validation-checkbox {
grid-column: 1 / 3;
}
42 changes: 21 additions & 21 deletions packages/react/src/forms/FormControl/FormControl.module.css.d.ts
Original file line number Diff line number Diff line change
@@ -1,22 +1,22 @@
declare const styles: {
readonly "FormControl": string;
readonly "FormControl--fullWidth": string;
readonly "FormControl--checkbox": string;
readonly "FormControl--border": string;
readonly "FormControl-label": string;
readonly "FormControl-label--visually-hidden": string;
readonly "FormControl-label--medium": string;
readonly "FormControl-label--large": string;
readonly "FormControl-label--checkbox": string;
readonly "FormControl-control--radio": string;
readonly "FormControl-hint": string;
readonly "FormControl-validation": string;
readonly "FormControl-validation--success": string;
readonly "FormControl-validation--error": string;
readonly "FormControl-validation-success-icon": string;
readonly "FormControl-validation-error-icon": string;
readonly "FormControl-validation--animate-in": string;
readonly "FormControlValidationFadeIn": string;
};
export = styles;

readonly FormControl: string
readonly 'FormControl--fullWidth': string
readonly 'FormControl--checkbox': string
readonly 'FormControl--border': string
readonly 'FormControl-label': string
readonly 'FormControl-label--visually-hidden': string
readonly 'FormControl-label--medium': string
readonly 'FormControl-label--large': string
readonly 'FormControl-label--checkbox': string
readonly 'FormControl-control--radio': string
readonly 'FormControl-hint': string
readonly 'FormControl-validation': string
readonly 'FormControl-validation--success': string
readonly 'FormControl-validation--error': string
readonly 'FormControl-validation-success-icon': string
readonly 'FormControl-validation-error-icon': string
readonly 'FormControl-validation--animate-in': string
readonly 'FormControl-validation-checkbox': string
readonly FormControlValidationFadeIn: string
}
export = styles
30 changes: 28 additions & 2 deletions packages/react/src/forms/FormControl/FormControl.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -296,12 +296,24 @@ export const CheckboxPlayground = args => {

CheckboxPlayground.storyName = 'w/ Checkbox - Playground'

export const RadioPlayground = () => (
export const RadioPlayground = args => (
<>
<Stack direction={{narrow: 'vertical', regular: 'horizontal'}} gap="condensed" padding="condensed">
<FormControl>
<FormControl
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is pretty much borrowed from the CheckboxPlayground implementation so I could re-use this component for the example with a validation message.

required={args.required}
validationStatus={args.validationStatus}
fullWidth={args.fullWidth}
size={args.size}
>
<FormControl.Label>Radio One</FormControl.Label>
<Radio name="radio-group" value="radio one" />
{args.validationStatus && args.validationStatus === 'error' && (
// eslint-disable-next-line i18n-text/no-en
<FormControl.Validation>{args.validationText || 'This is an error message'}</FormControl.Validation>
)}
{args.validationStatus && args.validationStatus === 'success' && (
<FormControl.Validation>{args.validationText || 'Great! It worked.'}</FormControl.Validation>
)}
</FormControl>
<FormControl>
<FormControl.Label>Radio Two</FormControl.Label>
Expand Down Expand Up @@ -334,3 +346,17 @@ export const ErrorValidation = () => {
)
}
ErrorValidation.storyName = 'w/ Error Validation - Playground'

export const ErrorValidationWithCheckbox = args => <CheckboxPlayground {...args} />
ErrorValidationWithCheckbox.storyName = 'w/ Error Validation with Checkbox - Playground'
ErrorValidationWithCheckbox.args = {
validationStatus: 'error',
validationText: 'This is an error message',
}

export const ErrorValidationWithRadio = args => <RadioPlayground {...args} />
ErrorValidationWithRadio.storyName = 'w/ Error Validation with Radio - Playground'
ErrorValidationWithRadio.args = {
validationStatus: 'error',
validationText: 'This is an error message',
}
4 changes: 3 additions & 1 deletion packages/react/src/forms/FormControl/FormControl.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,7 @@ const Root = ({
* Validation
*/
return React.cloneElement(child as React.ReactElement, {
className: clsx(isInlineControl && styles['FormControl-validation-checkbox'], child.props.className),
validationStatus,
})
} else {
Expand Down Expand Up @@ -213,13 +214,14 @@ type FormControlValidationProps = {
validationStatus?: FormValidationStatus
} & BaseProps<HTMLSpanElement>

const FormControlValidation = ({children, validationStatus}: FormControlValidationProps) => {
const FormControlValidation = ({children, validationStatus, className}: FormControlValidationProps) => {
return (
<span
className={clsx(
styles['FormControl-validation'],
validationStatus && styles['FormControl-validation--animate-in'],
validationStatus && styles[`FormControl-validation--${validationStatus}`],
className,
)}
>
{validationStatus === 'error' && (
Expand Down
18 changes: 18 additions & 0 deletions packages/react/src/forms/FormControl/FormControl.visual.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -69,4 +69,22 @@ test.describe('Visual Comparison: FormControl', () => {
await page.waitForTimeout(500)
expect(await page.screenshot({fullPage: true})).toMatchSnapshot()
})

test('FormControl / w/ Error Validation with Checkbox - Playground', async ({page}) => {
await page.goto(
'http://localhost:6006/iframe.html?args=&id=components-forms-formcontrol--error-validation-with-checkbox&viewMode=story',
)

await page.waitForTimeout(500)
expect(await page.screenshot({fullPage: true})).toMatchSnapshot()
})

test('FormControl / w/ Error Validation with Radio - Playground', async ({page}) => {
await page.goto(
'http://localhost:6006/iframe.html?args=&id=components-forms-formcontrol--error-validation-with-radio&viewMode=story',
)

await page.waitForTimeout(500)
expect(await page.screenshot({fullPage: true})).toMatchSnapshot()
})
})
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.
5 changes: 5 additions & 0 deletions packages/react/src/forms/Radio/Radio.module.css
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,11 @@
cursor: pointer;
}

[class*='FormControl'] .Radio-wrapper,
[id*='FormControl'] .Radio-wrapper {
grid-row: 1;
}

.Radio {
align-items: center;
background-color: var(--brand-control-radio-bg-default);
Expand Down
13 changes: 6 additions & 7 deletions packages/react/src/forms/Radio/Radio.module.css.d.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
declare const styles: {
readonly "Radio-wrapper": string;
readonly "Radio": string;
readonly "Radio-input": string;
readonly "Radio-dot": string;
};
export = styles;

readonly 'Radio-wrapper': string
readonly Radio: string
readonly 'Radio-input': string
readonly 'Radio-dot': string
}
export = styles
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading