Skip to content
Merged
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
2 changes: 1 addition & 1 deletion packages/react-core/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@
"tslib": "^2.8.1"
},
"devDependencies": {
"@patternfly/patternfly": "6.5.0-prerelease.27",
"@patternfly/patternfly": "6.5.0-prerelease.33",
"case-anything": "^3.1.2",
"css": "^3.0.0",
"fs-extra": "^11.3.0"
Expand Down
68 changes: 42 additions & 26 deletions packages/react-core/src/components/Compass/Compass.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ import compassBackgroundImageDark from '@patternfly/react-tokens/dist/esm/c_comp
export interface CompassProps extends React.HTMLProps<HTMLDivElement> {
/** Additional classes added to the Compass. */
className?: string;
/** Content of the docked navigation area of the layout */
dock?: React.ReactNode;
/** Content placed at the top of the layout */
header?: React.ReactNode;
/** Flag indicating if the header is expanded */
Expand Down Expand Up @@ -38,6 +40,7 @@ export interface CompassProps extends React.HTMLProps<HTMLDivElement> {

export const Compass: React.FunctionComponent<CompassProps> = ({
className,
dock,
header,
isHeaderExpanded = true,
sidebarStart,
Expand All @@ -64,32 +67,45 @@ export const Compass: React.FunctionComponent<CompassProps> = ({
}

const compassContent = (
<div className={css(styles.compass, className)} {...props} style={{ ...props.style, ...backgroundImageStyles }}>
<div
className={css(styles.compassHeader, isHeaderExpanded && 'pf-m-expanded')}
{...(!isHeaderExpanded && { inert: 'true' })}
>
{header}
</div>
<div
className={css(styles.compassSidebar, styles.modifiers.start, isSidebarStartExpanded && 'pf-m-expanded')}
{...(!isSidebarStartExpanded && { inert: 'true' })}
>
{sidebarStart}
</div>
<div className={css(styles.compassMain)}>{main}</div>
<div
className={css(styles.compassSidebar, styles.modifiers.end, isSidebarEndExpanded && 'pf-m-expanded')}
{...(!isSidebarEndExpanded && { inert: 'true' })}
>
{sidebarEnd}
</div>
<div
className={css(styles.compassFooter, isFooterExpanded && 'pf-m-expanded')}
{...(!isFooterExpanded && { inert: 'true' })}
>
{footer}
</div>
<div
className={css(styles.compass, dock !== undefined && styles.modifiers.dock, className)}
{...props}
style={{ ...props.style, ...backgroundImageStyles }}
>
{dock && <div className={css(`${styles.compass}__dock`)}>{dock}</div>}
{header && (
<div
className={css(styles.compassHeader, isHeaderExpanded && 'pf-m-expanded')}
{...(!isHeaderExpanded && { inert: 'true' })}
>
{header}
</div>
)}
{sidebarStart && (
<div
className={css(styles.compassSidebar, styles.modifiers.start, isSidebarStartExpanded && 'pf-m-expanded')}
{...(!isSidebarStartExpanded && { inert: 'true' })}
>
{sidebarStart}
</div>
)}
{main && <div className={css(styles.compassMain)}>{main}</div>}
{sidebarEnd && (
<div
className={css(styles.compassSidebar, styles.modifiers.end, isSidebarEndExpanded && 'pf-m-expanded')}
{...(!isSidebarEndExpanded && { inert: 'true' })}
>
{sidebarEnd}
</div>
)}
{footer && (
<div
className={css(styles.compassFooter, isFooterExpanded && 'pf-m-expanded')}
{...(!isFooterExpanded && { inert: 'true' })}
>
{footer}
</div>
)}
</div>
);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -170,3 +170,13 @@ test('Matches the snapshot with drawer', () => {
);
expect(asFragment()).toMatchSnapshot();
});

test(`Renders with ${styles.modifiers.dock} class when dock is passed`, () => {
render(<Compass dock={<div>Dock content</div>} data-testid="compass" />);
expect(screen.getByTestId('compass')).toHaveClass(styles.modifiers.dock);
});

test(`Does not render with ${styles.modifiers.dock} class when dock is not passed`, () => {
render(<Compass data-testid="compass" />);
expect(screen.getByTestId('compass')).not.toHaveClass(styles.modifiers.dock);
});
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ propComponents:
]
---

import { useRef, useState } from 'react';
import { useRef, useState, useEffect } from 'react';
import PlayIcon from '@patternfly/react-icons/dist/esm/icons/play-icon';
import OutlinedPlusSquare from '@patternfly/react-icons/dist/esm/icons/outlined-plus-square-icon';
import OutlinedCopy from '@patternfly/react-icons/dist/esm/icons/outlined-copy-icon';
Expand Down Expand Up @@ -51,6 +51,12 @@ When `footer` is used, its content will fill the width of the screen. By default

```

### With docked nav

```ts file="CompassDockLayout.tsx"

```

## Composable structure

When building a more custom implementation with Compass components, there are some intended or expected structures that must remain present.
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
import {
Compass,
CompassContent,
CompassMainHeader,
CompassPanel,
CompassMainHeaderContent
} from '@patternfly/react-core';
import './compass.css';

export const CompassBasic: React.FunctionComponent = () => {
const dockContent = <div>Content</div>;
const mainContent = (
<CompassContent>
<CompassMainHeader>
<CompassPanel>
<CompassMainHeaderContent>
<div>Content title</div>
</CompassMainHeaderContent>
</CompassPanel>
</CompassMainHeader>
<div>Content</div>
</CompassContent>
);
return <Compass dock={dockContent} main={mainContent} />;
};
12 changes: 12 additions & 0 deletions packages/react-core/src/components/Compass/examples/compass.css
Original file line number Diff line number Diff line change
Expand Up @@ -20,4 +20,16 @@
inset: 0;
border: var(--pf-t--global--border--width--regular) dashed var(--pf-t--global--border--color--default);
pointer-events: none;
}

#ws-react-a-compass-with-docked-nav [class*="pf-v6-c-compass"] {
position: relative;
}

#ws-react-a-compass-with-docked-nav [class*="pf-v6-c-compass"]:not([class*="footer"])::after {
content: "";
position: absolute;
inset: 0;
border: var(--pf-t--global--border--width--regular) dashed var(--pf-t--global--border--color--default);
pointer-events: none;
}
4 changes: 4 additions & 0 deletions packages/react-core/src/components/Masthead/Masthead.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ export interface MastheadProps extends React.DetailedHTMLProps<React.HTMLProps<H
xl?: 'insetNone' | 'insetXs' | 'insetSm' | 'insetMd' | 'insetLg' | 'insetXl' | 'inset2xl' | 'inset3xl';
'2xl'?: 'insetNone' | 'insetXs' | 'insetSm' | 'insetMd' | 'insetLg' | 'insetXl' | 'inset2xl' | 'inset3xl';
};
/** @beta Indicates the variant of the masthead */
variant?: 'default' | 'docked';
}

export const Masthead: React.FunctionComponent<MastheadProps> = ({
Expand All @@ -36,13 +38,15 @@ export const Masthead: React.FunctionComponent<MastheadProps> = ({
md: 'inline'
},
inset,
variant = 'default',
...props
}: MastheadProps) => {
const { width, getBreakpoint } = useContext(PageContext);
return (
<header
className={css(
styles.masthead,
variant === 'docked' && styles.modifiers.docked,
formatBreakpointMods(display, styles, 'display-', getBreakpoint(width)),
formatBreakpointMods(inset, styles, '', getBreakpoint(width)),
className
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { render } from '@testing-library/react';
import { render, screen } from '@testing-library/react';
import { Masthead, MastheadMain, MastheadLogo, MastheadContent, MastheadBrand, MastheadToggle } from '../index';
import styles from '@patternfly/react-styles/css/components/Masthead/masthead';

describe('Masthead', () => {
test('verify basic', () => {
Expand Down Expand Up @@ -71,6 +72,29 @@ describe('Masthead', () => {
expect(asFragment()).toMatchSnapshot();
});
});

test(`Renders with ${styles.modifiers.docked} class when variant is docked`, () => {
render(
<Masthead variant="docked" data-testid="masthead">
test
</Masthead>
);
expect(screen.getByTestId('masthead')).toHaveClass(styles.modifiers.docked);
});

test(`Does not render with ${styles.modifiers.docked} class when variant is default`, () => {
render(
<Masthead variant="default" data-testid="masthead">
test
</Masthead>
);
expect(screen.getByTestId('masthead')).not.toHaveClass(styles.modifiers.docked);
});

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

describe('MastheadLogo', () => {
Expand Down
5 changes: 3 additions & 2 deletions packages/react-core/src/components/Nav/Nav.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,8 @@ export interface NavProps
) => void;
/** Accessible label for the nav when there are multiple navs on the page */
'aria-label'?: string;
/** For horizontal navs */
variant?: 'default' | 'horizontal' | 'horizontal-subnav';
/** The nav variant to use. Docked is in beta. */
variant?: 'default' | 'horizontal' | 'horizontal-subnav' | 'docked';
/** 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 @@ -154,6 +154,7 @@ class Nav extends Component<
className={css(
styles.nav,
isHorizontal && styles.modifiers.horizontal,
variant === 'docked' && styles.modifiers.docked,
variant === 'horizontal-subnav' && styles.modifiers.subnav,
this.state.isScrollable && styles.modifiers.scrollable,
className
Expand Down
30 changes: 27 additions & 3 deletions packages/react-core/src/components/Nav/NavItem.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,14 @@
import { cloneElement, Fragment, isValidElement, useContext, useEffect, useRef, useState } from 'react';
import {
cloneElement,
Fragment,
isValidElement,
useContext,
useEffect,
useRef,
useState,
forwardRef,
MutableRefObject
} from 'react';
import styles from '@patternfly/react-styles/css/components/Nav/nav';
import menuStyles from '@patternfly/react-styles/css/components/Menu/menu';
import dividerStyles from '@patternfly/react-styles/css/components/Divider/divider';
Expand Down Expand Up @@ -42,9 +52,13 @@ export interface NavItemProps extends Omit<React.HTMLProps<HTMLAnchorElement>, '
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. */
ouiaSafe?: boolean;
/** React ref for the anchor element within the nav item. */
anchorRef?: React.Ref<HTMLAnchorElement>;
/** @hide Forwarded ref */
innerRef?: React.Ref<HTMLLIElement>;
}

export const NavItem: React.FunctionComponent<NavItemProps> = ({
const NavItemBase: React.FunctionComponent<NavItemProps> = ({
children,
styleChildren = true,
className,
Expand All @@ -61,13 +75,16 @@ export const NavItem: React.FunctionComponent<NavItemProps> = ({
ouiaSafe,
zIndex = 9999,
icon,
innerRef,
anchorRef,
...props
}: NavItemProps) => {
const { flyoutRef, setFlyoutRef, navRef } = useContext(NavContext);
const { isSidebarOpen } = useContext(PageSidebarContext);
const [flyoutTarget, setFlyoutTarget] = useState(null);
const [isHovered, setIsHovered] = useState(false);
const ref = useRef<HTMLLIElement>(undefined);
const _ref = useRef<HTMLLIElement>(undefined);
const ref = (innerRef as MutableRefObject<HTMLLIElement>) || _ref;
const flyoutVisible = ref === flyoutRef;
Comment on lines +86 to 88
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Critical ref handling issue remains unaddressed—callback refs will silently fail.

The previous critical review flagged unsafe ref handling, but the core runtime logic was not fixed:

  1. Line 86: useRef<HTMLLIElement>(undefined) should initialize with null (React convention).

  2. Line 87: Unsafe cast of innerRef as MutableRefObject<HTMLLIElement> assumes innerRef has a .current property. However, React.Ref<T> can be a callback function (node: T | null) => void, which has no .current. When a callback ref is passed from a parent (valid usage), line 134's ref?.current?.contains(target) will evaluate to undefined, silently breaking flyout keyboard navigation and hover detection.

  3. Line 88: Comparing ref objects by identity (ref === flyoutRef) instead of comparing DOM nodes (ref?.current === flyoutRef?.current) can fail if ref objects change between renders.

Apply this fix to safely handle both callback and object refs:

-  const _ref = useRef<HTMLLIElement>(undefined);
-  const ref = (innerRef as MutableRefObject<HTMLLIElement>) || _ref;
-  const flyoutVisible = ref === flyoutRef;
+  const _ref = useRef<HTMLLIElement>(null);
+  const ref = _ref;
+  
+  // Safely forward to both innerRef and local ref
+  const setRef = useCallback((node: HTMLLIElement | null) => {
+    _ref.current = node;
+    if (typeof innerRef === 'function') {
+      innerRef(node);
+    } else if (innerRef) {
+      (innerRef as MutableRefObject<HTMLLIElement>).current = node;
+    }
+  }, [innerRef]);
+  
+  const flyoutVisible = flyoutRef?.current === ref?.current;

Then update line 272:

-        ref={ref}
+        ref={setRef}

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents
In packages/react-core/src/components/Nav/NavItem.tsx around lines 86-88 (and
also update usage at line 272), change the ref handling to follow React
conventions: initialize the internal ref with null
(useRef<HTMLLIElement>(null)), do NOT cast innerRef to MutableRefObject; instead
create a small utility that normalizes a React.Ref to a DOM node (supporting
both object refs with .current and callback refs) and use that to derive the
current node; replace the identity comparison ref === flyoutRef with a node
comparison (normalize(innerRef) === normalize(flyoutRef) or
normalize(ref)?.contains(...) where appropriate) so all checks compare DOM nodes
and the contains call uses the actual element, ensuring callback refs and object
refs both work correctly.

const popperRef = useRef<HTMLDivElement>(undefined);
const hasFlyout = flyout !== undefined;
Expand Down Expand Up @@ -180,6 +197,7 @@ export const NavItem: React.FunctionComponent<NavItemProps> = ({
const preventLinkDefault = preventDefault || !to;
return (
<Component
ref={anchorRef}
href={to}
onClick={(e: any) => context.onSelect(e, groupId, itemId, to, preventLinkDefault, onClick)}
className={css(
Expand Down Expand Up @@ -208,6 +226,7 @@ export const NavItem: React.FunctionComponent<NavItemProps> = ({
className: css(styles.navLink, isActive && styles.modifiers.current, child.props && child.props.className)
}),
tabIndex: child.props.tabIndex || tabIndex,
ref: anchorRef,
children: hasFlyout ? (
<Fragment>
{child.props.children}
Expand Down Expand Up @@ -267,4 +286,9 @@ export const NavItem: React.FunctionComponent<NavItemProps> = ({

return navItem;
};

export const NavItem = forwardRef<HTMLLIElement, NavItemProps>((props, ref) => (
<NavItemBase {...props} innerRef={ref} />
));

NavItem.displayName = 'NavItem';
15 changes: 15 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 @@ -259,4 +259,19 @@ describe('Nav', () => {
);
expect(screen.getAllByText('this is an icon')[0].parentElement).toHaveClass(styles.navLinkIcon);
});

test(`Renders with ${styles.modifiers.docked} class when variant is docked`, () => {
renderNav(
<Nav variant="docked" data-testid="docked-nav">
<NavList>
{props.items.map((item) => (
<NavItem to={item.to} key={item.to}>
{item.label}
</NavItem>
))}
</NavList>
</Nav>
);
expect(screen.getByTestId('docked-nav')).toHaveClass(styles.modifiers.docked);
});
});
6 changes: 5 additions & 1 deletion packages/react-core/src/components/Page/Page.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ export interface PageProps extends React.HTMLProps<HTMLDivElement> {
children?: React.ReactNode;
/** Additional classes added to the page layout */
className?: string;
/** @beta Indicates the layout variant */
variant?: 'default' | 'docked';
/** Masthead component (e.g. <Masthead />) */
masthead?: React.ReactNode;
/** Sidebar component for a side nav, recommended to be a PageSidebar. If set to null, the page grid layout
Expand Down Expand Up @@ -229,6 +231,7 @@ class Page extends Component<PageProps, PageState> {
isBreadcrumbWidthLimited,
className,
children,
variant,
masthead,
sidebar,
notificationDrawer,
Expand Down Expand Up @@ -336,6 +339,7 @@ class Page extends Component<PageProps, PageState> {
{...rest}
className={css(
styles.page,
variant === 'docked' && styles.modifiers.dock,
width !== null && height !== null && 'pf-m-resize-observer',
width !== null && `pf-m-breakpoint-${getBreakpoint(width)}`,
height !== null && `pf-m-height-breakpoint-${getVerticalBreakpoint(height)}`,
Expand All @@ -344,7 +348,7 @@ class Page extends Component<PageProps, PageState> {
)}
>
{skipToContent}
{masthead}
{variant === 'docked' ? <div className={css(styles.pageDock)}>{masthead}</div> : masthead}
{sidebar}
{notificationDrawer && (
<div className={css(styles.pageDrawer)}>
Expand Down
Loading
Loading