fix: avoid duplicate onChange calls in FormRadioSet and FormCheckboxS…

…et (#2705) * fix: fixed duplicate calls in FormRadioSet and FormCheckboxSet * refactor: added new tests * refactor: refactoring after review (cherry picked from commit a395042)
openedx · Oct 13, 2023 · 6870106 · 6870106
1 parent cbfb47f
commit 6870106
Show file tree

Hide file tree

Showing 7 changed files with 89 additions and 43 deletions.
diff --git a/package-lock.json b/package-lock.json
diff --git a/package.json b/package.json
@@ -1,6 +1,6 @@
 {
   "name": "@edx/paragon",
-  "version": "20.18.1",
+  "version": "20.46.3",
   "description": "Accessible, responsive UI component library based on Bootstrap.",
   "main": "dist/index.js",
   "module": "dist/index.js",

diff --git a/src/Form/FormCheckbox.jsx b/src/Form/FormCheckbox.jsx
@@ -62,30 +62,27 @@ const FormCheckbox = React.forwardRef(({
   floatLabelLeft,
   ...props
 }, ref) => {
-  const { getCheckboxControlProps, hasCheckboxSetProvider } = useCheckboxSetContext();
+  const { hasCheckboxSetProvider } = useCheckboxSetContext();
   const { hasFormGroupProvider, useSetIsControlGroupEffect, getControlProps } = useFormGroupContext();
   useSetIsControlGroupEffect(true);
   const shouldActAsGroup = hasFormGroupProvider && !hasCheckboxSetProvider;
   const groupProps = shouldActAsGroup ? {
     ...getControlProps({}),
     role: 'group',
   } : {};
-  const checkboxInputProps = getCheckboxControlProps({
-    ...props,
-    className: controlClassName,
-  });
-  const control = React.createElement(controlAs, { ...checkboxInputProps, ref });
+
+  const control = React.createElement(controlAs, { ...props, className: controlClassName, ref });
   return (
     <FormGroupContextProvider
-      controlId={checkboxInputProps.id}
+      controlId={props.id}
       isInvalid={isInvalid}
       isValid={isValid}
     >
       <div
         className={classNames('pgn__form-checkbox', className, {
           'pgn__form-control-valid': isValid,
           'pgn__form-control-invalid': isInvalid,
-          'pgn__form-control-disabled': checkboxInputProps.disabled,
+          'pgn__form-control-disabled': props.disabled,
           'pgn__form-control-label-left': !!floatLabelLeft,
         })}
         {...groupProps}
@@ -107,6 +104,8 @@ const FormCheckbox = React.forwardRef(({
 });
 
 FormCheckbox.propTypes = {
+  /** Specifies id of the FormCheckbox component. */
+  id: PropTypes.string,
   /** Specifies contents of the component. */
   children: PropTypes.node.isRequired,
   /** Specifies class name to append to the base element. */
@@ -123,10 +122,14 @@ FormCheckbox.propTypes = {
   isValid: PropTypes.bool,
   /** Specifies control element. */
   controlAs: PropTypes.elementType,
+  /** Specifies whether the floating label should be aligned to the left. */
   floatLabelLeft: PropTypes.bool,
+  /** Specifies whether the `FormCheckbox` is disabled. */
+  disabled: PropTypes.bool,
 };
 
 FormCheckbox.defaultProps = {
+  id: undefined,
   className: undefined,
   controlClassName: undefined,
   labelClassName: undefined,
@@ -135,6 +138,7 @@ FormCheckbox.defaultProps = {
   isValid: false,
   controlAs: CheckboxControl,
   floatLabelLeft: false,
+  disabled: false,
 };
 
 export { CheckboxControl };

diff --git a/src/Form/FormRadio.jsx b/src/Form/FormRadio.jsx
@@ -40,42 +40,37 @@ const FormRadio = React.forwardRef(({
   isInvalid,
   isValid,
   ...props
-}, ref) => {
-  const { getRadioControlProps } = useRadioSetContext();
-  const radioInputProps = getRadioControlProps({
-    ...props,
-    className: controlClassName,
-  });
-  return (
-    <FormGroupContextProvider
-      controlId={radioInputProps.id}
-      isInvalid={isInvalid}
-      isValid={isValid}
+}, ref) => (
+  <FormGroupContextProvider
+    controlId={props.id}
+    isInvalid={isInvalid}
+    isValid={isValid}
+  >
+    <div
+      className={classNames('pgn__form-radio', className, {
+        'pgn__form-control-valid': isValid,
+        'pgn__form-control-invalid': isInvalid,
+        'pgn__form-control-disabled': props.disabled,
+      })}
     >
-      <div
-        className={classNames('pgn__form-radio', className, {
-          'pgn__form-control-valid': isValid,
-          'pgn__form-control-invalid': isInvalid,
-          'pgn__form-control-disabled': radioInputProps.disabled,
-        })}
-      >
-        <RadioControl {...radioInputProps} ref={ref} />
-        <div>
-          <FormLabel className={labelClassName}>
-            {children}
-          </FormLabel>
-          {description && (
-            <FormControlFeedback hasIcon={false}>
-              {description}
-            </FormControlFeedback>
-          )}
-        </div>
+      <RadioControl ref={ref} className={controlClassName} {...props} />
+      <div>
+        <FormLabel className={labelClassName}>
+          {children}
+        </FormLabel>
+        {description && (
+        <FormControlFeedback hasIcon={false}>
+          {description}
+        </FormControlFeedback>
+        )}
       </div>
-    </FormGroupContextProvider>
-  );
-});
+    </div>
+  </FormGroupContextProvider>
+));
 
 FormRadio.propTypes = {
+  /** Specifies id of the FormRadio component. */
+  id: PropTypes.string,
   /** Specifies contents of the component. */
   children: PropTypes.node.isRequired,
   /** Specifies class name to append to the base element. */
@@ -90,15 +85,19 @@ FormRadio.propTypes = {
   isInvalid: PropTypes.bool,
   /** Specifies whether to display component in valid state, this affects styling. */
   isValid: PropTypes.bool,
+  /** Specifies whether the `FormRadio` is disabled. */
+  disabled: PropTypes.bool,
 };
 
 FormRadio.defaultProps = {
+  id: undefined,
   className: undefined,
   controlClassName: undefined,
   labelClassName: undefined,
   description: undefined,
   isInvalid: false,
   isValid: false,
+  disabled: false,
 };
 
 export { RadioControl };

diff --git a/src/Form/tests/FormCheckboxSet.test.jsx b/src/Form/tests/FormCheckboxSet.test.jsx
@@ -1,5 +1,7 @@
 import React from 'react';
 import { mount } from 'enzyme';
+import userEvent from '@testing-library/user-event';
+import { render } from '@testing-library/react';
 import FormGroup from '../FormGroup';
 import FormCheckboxSet from '../FormCheckboxSet';
 import FormCheckbox from '../FormCheckbox';
@@ -149,4 +151,23 @@ describe('FormCheckboxSet', () => {
       expect(checkboxNode.props().defaultChecked).toBe(true);
     });
   });
+
+  it('checks if onClick is called once in FormCheckboxSet', () => {
+    const handleChange = jest.fn();
+    const { getByLabelText } = render(
+      <FormGroup controlId="my-field">
+        <FormLabel>Which color?</FormLabel>
+        <FormCheckboxSet
+          name="colors"
+          onChange={handleChange}
+        >
+          <FormCheckbox value="red">Red</FormCheckbox>
+          <FormCheckbox value="green">Green</FormCheckbox>
+        </FormCheckboxSet>
+      </FormGroup>,
+    );
+
+    userEvent.click(getByLabelText('Red'));
+    expect(handleChange).toHaveBeenCalledTimes(1);
+  });
 });
diff --git a/src/Form/tests/FormRadioSet.test.jsx b/src/Form/tests/FormRadioSet.test.jsx
@@ -1,5 +1,7 @@
 import React from 'react';
 import { mount } from 'enzyme';
+import userEvent from '@testing-library/user-event';
+import { render } from '@testing-library/react';
 import FormGroup from '../FormGroup';
 import FormRadioSet from '../FormRadioSet';
 import FormRadio from '../FormRadio';
@@ -102,4 +104,23 @@ describe('FormRadioSet', () => {
     const radioNode = wrapper.find('input[type="radio"]').first();
     expect(radioNode.props().name).toBe('trees');
   });
+
+  it('checks if onClick is called once in FormRadioSet', () => {
+    const handleChange = jest.fn();
+    const { getByLabelText } = render(
+      <FormGroup>
+        <FormLabel>Which color?</FormLabel>
+        <FormRadioSet
+          name="colors"
+          onChange={handleChange}
+        >
+          <FormRadio value="red">Red</FormRadio>
+          <FormRadio value="green">Green</FormRadio>
+        </FormRadioSet>
+      </FormGroup>,
+    );
+
+    userEvent.click(getByLabelText('Red'));
+    expect(handleChange).toHaveBeenCalledTimes(1);
+  });
 });
diff --git a/src/Menu/__snapshots__/Menu.test.jsx.snap b/src/Menu/__snapshots__/Menu.test.jsx.snap
@@ -126,6 +126,7 @@ exports[`Menu Item renders correctly renders as expected with menu items 1`] = `
   >
     <input
       className="pgn__form-checkbox-input"
+      disabled={false}
       id="form-field1"
       type="checkbox"
     />