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 docked variant button. For use in docked navigation. */
isDocked?: 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,
isDocked = 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,
isDocked && styles.modifiers.dock, // Replace wwith docked class from https://github.com/patternfly/patternfly/pull/8308
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,39 @@ describe('Favorite button', () => {
});
});

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

test(`Does not render with class ${styles.modifiers.dock} when isDocked 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 thread
coderabbitai[bot] marked this conversation as resolved.

test(`Renders with both ${styles.modifiers.dock} and ${styles.modifiers.textExpanded} when both props are true`, () => {
render(
<Button isDocked isTextExpanded>
Dock Text Expanded Button
</Button>
);
const button = screen.getByRole('button');
expect(button).toHaveClass(styles.modifiers.dock);
expect(button).toHaveClass(styles.modifiers.textExpanded);
});
});

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-:r35:"
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 @@ -53,6 +53,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 menu toggle is a docked variant. For use in docked navigation. */
isDocked?: 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 @@ -88,6 +92,8 @@ class MenuToggleBase extends Component<MenuToggleProps> {
isInForm: false,
isPlaceholder: false,
isCircle: false,
isDocked: false,
isTextExpanded: false,
size: 'default',
ouiaSafe: true
};
Expand All @@ -106,6 +112,8 @@ class MenuToggleBase extends Component<MenuToggleProps> {
isPlaceholder,
isCircle,
isSettings,
isDocked,
isTextExpanded,
splitButtonItems,
variant,
status,
Expand Down Expand Up @@ -190,6 +198,8 @@ class MenuToggleBase extends Component<MenuToggleProps> {
isDisabled && styles.modifiers.disabled,
isPlaceholder && styles.modifiers.placeholder,
isSettings && styles.modifiers.settings,
isDocked && styles.modifiers.dock, // Replace wwith docked class from https://github.com/patternfly/patternfly/pull/8308
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 @@ -155,3 +155,34 @@ 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 isDocked is passed`, () => {
render(<MenuToggle isDocked>Dock Toggle</MenuToggle>);
expect(screen.getByRole('button')).toHaveClass(styles.modifiers.dock);
});

test(`Does not render with class ${styles.modifiers.dock} when isDocked 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);
});

test(`Renders with both ${styles.modifiers.dock} and ${styles.modifiers.textExpanded} when both props are passed`, () => {
render(
<MenuToggle isDocked isTextExpanded>
Dock Text Expanded Toggle
</MenuToggle>
);
const button = screen.getByRole('button');
expect(button).toHaveClass(styles.modifiers.dock);
expect(button).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 docked nav is expanded on mobile. Only applies when variant is docked. */
isDockExpanded?: boolean;
/** @beta Flag indicating the docked nav should display text on desktop. Only applies when variant is docked. */
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