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();
+ });
 });