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
openedx · Oct 13, 2023 · a395042 · a395042
1 parent 9face09
commit a395042
Show file tree

Hide file tree

Showing 5 changed files with 82 additions and 40 deletions.
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
@@ -165,4 +165,23 @@ describe('FormCheckboxSet', () => {
       expect(checkboxNode.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
@@ -108,4 +108,23 @@ describe('FormRadioSet', () => {
     expect(evergreenRadio).toHaveAttribute('name', 'trees');
     expect(deciduousRadio).toHaveAttribute('name', '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"
     />