Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions packages/react-core/src/components/Button/Button.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,10 @@ export interface ButtonProps extends Omit<React.HTMLProps<HTMLButtonElement>, 'r
hamburgerVariant?: 'expand' | 'collapse';
/** @beta Flag indicating the button is a circle button. Intended for buttons that only contain an icon.. */
isCircle?: boolean;
/** @beta Flag indicating the button is a dock variant button. For use in docked navigation. */
isDock?: boolean;
/** @beta Flag indicating the dock button should display text. Only applies when isDock is true. */
isTextExpanded?: boolean;
/** @hide Forwarded ref */
innerRef?: React.Ref<any>;
/** Adds count number to button */
Expand All @@ -134,6 +138,8 @@ const ButtonBase: React.FunctionComponent<ButtonProps> = ({
isHamburger,
hamburgerVariant,
isCircle,
isDock = false,
isTextExpanded = false,
spinnerAriaValueText,
spinnerAriaLabelledBy,
spinnerAriaLabel,
Expand Down Expand Up @@ -265,6 +271,8 @@ const ButtonBase: React.FunctionComponent<ButtonProps> = ({
size === ButtonSize.sm && styles.modifiers.small,
size === ButtonSize.lg && styles.modifiers.displayLg,
isCircle && styles.modifiers.circle,
isDock && styles.modifiers.dock,
isTextExpanded && styles.modifiers.textExpanded,
className
)}
disabled={isButtonElement ? isDisabled : null}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -555,6 +555,28 @@ describe('Favorite button', () => {
});
});

describe('Dock variant', () => {
test(`Renders with class ${styles.modifiers.dock} when isDock = true`, () => {
render(<Button isDock>Dock Button</Button>);
expect(screen.getByRole('button')).toHaveClass(styles.modifiers.dock);
});

test(`Does not render with class ${styles.modifiers.dock} when isDock is not passed`, () => {
render(<Button>Button</Button>);
expect(screen.getByRole('button')).not.toHaveClass(styles.modifiers.dock);
});

test(`Renders with class ${styles.modifiers.textExpanded} when isTextExpanded = true`, () => {
render(<Button isTextExpanded>Text Expanded Button</Button>);
expect(screen.getByRole('button')).toHaveClass(styles.modifiers.textExpanded);
});

test(`Does not render with class ${styles.modifiers.textExpanded} when isTextExpanded is not passed`, () => {
render(<Button>Button</Button>);
expect(screen.getByRole('button')).not.toHaveClass(styles.modifiers.textExpanded);
});
Comment on lines +569 to +577
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Apr 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Align isTextExpanded assertions with dock-only usage.

This case currently verifies isTextExpanded without isDock, which drifts from the documented dock-only behavior and can lock in an unintended contract.

Suggested test adjustment
-  test(`Renders with class ${styles.modifiers.textExpanded} when isTextExpanded = true`, () => {
-    render(<Button isTextExpanded>Text Expanded Button</Button>);
+  test(`Renders with class ${styles.modifiers.textExpanded} when isDock and isTextExpanded are true`, () => {
+    render(<Button isDock isTextExpanded>Text Expanded Button</Button>);
     expect(screen.getByRole('button')).toHaveClass(styles.modifiers.textExpanded);
   });
 
-  test(`Does not render with class ${styles.modifiers.textExpanded} when isTextExpanded is not passed`, () => {
-    render(<Button>Button</Button>);
+  test(`Does not render with class ${styles.modifiers.textExpanded} when isDock is true and isTextExpanded is not passed`, () => {
+    render(<Button isDock>Button</Button>);
     expect(screen.getByRole('button')).not.toHaveClass(styles.modifiers.textExpanded);
   });
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/react-core/src/components/Button/__tests__/Button.test.tsx` around
lines 569 - 577, Tests assert the styles.modifiers.textExpanded class for
isTextExpanded without setting dock context; update the two tests in
Button.test.tsx to reflect dock-only behavior by passing isDock={true} when
expecting the class (render(<Button isDock isTextExpanded>...)) and keeping a
test that omits isDock to assert the class is not present (render(<Button
isTextExpanded>...) expect.not.toHaveClass(styles.modifiers.textExpanded)),
referencing the Button component and props isTextExpanded and isDock and the
styles.modifiers.textExpanded symbol so the assertions match the documented
dock-only contract.

Copy link
Copy Markdown
Contributor Author

@thatblindgeye thatblindgeye Apr 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mcoker is the expectation that we only allow consumers to render the pf-m-text-expanded class only for dock variant buttons/nav/etc? Right now the various components that have both a pf-m-dock and pf-m-text-expanded class, either modifier can be applied independently (you don't need isDock in order to render the class from isTextExpanded)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

});

test(`Renders basic button`, () => {
const { asFragment } = render(<Button aria-label="basic button">Basic Button</Button>);
expect(asFragment()).toMatchSnapshot();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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-:r30:"
data-ouia-component-id="OUIA-Generated-Button-primary-:r34:"
data-ouia-component-type="PF6/Button"
data-ouia-safe="true"
type="button"
Expand Down
9 changes: 8 additions & 1 deletion packages/react-core/src/components/Masthead/MastheadLogo.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,15 @@ export interface MastheadLogoProps extends React.DetailedHTMLProps<
className?: string;
/** Component type of the masthead logo. */
component?: React.ElementType<any> | React.ComponentType<any>;
/** @beta Flag indicating the logo is a compact variant. Used in docked layouts. */
isCompact?: boolean;
}

export const MastheadLogo: React.FunctionComponent<MastheadLogoProps> = ({
children,
className,
component,
isCompact = false,
...props
}: MastheadLogoProps) => {
let Component = component as any;
Expand All @@ -28,7 +31,11 @@ export const MastheadLogo: React.FunctionComponent<MastheadLogoProps> = ({
}
}
return (
<Component className={css(styles.mastheadLogo, className)} {...(Component === 'a' && { tabIndex: 0 })} {...props}>
<Component
className={css(styles.mastheadLogo, isCompact && styles.modifiers.compact, className)}
{...(Component === 'a' && { tabIndex: 0 })}
{...props}
>
{children}
</Component>
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,29 @@ describe('MastheadLogo', () => {

expect(asFragment()).toMatchSnapshot();
});

test(`Renders with ${styles.modifiers.compact} class when isCompact is true`, () => {
render(
<MastheadLogo isCompact data-testid="compact-logo">
test
</MastheadLogo>
);
expect(screen.getByTestId('compact-logo')).toHaveClass(styles.modifiers.compact);
});

test(`Does not render with ${styles.modifiers.compact} class when isCompact is false`, () => {
render(
<MastheadLogo isCompact={false} data-testid="logo">
test
</MastheadLogo>
);
expect(screen.getByTestId('logo')).not.toHaveClass(styles.modifiers.compact);
});

test(`Does not render with ${styles.modifiers.compact} class when isCompact is not passed`, () => {
render(<MastheadLogo data-testid="logo">test</MastheadLogo>);
expect(screen.getByTestId('logo')).not.toHaveClass(styles.modifiers.compact);
});
});

describe('MastheadContent', () => {
Expand Down
10 changes: 10 additions & 0 deletions packages/react-core/src/components/MenuToggle/MenuToggle.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,10 @@ export interface MenuToggleProps
isCircle?: boolean;
/** Flag indicating whether the toggle is a settings toggle. This will override the icon property */
isSettings?: boolean;
/** @beta Flag indicating the toggle is a dock variant. For use in docked navigation. */
isDock?: boolean;
/** @beta Flag indicating the dock toggle should display text. Only applies when isDock is true. */
isTextExpanded?: boolean;
/** Elements to display before the toggle button. When included, renders the menu toggle as a split button. */
splitButtonItems?: React.ReactNode[];
/** Variant styles of the menu toggle */
Expand Down Expand Up @@ -85,6 +89,8 @@ class MenuToggleBase extends Component<MenuToggleProps> {
isFullHeight: false,
isPlaceholder: false,
isCircle: false,
isDock: false,
isTextExpanded: false,
size: 'default',
ouiaSafe: true
};
Expand All @@ -102,6 +108,8 @@ class MenuToggleBase extends Component<MenuToggleProps> {
isPlaceholder,
isCircle,
isSettings,
isDock,
isTextExpanded,
splitButtonItems,
variant,
status,
Expand Down Expand Up @@ -185,6 +193,8 @@ class MenuToggleBase extends Component<MenuToggleProps> {
isDisabled && styles.modifiers.disabled,
isPlaceholder && styles.modifiers.placeholder,
isSettings && styles.modifiers.settings,
isDock && styles.modifiers.dock,
isTextExpanded && styles.modifiers.textExpanded,
size === MenuToggleSize.sm && styles.modifiers.small,
className
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -138,3 +138,23 @@ test('Does not render custom icon when icon prop and isSettings are passed', ()
);
expect(screen.queryByText('Custom icon')).not.toBeInTheDocument();
});

test(`Renders with class ${styles.modifiers.dock} when isDock is passed`, () => {
render(<MenuToggle isDock>Dock Toggle</MenuToggle>);
expect(screen.getByRole('button')).toHaveClass(styles.modifiers.dock);
});

test(`Does not render with class ${styles.modifiers.dock} when isDock is not passed`, () => {
render(<MenuToggle>Toggle</MenuToggle>);
expect(screen.getByRole('button')).not.toHaveClass(styles.modifiers.dock);
});

test(`Renders with class ${styles.modifiers.textExpanded} when isTextExpanded is passed`, () => {
render(<MenuToggle isTextExpanded>Text Expanded Toggle</MenuToggle>);
expect(screen.getByRole('button')).toHaveClass(styles.modifiers.textExpanded);
});

test(`Does not render with class ${styles.modifiers.textExpanded} when isTextExpanded is not passed`, () => {
render(<MenuToggle>Toggle</MenuToggle>);
expect(screen.getByRole('button')).not.toHaveClass(styles.modifiers.textExpanded);
});
4 changes: 4 additions & 0 deletions packages/react-core/src/components/Nav/Nav.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,8 @@ export interface NavProps
'aria-label'?: string;
/** The nav variant to use. Docked is in beta. */
variant?: 'default' | 'horizontal' | 'horizontal-subnav' | 'docked';
/** @beta Flag indicating the docked nav should display text. Only applies when variant is docked. */
isTextExpanded?: boolean;
/** Value to overwrite the randomly generated data-ouia-component-id.*/
ouiaId?: number | string;
/** Set the value of data-ouia-safe. Only set to true when the component is in a static state, i.e. no animations are occurring. At all other times, this value must be false. */
Expand Down Expand Up @@ -119,6 +121,7 @@ class Nav extends Component<NavProps, { isScrollable: boolean; flyoutRef: React.
ouiaId,
ouiaSafe,
variant,
isTextExpanded = false,
...props
} = this.props;
const isHorizontal = ['horizontal', 'horizontal-subnav'].includes(variant);
Expand Down Expand Up @@ -156,6 +159,7 @@ class Nav extends Component<NavProps, { isScrollable: boolean; flyoutRef: React.
isHorizontal && styles.modifiers.horizontal,
variant === 'docked' && styles.modifiers.docked,
variant === 'horizontal-subnav' && styles.modifiers.subnav,
isTextExpanded && styles.modifiers.textExpanded,
this.state.isScrollable && styles.modifiers.scrollable,
className
)}
Expand Down
30 changes: 30 additions & 0 deletions packages/react-core/src/components/Nav/__tests__/Nav.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -274,4 +274,34 @@ describe('Nav', () => {
);
expect(screen.getByTestId('docked-nav')).toHaveClass(styles.modifiers.docked);
});

test(`Renders with ${styles.modifiers.textExpanded} class when isTextExpanded is true`, () => {
renderNav(
<Nav isTextExpanded data-testid="text-expanded-nav">
<NavList>
{props.items.map((item) => (
<NavItem to={item.to} key={item.to}>
{item.label}
</NavItem>
))}
</NavList>
</Nav>
);
expect(screen.getByTestId('text-expanded-nav')).toHaveClass(styles.modifiers.textExpanded);
});

test(`Does not render with ${styles.modifiers.textExpanded} class when isTextExpanded is not passed`, () => {
renderNav(
<Nav data-testid="nav">
<NavList>
{props.items.map((item) => (
<NavItem to={item.to} key={item.to}>
{item.label}
</NavItem>
))}
</NavList>
</Nav>
);
expect(screen.getByTestId('nav')).not.toHaveClass(styles.modifiers.textExpanded);
});
Comment on lines +278 to +306
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

isTextExpanded tests should run in docked variant context.

These tests currently validate isTextExpanded on a non-docked Nav, which conflicts with the docked-only contract and risks cementing unsupported behavior.

Suggested test correction
-  test(`Renders with ${styles.modifiers.textExpanded} class when isTextExpanded is true`, () => {
+  test(`Renders with ${styles.modifiers.textExpanded} class when variant is docked and isTextExpanded is true`, () => {
     renderNav(
-      <Nav isTextExpanded data-testid="text-expanded-nav">
+      <Nav variant="docked" isTextExpanded data-testid="text-expanded-nav">
         <NavList>
           {props.items.map((item) => (
             <NavItem to={item.to} key={item.to}>
               {item.label}
             </NavItem>
           ))}
         </NavList>
       </Nav>
     );
     expect(screen.getByTestId('text-expanded-nav')).toHaveClass(styles.modifiers.textExpanded);
   });
 
-  test(`Does not render with ${styles.modifiers.textExpanded} class when isTextExpanded is not passed`, () => {
+  test(`Does not render with ${styles.modifiers.textExpanded} class when variant is docked and isTextExpanded is not passed`, () => {
     renderNav(
-      <Nav data-testid="nav">
+      <Nav variant="docked" data-testid="nav">
         <NavList>
           {props.items.map((item) => (
             <NavItem to={item.to} key={item.to}>
               {item.label}
             </NavItem>
           ))}
         </NavList>
       </Nav>
     );
     expect(screen.getByTestId('nav')).not.toHaveClass(styles.modifiers.textExpanded);
   });
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
test(`Renders with ${styles.modifiers.textExpanded} class when isTextExpanded is true`, () => {
renderNav(
<Nav isTextExpanded data-testid="text-expanded-nav">
<NavList>
{props.items.map((item) => (
<NavItem to={item.to} key={item.to}>
{item.label}
</NavItem>
))}
</NavList>
</Nav>
);
expect(screen.getByTestId('text-expanded-nav')).toHaveClass(styles.modifiers.textExpanded);
});
test(`Does not render with ${styles.modifiers.textExpanded} class when isTextExpanded is not passed`, () => {
renderNav(
<Nav data-testid="nav">
<NavList>
{props.items.map((item) => (
<NavItem to={item.to} key={item.to}>
{item.label}
</NavItem>
))}
</NavList>
</Nav>
);
expect(screen.getByTestId('nav')).not.toHaveClass(styles.modifiers.textExpanded);
});
test(`Renders with ${styles.modifiers.textExpanded} class when variant is docked and isTextExpanded is true`, () => {
renderNav(
<Nav variant="docked" isTextExpanded data-testid="text-expanded-nav">
<NavList>
{props.items.map((item) => (
<NavItem to={item.to} key={item.to}>
{item.label}
</NavItem>
))}
</NavList>
</Nav>
);
expect(screen.getByTestId('text-expanded-nav')).toHaveClass(styles.modifiers.textExpanded);
});
test(`Does not render with ${styles.modifiers.textExpanded} class when variant is docked and isTextExpanded is not passed`, () => {
renderNav(
<Nav variant="docked" data-testid="nav">
<NavList>
{props.items.map((item) => (
<NavItem to={item.to} key={item.to}>
{item.label}
</NavItem>
))}
</NavList>
</Nav>
);
expect(screen.getByTestId('nav')).not.toHaveClass(styles.modifiers.textExpanded);
});
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/react-core/src/components/Nav/__tests__/Nav.test.tsx` around lines
278 - 306, The tests for isTextExpanded are asserting docked-only behavior but
render a non-docked Nav; update both tests to render the Nav in the docked
variant context so they validate the correct contract—e.g. in the two tests that
call renderNav and render a <Nav ...> component, include the docked variant
(pass variant="docked" or the equivalent docked prop your Nav API accepts) when
constructing the Nav (the tests referencing Nav, isTextExpanded, renderNav, and
styles.modifiers.textExpanded) so the assertions run against a docked Nav.

});
26 changes: 22 additions & 4 deletions packages/react-core/src/components/Page/Page.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,14 @@ export interface PageProps extends React.HTMLProps<HTMLDivElement> {
className?: string;
/** @beta Indicates the layout variant */
variant?: 'default' | 'docked';
/** Masthead component (e.g. <Masthead />) */
/** @beta Flag indicating the dock nav is expanded on mobile. Only applies when variant is dock. */
isDockExpanded?: boolean;
/** @beta Flag indicating the dock nav should display text. Only applies when variant is dock. */
isDockTextExpanded?: boolean;
/** The horizontal masthead content (e.g. <Masthead />). When using the dock variant, this content will only render at mobile viewports. */
masthead?: React.ReactNode;
/** @beta Content to render in the vertical dock when variant of dock is used. At mobile viewports, this content will be replaced with the content passed to masthead. */
dockContent?: React.ReactNode;
/** Sidebar component for a side nav, recommended to be a PageSidebar. If set to null, the page grid layout
* will render without a sidebar.
*/
Expand Down Expand Up @@ -232,7 +238,10 @@ class Page extends Component<PageProps, PageState> {
className,
children,
variant,
isDockExpanded = false,
isDockTextExpanded = false,
masthead,
dockContent,
sidebar,
notificationDrawer,
isNotificationDrawerExpanded,
Expand Down Expand Up @@ -349,9 +358,18 @@ class Page extends Component<PageProps, PageState> {
>
{skipToContent}
{variant === 'docked' ? (
<div className={css(styles.pageDock)}>
<div className={css(styles.pageDockMain)}>{masthead}</div>
</div>
<>
{masthead}
<div
className={css(
styles.pageDock,
isDockExpanded && styles.modifiers.expanded,
isDockTextExpanded && styles.modifiers.textExpanded
)}
>
<div className={css(styles.pageDockMain)}>{dockContent}</div>
</div>
</>
) : (
masthead
)}
Expand Down
Loading
Loading