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

feat!: Use invalid prop for most inputs #1240

Merged
merged 13 commits into from
May 29, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions packages/css/documentation/coding-conventions.md
Original file line number Diff line number Diff line change
Expand Up @@ -41,3 +41,9 @@ Still, we define its thickness and offset for the initial state.
We use Sass mixins to extract common patterns, especially if they need more than 1 declaration.
We collect reset styles in mixins as well.
Both the name of the mixins and their documentation help explain the approach.

## Form validation styling

We style both the native invalid state (`:invalid`) as the invalid state you can set manually, using `aria-invalid`.
We’re currently not sure how our users will implement forms, which is why both options are supported.
[Native form validation isn’t without its issues](https://adrianroselli.com/2019/02/avoid-default-field-validation.html) though, so we might only support manual validation in the future.
10 changes: 10 additions & 0 deletions packages/css/src/components/search-field/search-field.scss
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,16 @@
}
}

.ams-search-field__input:invalid,
.ams-search-field__input[aria-invalid="true"] {
box-shadow: var(--ams-search-field-input-invalid-box-shadow);

&:hover {
// TODO: this should be the (currently non-existent) dark red hover color
box-shadow: var(--ams-search-field-input-invalid-hover-box-shadow);
}
}

.ams-search-field__input::placeholder {
color: var(--ams-search-field-input-placeholder-color);
opacity: 100%; // This resets the lower opacity set by Firefox
Expand Down
10 changes: 6 additions & 4 deletions packages/css/src/components/select/select.scss
Original file line number Diff line number Diff line change
Expand Up @@ -37,12 +37,9 @@
@include reset;
}

.ams-select:invalid,
.ams-select[aria-invalid="true"] {
VincentSmedinga marked this conversation as resolved.
Show resolved Hide resolved
box-shadow: var(--ams-select-invalid-box-shadow);

&:hover {
box-shadow: var(--ams-select-invalid-hover-box-shadow);
}
}

.ams-select:disabled {
Expand All @@ -55,6 +52,11 @@
}
}

.ams-select:invalid:hover,
.ams-select[aria-invalid="true"]:hover {
box-shadow: var(--ams-select-invalid-hover-box-shadow);
}

.ams-select__option:disabled {
color: var(--ams-select-option-disabled-color);
}
116 changes: 58 additions & 58 deletions packages/react/src/Checkbox/Checkbox.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -43,97 +43,97 @@ describe('Checkbox', () => {
expect(wrapper).toHaveClass('ams-checkbox')
})

// checked state
describe('Checked state', () => {
it('is not checked by default', () => {
render(<Checkbox />)

it('is not checked by default', () => {
render(<Checkbox />)
const input = screen.getByRole('checkbox')

const input = screen.getByRole('checkbox')
expect(input).not.toBeChecked()
})

expect(input).not.toBeChecked()
})
it('can have a checked state', () => {
const handleChange = () => {}
render(<Checkbox checked onChange={handleChange} />)

it('can have a checked state', () => {
const handleChange = () => {}
render(<Checkbox checked onChange={handleChange} />)
const input = screen.getByRole('checkbox')

const input = screen.getByRole('checkbox')

expect(input).toBeChecked()
expect(input).toBeChecked()
})
})

// indeterminate state

it('is not indeterminate by default', () => {
render(<Checkbox />)
describe('Indeterminate state', () => {
it('is not indeterminate by default', () => {
render(<Checkbox />)

const input = screen.getByRole('checkbox')
const input = screen.getByRole('checkbox')

expect(input).not.toBePartiallyChecked()
})
expect(input).not.toBePartiallyChecked()
})

it('can have an indeterminate state', () => {
render(<Checkbox indeterminate />)
it('can have an indeterminate state', () => {
render(<Checkbox indeterminate />)

const input = screen.getByRole('checkbox')
const input = screen.getByRole('checkbox')

expect(input).toBePartiallyChecked()
expect(input).toBePartiallyChecked()
})
})

// invalid state
describe('Invalid state', () => {
it('is not invalid by default', () => {
render(<Checkbox />)

it('is not invalid by default', () => {
render(<Checkbox />)

const input = screen.getByRole('checkbox')
const input = screen.getByRole('checkbox')

expect(input).not.toBeInvalid()
})
expect(input).not.toBeInvalid()
})

it('can have an invalid state', () => {
render(<Checkbox invalid />)
it('can have an invalid state', () => {
render(<Checkbox invalid />)

const input = screen.getByRole('checkbox')
const input = screen.getByRole('checkbox')

expect(input).toHaveAttribute('aria-invalid')
expect(input).toBeInvalid()
})
expect(input).toHaveAttribute('aria-invalid')
expect(input).toBeInvalid()
})

it('omits non-essential invalid attributes when not invalid', () => {
render(<Checkbox invalid={false} />)
it('omits non-essential invalid attributes when not invalid', () => {
render(<Checkbox invalid={false} />)

const input = screen.getByRole('checkbox')
const input = screen.getByRole('checkbox')

expect(input).not.toHaveAttribute('aria-invalid')
expect(input).not.toHaveAttribute('aria-invalid')
})
})

// disabled state

it('is not disabled by default', () => {
render(<Checkbox />)
describe('Disabled state', () => {
it('is not disabled by default', () => {
render(<Checkbox />)

const input = screen.getByRole('checkbox')
const input = screen.getByRole('checkbox')

expect(input).not.toBeDisabled()
})
expect(input).not.toBeDisabled()
})

it('can have a disabled state', () => {
render(<Checkbox disabled />)
it('can have a disabled state', () => {
render(<Checkbox disabled />)

const input = screen.getByRole('checkbox')
const input = screen.getByRole('checkbox')

expect(input).toBeDisabled()
expect(input).toBeDisabled()
})
})

// disabled invalid state
describe('Disabled invalid state', () => {
it('can have a disabled invalid state', () => {
render(<Checkbox disabled invalid />)

it('can have a disabled invalid state', () => {
render(<Checkbox disabled invalid />)

const input = screen.getByRole('checkbox')
const input = screen.getByRole('checkbox')

expect(input).toBeDisabled()
expect(input).toBeInvalid()
expect(input).toBeDisabled()
expect(input).toBeInvalid()
})
})

it('can trigger a change event', () => {
Expand Down
8 changes: 4 additions & 4 deletions packages/react/src/Checkbox/Checkbox.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ export type CheckboxProps = {
invalid?: boolean
/** Allows being neither checked nor unchecked. */
indeterminate?: boolean
} & PropsWithChildren<InputHTMLAttributes<HTMLInputElement>>
} & PropsWithChildren<Omit<InputHTMLAttributes<HTMLInputElement>, 'aria-invalid' | 'type'>>
VincentSmedinga marked this conversation as resolved.
Show resolved Hide resolved

export const Checkbox = forwardRef(
(
Expand All @@ -38,11 +38,11 @@ export const Checkbox = forwardRef(
<div className={clsx('ams-checkbox', className)}>
<input
{...restProps}
type="checkbox"
aria-invalid={invalid || undefined}
className="ams-checkbox__input"
ref={innerRef}
id={id}
aria-invalid={invalid || undefined}
ref={innerRef}
type="checkbox"
/>
<label className="ams-checkbox__label" htmlFor={id}>
<span className="ams-checkbox__checkmark" />
Expand Down
27 changes: 27 additions & 0 deletions packages/react/src/DateInput/DateInput.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -38,4 +38,31 @@ describe('Date input', () => {

expect(ref.current).toBe(component)
})

describe('Invalid state', () => {
it('is not invalid by default', () => {
const { container } = render(<DateInput />)

const component = container.querySelector(':only-child')

expect(component).not.toBeInvalid()
})

it('can have an invalid state', () => {
const { container } = render(<DateInput invalid />)

const component = container.querySelector(':only-child')

expect(component).toHaveAttribute('aria-invalid')
expect(component).toBeInvalid()
})

it('omits non-essential invalid attributes when not invalid', () => {
const { container } = render(<DateInput invalid={false} />)

const component = container.querySelector(':only-child')

expect(component).not.toHaveAttribute('aria-invalid')
})
})
})
15 changes: 12 additions & 3 deletions packages/react/src/DateInput/DateInput.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,20 @@ import clsx from 'clsx'
import { forwardRef } from 'react'
import type { ForwardedRef, InputHTMLAttributes } from 'react'

export type DateInputProps = InputHTMLAttributes<HTMLInputElement>
export type DateInputProps = {
/** Whether the value fails a validation rule. */
invalid?: boolean
} & Omit<InputHTMLAttributes<HTMLInputElement>, 'aria-invalid' | 'type'>

export const DateInput = forwardRef(
({ className, ...restProps }: DateInputProps, ref: ForwardedRef<HTMLInputElement>) => (
<input {...restProps} ref={ref} className={clsx('ams-date-input', className)} type="date" />
({ className, invalid, ...restProps }: DateInputProps, ref: ForwardedRef<HTMLInputElement>) => (
<input
{...restProps}
aria-invalid={invalid || undefined}
RubenSibon marked this conversation as resolved.
Show resolved Hide resolved
className={clsx('ams-date-input', className)}
ref={ref}
type="date"
/>
),
)

Expand Down
Loading
Loading