feat!: Use invalid prop for most inputs (#1240)

Co-authored-by: Vincent Smedinga <v.smedinga@amsterdam.nl>
Amsterdam · May 29, 2024 · 9477186 · 9477186
1 parent b7572d1
commit 9477186
Show file tree

Hide file tree

Showing 29 changed files with 352 additions and 213 deletions.
diff --git a/packages/css/documentation/coding-conventions.md b/packages/css/documentation/coding-conventions.md
@@ -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.
diff --git a/packages/css/src/components/search-field/search-field.scss b/packages/css/src/components/search-field/search-field.scss
@@ -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

diff --git a/packages/css/src/components/select/select.scss b/packages/css/src/components/select/select.scss
@@ -37,12 +37,9 @@
   @include reset;
 }
 
+.ams-select:invalid,
 .ams-select[aria-invalid="true"] {
   box-shadow: var(--ams-select-invalid-box-shadow);
-
-  &:hover {
-    box-shadow: var(--ams-select-invalid-hover-box-shadow);
-  }
 }
 
 .ams-select:disabled {
@@ -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);
 }
diff --git a/packages/react/src/Checkbox/Checkbox.test.tsx b/packages/react/src/Checkbox/Checkbox.test.tsx
@@ -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', () => {

diff --git a/packages/react/src/Checkbox/Checkbox.tsx b/packages/react/src/Checkbox/Checkbox.tsx
@@ -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'>>
 
 export const Checkbox = forwardRef(
   (
@@ -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" />

diff --git a/packages/react/src/DateInput/DateInput.test.tsx b/packages/react/src/DateInput/DateInput.test.tsx
@@ -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')
+    })
+  })
 })
diff --git a/packages/react/src/DateInput/DateInput.tsx b/packages/react/src/DateInput/DateInput.tsx
@@ -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}
+      className={clsx('ams-date-input', className)}
+      ref={ref}
+      type="date"
+    />
   ),
 )