fix(radio-group): allow uncontrolled radio and radiogroup (#2375)

* fix(radio-group): allow uncontrolled radio and radiogroup * chore(sibling-box, base-radio-checkbox): add styles for error and disabled sibling-box * chore(checkbox): minor code refactor and adding stories for error cases * chore: disable bad advice Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
twilio-labs · May 3, 2022 · 13aad7a · 13aad7a
1 parent b465e11
commit 13aad7a
Show file tree

Hide file tree

Showing 18 changed files with 975 additions and 220 deletions.
diff --git a/.changeset/eleven-dogs-eat.md b/.changeset/eleven-dogs-eat.md
@@ -0,0 +1,6 @@
+---
+'@twilio-paste/checkbox': patch
+'@twilio-paste/core': patch
+---
+
+[Checkbox] Minor code refactor
diff --git a/.changeset/hungry-owls-jam.md b/.changeset/hungry-owls-jam.md
@@ -0,0 +1,6 @@
+---
+'@twilio-paste/radio-group': patch
+'@twilio-paste/core': patch
+---
+
+[RadioGroup] Allows usage of uncontrolled Radios and RadioGroups
diff --git a/.changeset/unlucky-fishes-tan.md b/.changeset/unlucky-fishes-tan.md
@@ -0,0 +1,7 @@
+---
+'@twilio-paste/core': patch
+'@twilio-paste/sibling-box': patch
+'@twilio-paste/base-radio-checkbox': patch
+---
+
+[Sibling-box, Base-radio-checkbox] Add styles for when the element has an error while it is disabled
diff --git a/.eslintrc.js b/.eslintrc.js
@@ -71,6 +71,7 @@ module.exports = {
     'unicorn/no-fn-reference-in-iterator': 'off',
     // weirdly specific
     'unicorn/import-style': 'off',
+    'unicorn/prefer-ternary': 'off',
     // Too big of a change
     'unicorn/numeric-separators-style': 'off',
     // Mixed ts and node code base

diff --git a/packages/paste-core/components/base-radio-checkbox/src/BaseRadioCheckbox.tsx b/packages/paste-core/components/base-radio-checkbox/src/BaseRadioCheckbox.tsx
@@ -63,6 +63,9 @@ const BaseRadioCheckboxControl = React.forwardRef<HTMLSpanElement, BaseRadioChec
         _invalidAndHoverSibling={{
           borderColor: 'colorBorderErrorStronger',
         }}
+        _invalidAndDisabledSibling={{
+          borderColor: 'colorBorderWeaker',
+        }}
         _checkedAndHoverSibling={{
           borderColor: 'colorBorderPrimaryStronger',
           backgroundColor: 'colorBackgroundPrimaryStronger',
@@ -95,6 +98,11 @@ const BaseRadioCheckboxControl = React.forwardRef<HTMLSpanElement, BaseRadioChec
           borderColor: 'colorBorderErrorStronger',
           backgroundColor: 'colorBackgroundErrorStronger',
         }}
+        _checkedAndInvalidAndDisabledSibling={{
+          borderColor: 'colorBorderWeaker',
+          backgroundColor: 'colorBackgroundStrong',
+          color: 'colorTextWeakest',
+        }}
         {...props}
       >
         {children}

diff --git a/packages/paste-core/components/checkbox/src/Checkbox.tsx b/packages/paste-core/components/checkbox/src/Checkbox.tsx
@@ -3,7 +3,6 @@ import * as PropTypes from 'prop-types';
 import {useUID} from '@twilio-paste/uid-library';
 import {Box, safelySpreadBoxProps} from '@twilio-paste/box';
 import type {BoxProps} from '@twilio-paste/box';
-import type {BackgroundColorOptions, SpaceOptions} from '@twilio-paste/style-props';
 import {CheckboxCheckIcon} from '@twilio-paste/icons/esm/CheckboxCheckIcon';
 import {MinusIcon} from '@twilio-paste/icons/esm/MinusIcon';
 import {
@@ -16,6 +15,25 @@ import {MediaObject, MediaFigure, MediaBody} from '@twilio-paste/media-object';
 import {RequiredDot} from '@twilio-paste/label';
 import {CheckboxContext} from './CheckboxContext';
 
+const selectAllStyleProps = {
+  paddingTop: 'space20',
+  paddingRight: 'space30',
+  paddingBottom: 'space20',
+  paddingLeft: 'space20',
+  borderRadius: 'borderRadius10',
+  backgroundColor: 'colorBackground',
+};
+
+const selectAllActiveStyleProps = {
+  ...selectAllStyleProps,
+  backgroundColor: 'colorBackgroundPrimaryWeakest',
+};
+
+const selectAllChildStyleProps = {
+  paddingLeft: 'space30',
+  paddingRight: 'space30',
+};
+
 export interface CheckboxProps extends React.InputHTMLAttributes<HTMLInputElement>, Pick<BoxProps, 'element'> {
   children: NonNullable<React.ReactNode>;
   hasError?: boolean;
@@ -30,7 +48,16 @@ export interface CheckboxProps extends React.InputHTMLAttributes<HTMLInputElemen
 
 type HiddenCheckboxProps = Pick<
   CheckboxProps,
-  'checked' | 'element' | 'disabled' | 'id' | 'indeterminate' | 'name' | 'onChange' | 'required' | 'value'
+  | 'checked'
+  | 'defaultChecked'
+  | 'element'
+  | 'disabled'
+  | 'id'
+  | 'indeterminate'
+  | 'name'
+  | 'onChange'
+  | 'required'
+  | 'value'
 > & {
   ref?: any | undefined;
 };
@@ -77,7 +104,8 @@ const CheckboxIcon: React.FC<{
 const Checkbox = React.forwardRef<HTMLInputElement, CheckboxProps>(
   (
     {
-      checked: checkedProp,
+      checked,
+      defaultChecked,
       element = 'CHECKBOX',
       children,
       helpText,
@@ -86,71 +114,67 @@ const Checkbox = React.forwardRef<HTMLInputElement, CheckboxProps>(
       isSelectAll,
       isSelectAllChild,
       required,
+      onChange,
       ...props
     },
     ref
   ) => {
-    const helpTextId = useUID();
+    if (checked != null && defaultChecked != null) {
+      throw new Error(
+        `[Paste Checkbox] Do not provide both 'defaultChecked' and 'checked' to Checkbox at the same time. Please consider if you want this component to be controlled or uncontrolled.`
+      );
+    }
+
+    // Keeps track of the `checked` state on uncontrolled Checkboxes
+    // in order to properly render the checkbox icon svg.
+    const [checkedState, setCheckedState] = React.useState(defaultChecked);
+
     const checkboxGroupContext = React.useContext(CheckboxContext);
+    const helpTextId = useUID();
+    // We shouldn't change between controlled and uncontrolled after mount, so we memo this for safety
+    const isControlled = React.useMemo(() => checked !== undefined, []);
+
+    // Determines if the checkbox is checked in either controlled or uncontrolled environments
+    const mergedChecked = isControlled ? checked : checkedState;
+
+    const handleChange = React.useCallback(
+      (event: React.ChangeEvent<HTMLInputElement>): void => {
+        if (!isControlled) {
+          setCheckedState(event.target.checked);
+        } else if (onChange) {
+          onChange(event);
+        } else {
+          checkboxGroupContext.onChange(event);
+        }
+      },
+      [onChange, checkboxGroupContext.onChange]
+    );
 
     // Prioritizing direct props values over whatever CheckboxGroupContext passes down
     const disabled = props.disabled != null ? props.disabled : checkboxGroupContext.disabled;
     const name = props.name != null ? props.name : checkboxGroupContext.name;
     const hasError = props.hasError != null ? props.hasError : checkboxGroupContext.hasError;
 
-    // Manages what the value of checked is depending on if it is controlled or uncontrolled
-    const [checkedState, setCheckedState] = React.useState(!!props.defaultChecked);
-    const isControlled = checkedProp !== undefined;
-    const mergedChecked = isControlled ? checkedProp : checkedState;
-
-    const handleChange = (event: React.ChangeEvent<HTMLInputElement>): void => {
-      if (!isControlled) {
-        setCheckedState(event.target.checked);
-      } else if (props.onChange) {
-        props.onChange(event);
-      } else {
-        checkboxGroupContext.onChange(event);
-      }
-    };
-
-    let paddingLeft: SpaceOptions | null = null;
-    let checkboxBackground: BackgroundColorOptions | null = null;
-
+    // Custom checkbox styles if selectAll(Child)
+    let selectAllStyles = {};
     if (isSelectAll) {
-      paddingLeft = 'space20';
-
-      checkboxBackground = 'colorBackground';
-
-      if ((mergedChecked || indeterminate) && !disabled) {
-        checkboxBackground = 'colorBackgroundPrimaryWeakest';
-      }
+      selectAllStyles = !disabled && (mergedChecked || indeterminate) ? selectAllActiveStyleProps : selectAllStyleProps;
     }
     if (isSelectAllChild) {
-      paddingLeft = 'space30';
+      selectAllStyles = selectAllChildStyleProps;
     }
 
     return (
-      <Box
-        element={element}
-        backgroundColor={checkboxBackground}
-        borderRadius={isSelectAll ? 'borderRadius10' : null}
-        display="inline-flex"
-        position="relative"
-        flexDirection="column"
-        padding={isSelectAll ? 'space30' : null}
-        paddingBottom={isSelectAll ? 'space20' : null}
-        paddingLeft={paddingLeft}
-        paddingRight={isSelectAllChild ? 'space30' : null}
-        paddingTop={isSelectAll ? 'space20' : null}
-      >
+      <Box element={element} display="inline-flex" position="relative" flexDirection="column" {...selectAllStyles}>
         <HiddenCheckbox
           {...safelySpreadBoxProps(props)}
-          checked={checkedProp}
+          checked={checked}
+          defaultChecked={defaultChecked}
           disabled={disabled}
           name={name}
           onChange={handleChange}
           aria-describedby={helpTextId}
-          aria-checked={indeterminate ? 'mixed' : checkedProp}
+          aria-checked={indeterminate ? 'mixed' : checked}
           aria-invalid={hasError}
           id={id}
           required={required}

diff --git a/packages/paste-core/components/checkbox/stories/checkbox.stories.tsx b/packages/paste-core/components/checkbox/stories/checkbox.stories.tsx
@@ -44,6 +44,17 @@ CheckboxChecked.story = {
   name: 'Checkbox - Checked',
 };
 
+export const CheckboxDefaultChecked = (): React.ReactNode => {
+  return (
+    <Checkbox id={useUID()} value="label" name="foo" defaultChecked>
+      Label
+    </Checkbox>
+  );
+};
+CheckboxDefaultChecked.story = {
+  name: 'Checkbox - defaultChecked',
+};
+
 export const CheckboxRequired = (): React.ReactNode => {
   return (
     <Checkbox id={useUID()} value="label" name="foo" helpText="Some interesting help text" required>
@@ -124,6 +135,30 @@ CheckboxErrorChecked.story = {
   name: 'Checkbox - Error & Checked',
 };
 
+export const CheckboxErrorDisabled = (): React.ReactNode => {
+  return (
+    <Checkbox id={useUID()} value="1" name="foo" hasError disabled>
+      Label
+    </Checkbox>
+  );
+};
+
+CheckboxErrorDisabled.story = {
+  name: 'Checkbox - Error & Disabled',
+};
+
+export const CheckboxErrorDisabledChecked = (): React.ReactNode => {
+  return (
+    <Checkbox id={useUID()} value="1" name="foo" hasError disabled checked>
+      Label
+    </Checkbox>
+  );
+};
+
+CheckboxErrorDisabledChecked.story = {
+  name: 'Checkbox - Error & Disabled & Checked',
+};
+
 export const CheckboxHelpTextString = (): React.ReactNode => {
   return (
     <Checkbox id={useUID()} value="1" name="foo" helpText="Some interesting help text">

diff --git a/packages/paste-core/components/radio-group/__tests__/radio.test.tsx b/packages/paste-core/components/radio-group/__tests__/radio.test.tsx
@@ -52,7 +52,7 @@ describe('Radio', () => {
     expect(getByRole('radio').getAttribute('aria-invalid')).toBeTruthy();
   });
 
-  it('should render as checked', () => {
+  it('should render as checked (controlled)', () => {
     const {getByLabelText} = render(
       <Radio {...defaultProps} checked>
         foo
@@ -61,6 +61,15 @@ describe('Radio', () => {
     expect((getByLabelText('foo') as HTMLInputElement).checked).toBeTruthy();
   });
 
+  it('should render as checked (uncontrolled)', () => {
+    const {getByLabelText} = render(
+      <Radio {...defaultProps} defaultChecked>
+        foo
+      </Radio>
+    );
+    expect((getByLabelText('foo') as HTMLInputElement).checked).toBeTruthy();
+  });
+
   it('should render as disabled', () => {
     const {getByLabelText} = render(
       <Radio {...defaultProps} disabled>
@@ -265,9 +274,9 @@ describe('Radio Group event handlers', () => {
     expect(onBlurMock).toHaveBeenCalledTimes(1);
   });
 
-  it('Should check the selected radio', () => {
+  it('Should check the selected radio (controlled)', () => {
     const MockRadioGroup: React.FC = () => {
-      const [value, setValue] = React.useState('');
+      const [value, setValue] = React.useState('2');
       return (
         <RadioGroup
           legend="foo"
@@ -278,25 +287,58 @@ describe('Radio Group event handlers', () => {
             setValue(newVal);
           }}
         >
-          <Radio data-testid="radio-button" id="bar" name="bar" value="1">
+          <Radio data-testid="radio-button" id="bar" name="bar" value="1" checked>
             bar
           </Radio>
           <Radio data-testid="radio-button1" id="bar" name="bar" value="2">
             bar
           </Radio>
+          <Radio data-testid="radio-button2" id="bar" name="bar" value="3" defaultChecked>
+            bar
+          </Radio>
         </RadioGroup>
       );
     };
 
     const {getByTestId} = render(<MockRadioGroup />);
 
     expect((getByTestId('radio-button') as HTMLInputElement).checked).toBe(false);
+    expect((getByTestId('radio-button1') as HTMLInputElement).checked).toBe(true);
+    expect((getByTestId('radio-button2') as HTMLInputElement).checked).toBe(false);
+    fireEvent.click(getByTestId('radio-button'));
+    expect((getByTestId('radio-button') as HTMLInputElement).checked).toBe(true);
     expect((getByTestId('radio-button1') as HTMLInputElement).checked).toBe(false);
+    expect((getByTestId('radio-button2') as HTMLInputElement).checked).toBe(false);
+    fireEvent.click(getByTestId('radio-button2'));
+    expect((getByTestId('radio-button') as HTMLInputElement).checked).toBe(false);
+    expect((getByTestId('radio-button1') as HTMLInputElement).checked).toBe(false);
+    expect((getByTestId('radio-button2') as HTMLInputElement).checked).toBe(true);
+  });
+
+  it('Should check the selected radio (uncontrolled)', () => {
+    const MockRadioGroup: React.FC = () => {
+      return (
+        <RadioGroup legend="foo" id="foo" name="foo">
+          <Radio data-testid="radio-button" id="bar" name="bar" value="1">
+            bar
+          </Radio>
+          <Radio data-testid="radio-button1" id="bar" name="bar" value="2" defaultChecked>
+            bar
+          </Radio>
+        </RadioGroup>
+      );
+    };
+
+    const {getByTestId} = render(<MockRadioGroup />);
+
+    expect((getByTestId('radio-button') as HTMLInputElement).checked).toBe(false);
+    expect((getByTestId('radio-button1') as HTMLInputElement).checked).toBe(true);
     fireEvent.click(getByTestId('radio-button'));
     expect((getByTestId('radio-button') as HTMLInputElement).checked).toBe(true);
+    expect((getByTestId('radio-button1') as HTMLInputElement).checked).toBe(false);
     fireEvent.click(getByTestId('radio-button1'));
-    expect((getByTestId('radio-button1') as HTMLInputElement).checked).toBe(true);
     expect((getByTestId('radio-button') as HTMLInputElement).checked).toBe(false);
+    expect((getByTestId('radio-button1') as HTMLInputElement).checked).toBe(true);
   });
 
   it('Should check the selected value on initial', () => {