Removed restriction of variant and children content

Author: Eric Olkowski

packages/react-core/src/components/Button/Button.tsx

@@ -153,11 +153,8 @@ const ButtonBase: React.FunctionComponent<ButtonProps> = ({
       'Button: when the isHamburger property is passed in, you must also pass in a boolean value to the isExpanded property. It is expected that a hamburger button controls the expansion of other content.'
     );
   }
-  // TODO: Remove isSettings in breaking change to throw this warning for any non-hamburger button that does not have children or aria-label
-  if (
-    (isHamburger && !ariaLabel && !props['aria-labelledby']) ||
-    (isSettings && !ariaLabel && !children && !props['aria-labelledby'])
-  ) {
+  // TODO: Remove isSettings || isHamburger conditional in breaking change to throw this warning for any button that does not have children or aria name
+  if ((isSettings || isHamburger) && !ariaLabel && !children && !props['aria-labelledby']) {
     // eslint-disable-next-line no-console
     console.error(
       'Button: you must provide either visible text content or an accessible name via the aria-label or aria-labelledby properties.'
@@ -169,7 +166,6 @@ const ButtonBase: React.FunctionComponent<ButtonProps> = ({
   const isButtonElement = Component === 'button';
   const isInlineSpan = isInline && Component === 'span';
   const isIconAlignedAtEnd = iconPosition === 'end' || iconPosition === 'right';
-  const shouldForcePlainVariant = isSettings || isHamburger;
   const shouldOverrideIcon = isSettings || isHamburger;
 
   const preventedEvents = inoperableEvents.reduce(
@@ -214,7 +210,7 @@ const ButtonBase: React.FunctionComponent<ButtonProps> = ({
     );
   };
   const _icon = renderIcon();
-  const _children = children && !isHamburger && <span className={css('pf-v6-c-button__text')}>{children}</span>;
+  const _children = children && <span className={css('pf-v6-c-button__text')}>{children}</span>;
   // We only want to render the aria-disabled attribute when true, similar to the disabled attribute natively.
   const shouldRenderAriaDisabled = isAriaDisabled || (!isButtonElement && isDisabled);
 
@@ -227,7 +223,7 @@ const ButtonBase: React.FunctionComponent<ButtonProps> = ({
       aria-label={ariaLabel}
       className={css(
         styles.button,
-        shouldForcePlainVariant ? styles.modifiers.plain : styles.modifiers[variant],
+        styles.modifiers[variant],
         isSettings && styles.modifiers.settings,
         isHamburger && styles.modifiers.hamburger,
         isHamburger && hamburgerVariant && styles.modifiers[hamburgerVariant],

packages/react-core/src/components/Button/__tests__/Button.test.tsx

@@ -306,7 +306,8 @@ describe('Hamburger button', () => {
     );
   });
 
-  test('Throws console error when isHamburger is true and neither aria-label nor aria-lablledby are passed', () => {
+  // TODO: Remove isHamburger in breaking change to throw error for any button that does not have children or aria name
+  test('Throws console error when isHamburger is true and neither children, aria-label nor aria-lablledby are passed', () => {
     const consoleError = jest.spyOn(console, 'error').mockImplementation();
 
     render(<Button isHamburger isExpanded />);
@@ -315,7 +316,22 @@ describe('Hamburger button', () => {
       'Button: you must provide either visible text content or an accessible name via the aria-label or aria-labelledby properties.'
     );
   });
+  // TODO: Remove isHamburger in breaking change to throw error for any button that does not have children or aria name
+  test('Does not throw console error when isHamburger is true and children is passed', () => {
+    const consoleError = jest.spyOn(console, 'error').mockImplementation();
+
+    render(
+      <Button isHamburger isExpanded>
+        Test
+      </Button>
+    );
 
+    expect(consoleError).not.toHaveBeenCalledWith(
+      'Button: you must provide either visible text content or an accessible name via the aria-label or aria-labelledby properties.'
+    );
+  });
+
+  // TODO: Remove isHamburger in breaking change to throw error for any button that does not have children or aria name
   test('Does not throw console error when isHamburger is true and aria-label is passed', () => {
     const consoleError = jest.spyOn(console, 'error').mockImplementation();
 
@@ -326,6 +342,7 @@ describe('Hamburger button', () => {
     );
   });
 
+  // TODO: Remove isHamburger in breaking change to throw error for any button that does not have children or aria name
   test('Does not throw console error when isHamburger is true and aria-labelledby is passed', () => {
     const consoleError = jest.spyOn(console, 'error').mockImplementation();
 
@@ -341,19 +358,32 @@ describe('Hamburger button', () => {
     );
   });
 
+  test(`Does not render with class ${styles.modifiers.hamburger} by default`, () => {
+    render(<Button aria-label="test" />);
+
+    expect(screen.getByRole('button')).not.toHaveClass(styles.modifiers.hamburger);
+  });
+
   test(`Renders with class ${styles.modifiers.hamburger} when isHamburger is true`, () => {
     render(<Button isHamburger isExpanded aria-label="test" />);
 
     expect(screen.getByRole('button')).toHaveClass(styles.modifiers.hamburger);
   });
 
-  test('Does not render with hamburgerVariant class when isHamburger is false', () => {
+  test('Does not render with hamburgerVariant class when isHamburger is true and hamburgerVariant is not passed', () => {
     render(<Button isHamburger isExpanded aria-label="test" />);
 
     expect(screen.getByRole('button')).not.toHaveClass(styles.modifiers.expand);
     expect(screen.getByRole('button')).not.toHaveClass(styles.modifiers.collapse);
   });
 
+  test('Does not render with hamburgerVariant class when isHamburger is false and hamburgerVariant is passed', () => {
+    render(<Button hamburgerVariant="expand" isExpanded aria-label="test" />);
+
+    expect(screen.getByRole('button')).not.toHaveClass(styles.modifiers.expand);
+    expect(screen.getByRole('button')).not.toHaveClass(styles.modifiers.collapse);
+  });
+
   test(`Renders with class ${styles.modifiers.expand} when isHamburger is true and hamburgerVariant = expand`, () => {
     render(<Button isHamburger hamburgerVariant="expand" isExpanded aria-label="test" />);
 
@@ -368,32 +398,15 @@ describe('Hamburger button', () => {
     expect(screen.getByRole('button')).toHaveClass(styles.modifiers.collapse);
   });
 
-  test('Forces plain variant when isHamburger is passed', () => {
-    render(<Button variant="secondary" isHamburger isExpanded aria-label="test" />);
-
-    expect(screen.getByRole('button')).not.toHaveClass(styles.modifiers.secondary);
-    expect(screen.getByRole('button')).toHaveClass(styles.modifiers.plain);
-  });
-
   test('Does not render custom icon when icon prop and isHamburger are passed', () => {
     render(<Button isHamburger isExpanded aria-label="test" icon={<div>Custom icon</div>} />);
 
     expect(screen.queryByText('Custom icon')).not.toBeInTheDocument();
   });
-
-  test('Does not render text content when isHamburger is passed', () => {
-    render(
-      <Button isHamburger isExpanded aria-label="test">
-        Hamburger text content
-      </Button>
-    );
-
-    expect(screen.queryByText('Hamburger text content')).not.toBeInTheDocument();
-  });
 });
 
 describe('Settings button', () => {
-  // TODO: Remove isSettings in breaking change to throw error for any non-hamburger button that does not have children or aria-label
+  // TODO: Remove isSettings in breaking change to throw error for any button that does not have children or aria name
   test('Throws console error when isSettings is true and neither children, aria-label nor aria-lablledby are passed', () => {
     const consoleError = jest.spyOn(console, 'error').mockImplementation();
 
@@ -404,7 +417,7 @@ describe('Settings button', () => {
     );
   });
 
-  // TODO: Remove isSettings in breaking change to throw error for any non-hamburger button that does not have children or aria-label
+  // TODO: Remove isSettings in breaking change to throw error for any button that does not have children or aria name
   test('Does not throw console error when isSettings is true and children is passed', () => {
     const consoleError = jest.spyOn(console, 'error').mockImplementation();
 
@@ -415,7 +428,7 @@ describe('Settings button', () => {
     );
   });
 
-  // TODO: Remove isSettings in breaking change to throw error for any non-hamburger button that does not have children or aria-label
+  // TODO: Remove isSettings in breaking change to throw error for any button that does not have children or aria name
   test('Does not throw console error when isSettings is true and aria-label is passed', () => {
     const consoleError = jest.spyOn(console, 'error').mockImplementation();
 
@@ -426,7 +439,7 @@ describe('Settings button', () => {
     );
   });
 
-  // TODO: Remove isSettings in breaking change to throw error for any non-hamburger button that does not have children or aria-label
+  // TODO: Remove isSettings in breaking change to throw error for any button that does not have children or aria name
   test('Does not throw console error when isSettings is true and aria-labelledby is passed', () => {
     const consoleError = jest.spyOn(console, 'error').mockImplementation();
 
@@ -442,17 +455,16 @@ describe('Settings button', () => {
     );
   });
 
-  test(`Renders with class ${styles.modifiers.settings} when isSettings is true`, () => {
-    render(<Button isSettings aria-label="test" />);
+  test(`Does not render with class ${styles.modifiers.settings} by default`, () => {
+    render(<Button aria-label="test" />);
 
-    expect(screen.getByRole('button')).toHaveClass(styles.modifiers.settings);
+    expect(screen.getByRole('button')).not.toHaveClass(styles.modifiers.settings);
   });
 
-  test('Forces plain variant when isSettings is passed', () => {
-    render(<Button variant="secondary" isSettings aria-label="test" />);
+  test(`Renders with class ${styles.modifiers.settings} when isSettings is true`, () => {
+    render(<Button isSettings aria-label="test" />);
 
-    expect(screen.getByRole('button')).not.toHaveClass(styles.modifiers.secondary);
-    expect(screen.getByRole('button')).toHaveClass(styles.modifiers.plain);
+    expect(screen.getByRole('button')).toHaveClass(styles.modifiers.settings);
   });
 
   test('Does not render custom icon when icon prop and isSettings are passed', () => {

packages/react-core/src/components/Button/__tests__/__snapshots__/Button.test.tsx.snap

@@ -5,7 +5,7 @@ exports[`Renders basic button 1`] = `
   <button
     aria-label="basic button"
     class="pf-v6-c-button pf-m-primary"
-    data-ouia-component-id="OUIA-Generated-Button-primary-54"
+    data-ouia-component-id="OUIA-Generated-Button-primary-57"
     data-ouia-component-type="PF6/Button"
     data-ouia-safe="true"
     type="button"