feat: improve disabled prop handling in Button and ButtonGroup (#…

…930) * feat: support disabled button group * ci: add `build-public` script * docs: update the default value of the `disabled` prop for `ButtonGroup`, `CheckboxGroup`, and `RadioGroup` * feat: use nullish coalescing operator (??) for checking the group's disabled prop * chore: add changeset `tonic-ui-927.md` * refactor: allow individual control of disabled prop for button, checkbox, or radio, overriding group setting when needed * Update tonic-ui-927.md * Update tonic-ui-927.md * feat: detect name prop conflicts between Checkbox/Radio and CheckboxGroup/RadioGroup * docs: remove console.log from TableOfContents * docs: resolve duplicate `key` prop errors in side navigation menu
trendmicro-frontend · Sep 26, 2024 · cdb2bbc · cdb2bbc
1 parent da62c8c
commit cdb2bbc
Show file tree

Hide file tree

Showing 13 changed files with 96 additions and 18 deletions.
diff --git a/.changeset/tonic-ui-927.md b/.changeset/tonic-ui-927.md
@@ -0,0 +1,5 @@
+---
+"@tonic-ui/react": minor
+---
+
+feat: improve `disabled` prop handling in `Button` and `ButtonGroup`
diff --git a/package.json b/package.json
@@ -7,6 +7,7 @@
   ],
   "scripts": {
     "build": "lerna exec -- yarn build",
+    "build-public": "lerna exec --no-private -- yarn build",
     "clean": "lerna exec --parallel -- yarn clean && lerna clean --yes",
     "changeset": "changeset",
     "ci-publish": "yarn lerna-publish-from-package --yes",

diff --git a/packages/react-docs/components/Sidebar.js b/packages/react-docs/components/Sidebar.js
@@ -208,7 +208,7 @@ const Sidebar = forwardRef((
                         if (heading) {
                           return (
                             <Text
-                              key={title}
+                              key={`${sectionTitle} > ${title}`}
                               color={colorStyle?.color?.tertiary}
                               fontSize="xs"
                               lineHeight="xs"
@@ -231,7 +231,7 @@ const Sidebar = forwardRef((
 
                         return (
                           <NavLink
-                            key={title}
+                            key={path}
                             data-path={path}
                             data-track={`SideMenu|click_menu_item|${x({ path: navigateTo, title: [sectionTitle, title].join(' > ') })}`}
                             isActive={isActive}

diff --git a/packages/react-docs/components/TableOfContents.js b/packages/react-docs/components/TableOfContents.js
@@ -55,7 +55,6 @@ const TableOfContents = (props) => {
     if (mainContent) {
       // Use the `:scope` pseudo-class to select all direct child heading elements of the main content, excluding h1
       const headingSelectors = ['h2', 'h3', 'h4', 'h5', 'h6'].map(h => `:scope>${h}`).join(',');
-      console.log(headingSelectors);
       setNodes(Array.from(mainContent.querySelectorAll(headingSelectors)));
     }
 

diff --git a/packages/react-docs/pages/components/button-group/index.page.mdx b/packages/react-docs/pages/components/button-group/index.page.mdx
@@ -47,6 +47,7 @@ The following example shows different states (i.e. normal, disabled, and selecte
 | Name | Type | Default | Description |
 | :--- | :--- | :------ | :---------- |
 | children | ReactNode \| `(context) => ReactNode` | | A function child can be used intead of a React element. This function is called with the context object. |
+| disabled | boolean | | If `true`, all buttons will be disabled. |
 | orientation | string | 'horizontal' | The orientation of the button group. One of: 'horizontal', 'vertical' |
 | size | string | 'md' | The size of the button group. One of: 'sm', 'md', 'lg' |
 | variant | string | 'default' | The variant of the button group. One of: 'emphasis', 'primary', 'default', 'secondary', 'ghost' |
diff --git a/packages/react-docs/pages/components/checkbox-group/index.page.mdx b/packages/react-docs/pages/components/checkbox-group/index.page.mdx
@@ -52,7 +52,7 @@ Use the `size` prop to change the size of the `CheckboxGroup`. You can set the v
 | :--- | :--- | :------ | :---------- |
 | children | ReactNode \| `(context) => ReactNode` | | A function child can be used intead of a React element. This function is called with the context object. |
 | defaultValue | (string\|number)[] | | The initial value of the checkbox group. |
-| disabled | boolean | false | If `true`, all checkboxes will be disabled. |
+| disabled | boolean | | If `true`, all checkboxes will be disabled. |
 | name | string | | The name used to reference the value of the control. If you don't provide this prop, it falls back to a randomly generated name. |
 | onChange | (event) => void | | A callback fired when any descendant `Checkbox` is checked or unchecked. |
 | size | string | 'md' | The size (width and height) of the checkbox. One of: 'sm', 'md', 'lg' |

diff --git a/packages/react-docs/pages/components/radio-group/index.page.mdx b/packages/react-docs/pages/components/radio-group/index.page.mdx
@@ -52,7 +52,7 @@ Use the `size` prop to change the size of the `RadioGroup`. You can set the valu
 | :--- | :--- | :------ | :---------- |
 | children | ReactNode \| `(context) => ReactNode` | | A function child can be used intead of a React element. This function is called with the context object. |
 | defaultValue | string \| number | | The default `input` element value. Use when the component is not controlled. |
-| disabled | boolean | false | If `true`, all radios will be disabled. |
+| disabled | boolean | | If `true`, all radios will be disabled. |
 | name | string | | The name used to reference the value of the control. If you don't provide this prop, it falls back to a randomly generated name. |
 | onChange | function | | A callback called when the state of the radio changes. |
 | size | string | 'md' | The size (width and height) of the radio. One of: 'sm', 'md', 'lg' |

diff --git a/packages/react/src/button/Button.js b/packages/react/src/button/Button.js
@@ -11,28 +11,31 @@ const defaultOrientation = 'horizontal';
 
 const Button = forwardRef((inProps, ref) => {
   const {
-    disabled,
+    disabled: disabledProp,
     selected,
     size: sizeProp,
     variant: variantProp,
     ...rest
   } = useDefaultProps({ props: inProps, name: 'Button' });
 
-  let orientation; // orientation for ButtonGroup
+  let disabled = disabledProp;
+  let orientation; // No default value assigned; orientation is determined by `ButtonGroup`
   let size = sizeProp;
   let variant = variantProp;
 
   const buttonGroupContext = useButtonGroup();
   if (buttonGroupContext) {
     const {
+      disabled: buttonGroupDisabled,
+      orientation: buttonGroupOrientation,
       size: buttonGroupSize,
       variant: buttonGroupVariant,
-      orientation: buttonGroupOrientation,
     } = { ...buttonGroupContext };
-    // Use fallback values if values are null or undefined
+    disabled = (disabled ?? buttonGroupDisabled);
+    orientation = buttonGroupOrientation ?? defaultOrientation;
+    // Use the default value if the value is null or undefined
     size = (size ?? buttonGroupSize) ?? defaultSize;
     variant = (variant ?? buttonGroupVariant) ?? defaultVariant;
-    orientation = buttonGroupOrientation ?? defaultOrientation;
   } else {
     // Use the default value if the value is null or undefined
     size = size ?? defaultSize;
@@ -56,7 +59,7 @@ const Button = forwardRef((inProps, ref) => {
   };
 
   const styleProps = useButtonStyle({
-    orientation, // No default value if not specified
+    orientation, // No default value if not used within `ButtonGroup`
     size,
     variant,
   });

diff --git a/packages/react/src/button/ButtonGroup.js b/packages/react/src/button/ButtonGroup.js
@@ -15,13 +15,19 @@ const defaultVariant = 'default';
 const ButtonGroup = forwardRef((inProps, ref) => {
   const {
     children,
+    disabled,
     orientation = defaultOrientation,
     size = defaultSize,
     variant = defaultVariant,
     ...rest
   } = useDefaultProps({ props: inProps, name: 'ButtonGroup' });
   const styleProps = useButtonGroupStyle({ orientation });
-  const context = getMemoizedState({ orientation, size, variant });
+  const context = getMemoizedState({
+    disabled,
+    orientation,
+    size,
+    variant,
+  });
 
   return (
     <ButtonGroupContext.Provider value={context}>

diff --git a/packages/react/src/checkbox/Checkbox.js b/packages/react/src/checkbox/Checkbox.js
@@ -41,6 +41,7 @@ const Checkbox = forwardRef((inProps, ref) => {
   const inputRef = useRef();
   const combinedInputRef = useMergeRefs(inputRefProp, inputRef);
   const checkboxGroupContext = useCheckboxGroup();
+  const isNameConflictRef = useRef(false);
 
   if (checkboxGroupContext) {
     const {
@@ -54,8 +55,19 @@ const Checkbox = forwardRef((inProps, ref) => {
     if (checkboxGroupValue !== undefined) {
       checked = ensureArray(checkboxGroupValue).includes(value);
     }
-    disabled = checkboxGroupDisabled || disabled;
-    name = checkboxGroupName ?? name;
+    disabled = (disabled ?? checkboxGroupDisabled);
+
+    const isNameConflict = (!isNullish(name) && !isNullish(checkboxGroupName) && (name !== checkboxGroupName));
+    if (process.env.NODE_ENV !== 'production' && isNameConflict && !isNameConflict.current) {
+      // Log the warning message only once
+      console.error(
+        `Warning: The \`Checkbox\` has a \`name\` prop ("${name}") that conflicts with the \`CheckboxGroup\`'s \`name\` prop ("${checkboxGroupName}")`
+      );
+      isNameConflictRef.current = true;
+    }
+
+    name = name ?? checkboxGroupName;
+
     onChange = callAll(
       onChange,
       checkboxGroupOnChange,

diff --git a/packages/react/src/checkbox/__tests__/Checkbox.test.js b/packages/react/src/checkbox/__tests__/Checkbox.test.js
@@ -1,6 +1,6 @@
 import { testA11y } from '@tonic-ui/react/test-utils/accessibility';
 import { render } from '@tonic-ui/react/test-utils/render';
-import { Checkbox } from '@tonic-ui/react/src';
+import { Checkbox, CheckboxGroup } from '@tonic-ui/react/src';
 import React, { useEffect, useRef } from 'react';
 
 describe('Checkbox', () => {
@@ -53,4 +53,24 @@ describe('Checkbox', () => {
       <TestComponent />
     );
   });
+
+  it('should output a warning message when the Checkbox\'s name prop conflicts with the CheckboxGroup\'s name prop', () => {
+    // Mock console.error
+    const consoleErrorMock = jest.spyOn(console, 'error').mockImplementation(() => {});
+
+    const checkboxGroupName = 'checkbox-group';
+    const checkboxName = 'checkbox';
+
+    render(
+      <CheckboxGroup name={checkboxGroupName}>
+        <Checkbox name={checkboxName} />
+      </CheckboxGroup>
+    );
+
+    const expectedErrorMessage = `Warning: The \`Checkbox\` has a \`name\` prop ("${checkboxName}") that conflicts with the \`CheckboxGroup\`'s \`name\` prop ("${checkboxGroupName}")`;
+    expect(consoleErrorMock).toHaveBeenLastCalledWith(expectedErrorMessage);
+
+    // Restore the original console.error
+    consoleErrorMock.mockRestore();
+  });
 });
diff --git a/packages/react/src/radio/Radio.js b/packages/react/src/radio/Radio.js
@@ -39,6 +39,7 @@ const Radio = forwardRef((inProps, ref) => {
   const inputRef = useRef();
   const combinedInputRef = useMergeRefs(inputRefProp, inputRef);
   const radioGroupContext = useRadioGroup();
+  const isNameConflictRef = useRef(false);
 
   if (radioGroupContext) {
     const {
@@ -53,8 +54,18 @@ const Radio = forwardRef((inProps, ref) => {
     if (radioGroupValue !== undefined) {
       checked = (radioGroupValue === value);
     }
-    disabled = radioGroupDisabled || disabled;
-    name = radioGroupName ?? name;
+    disabled = (disabled ?? radioGroupDisabled);
+
+    const isNameConflict = (!isNullish(name) && !isNullish(radioGroupName) && (name !== radioGroupName));
+    if (process.env.NODE_ENV !== 'production' && isNameConflict && !isNameConflict.current) {
+      // Log the warning message only once
+      console.error(
+        `Warning: The \`Radio\` has a \`name\` prop ("${name}") that conflicts with the \`RadioGroup\`'s \`name\` prop ("${radioGroupName}")`
+      );
+      isNameConflictRef.current = true;
+    }
+    name = (name ?? radioGroupName);
+
     onChange = callAll(
       onChange,
       radioGroupOnChange,

diff --git a/packages/react/src/radio/__tests__/Radio.test.js b/packages/react/src/radio/__tests__/Radio.test.js
@@ -1,6 +1,6 @@
 import { testA11y } from '@tonic-ui/react/test-utils/accessibility';
 import { render } from '@tonic-ui/react/test-utils/render';
-import { Radio } from '@tonic-ui/react/src';
+import { Radio, RadioGroup } from '@tonic-ui/react/src';
 import React, { useEffect, useRef } from 'react';
 
 describe('Radio', () => {
@@ -51,4 +51,24 @@ describe('Radio', () => {
       <TestComponent />
     );
   });
+
+  it('should output a warning message when the Radio\'s name prop conflicts with the RadioGroup\'s name prop', () => {
+    // Mock console.error
+    const consoleErrorMock = jest.spyOn(console, 'error').mockImplementation(() => {});
+
+    const radioGroupName = 'radio-group';
+    const radioName = 'radio';
+
+    render(
+      <RadioGroup name={radioGroupName}>
+        <Radio name={radioName} />
+      </RadioGroup>
+    );
+
+    const expectedErrorMessage = `Warning: The \`Radio\` has a \`name\` prop ("${radioName}") that conflicts with the \`RadioGroup\`'s \`name\` prop ("${radioGroupName}")`;
+    expect(consoleErrorMock).toHaveBeenLastCalledWith(expectedErrorMessage);
+
+    // Restore the original console.error
+    consoleErrorMock.mockRestore();
+  });
 });