Skip to content
Draft
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
40 changes: 36 additions & 4 deletions packages/react-core/src/components/OverflowMenu/OverflowMenu.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,18 +3,21 @@
import { css } from '@patternfly/react-styles';
import { OverflowMenuContext } from './OverflowMenuContext';
import { debounce } from '../../helpers/util';
import { globalWidthBreakpoints } from '../../helpers/constants';
import { globalWidthBreakpoints, globalHeightBreakpoints } from '../../helpers/constants';
import { getResizeObserver } from '../../helpers/resizeObserver';
import { PickOptional } from 'src/helpers';

export interface OverflowMenuProps extends React.HTMLProps<HTMLDivElement> {
/** Any elements that can be rendered in the menu */
children?: any;
/** Additional classes added to the OverflowMenu. */
className?: string;
/** Indicates breakpoint at which to switch between horizontal menu and vertical dropdown */
/** Indicates breakpoint at which to switch between expanded and collapsed states */
breakpoint: 'sm' | 'md' | 'lg' | 'xl' | '2xl';
/** A container reference to base the specified breakpoint on instead of the viewport width. */
breakpointReference?: HTMLElement | (() => HTMLElement) | React.RefObject<any>;
/** Indicates the overflow menu orientation is vertical and should respond to height changes instead of width. */
isVertical?: boolean;
}

export interface OverflowMenuState extends React.HTMLProps<HTMLDivElement> {
Expand All @@ -24,6 +27,11 @@

class OverflowMenu extends Component<OverflowMenuProps, OverflowMenuState> {
static displayName = 'OverflowMenu';

static defaultProps: PickOptional<OverflowMenuProps> = {
isVertical: false
};

constructor(props: OverflowMenuProps) {
super(props);
this.state = {
Expand Down Expand Up @@ -69,6 +77,15 @@
}

handleResize = () => {
const { isVertical } = this.props;
if (isVertical) {
this.handleResizeHeight();
} else {
this.handleResizeWidth();
}
};

handleResizeWidth = () => {
const breakpointWidth = globalWidthBreakpoints[this.props.breakpoint];
if (!breakpointWidth) {
// eslint-disable-next-line no-console
Expand All @@ -83,14 +100,29 @@
}
};

handleResizeHeight = () => {
const breakpointHeight = globalHeightBreakpoints[this.props.breakpoint];
if (!breakpointHeight) {
// eslint-disable-next-line no-console
console.error('OverflowMenu will not be visible without a valid breakpoint.');
return;
}

const relativeHeight = this.state.breakpointRef ? this.state.breakpointRef.clientHeight : window.innerHeight;
const isBelowBreakpoint = relativeHeight < breakpointHeight;
if (this.state.isBelowBreakpoint !== isBelowBreakpoint) {
this.setState({ isBelowBreakpoint });
}
};

handleResizeWithDelay = debounce(this.handleResize, 250);

render() {
// eslint-disable-next-line @typescript-eslint/no-unused-vars
const { className, breakpoint, children, breakpointReference, ...props } = this.props;
const { className, breakpoint, children, breakpointReference, isVertical, ...props } = this.props;

return (
<div {...props} className={css(styles.overflowMenu, className)}>
<div {...props} className={css(styles.overflowMenu, isVertical && styles.modifiers.vertical, className)}>

Check failure on line 125 in packages/react-core/src/components/OverflowMenu/OverflowMenu.tsx

View workflow job for this annotation

GitHub Actions / Build, test & deploy

Property 'vertical' does not exist on type '{ buttonGroup: "pf-m-button-group"; iconButtonGroup: "pf-m-icon-button-group"; }'.

Check failure on line 125 in packages/react-core/src/components/OverflowMenu/OverflowMenu.tsx

View workflow job for this annotation

GitHub Actions / Build

Property 'vertical' does not exist on type '{ buttonGroup: "pf-m-button-group"; iconButtonGroup: "pf-m-icon-button-group"; }'.
<OverflowMenuContext.Provider value={{ isBelowBreakpoint: this.state.isBelowBreakpoint }}>
{children}
</OverflowMenuContext.Provider>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,12 @@ import EllipsisVIcon from '@patternfly/react-icons/dist/esm/icons/ellipsis-v-ico

```

### Simple vertical (responsive)

```ts file="./OverflowMenuSimpleVertical.tsx"

```

### Group types

```ts file="./OverflowMenuGroupTypes.tsx"
Expand All @@ -45,7 +51,7 @@ import EllipsisVIcon from '@patternfly/react-icons/dist/esm/icons/ellipsis-v-ico

```

### Breakpoint on container
### Breakpoint on container width

By passing in the `breakpointReference` property, the overflow menu's breakpoint will be relative to the width of the reference container rather than the viewport width.

Expand All @@ -54,3 +60,11 @@ You can change the container width in this example by adjusting the slider. As t
```ts file="./OverflowMenuBreakpointOnContainer.tsx"

```

### Breakpoint on container height

By passing in the `breakpointReference` and `isVertical` properties, the overflow menu's breakpoint will be relative to the height of the reference container rather than the viewport height.

```ts file="./OverflowMenuBreakpointOnContainerHeight.tsx"

```
Original file line number Diff line number Diff line change
@@ -0,0 +1,111 @@
import { useRef, useState } from 'react';
import {
OverflowMenu,
OverflowMenuControl,
OverflowMenuContent,
OverflowMenuGroup,
OverflowMenuItem,
OverflowMenuDropdownItem,
MenuToggle,
Slider,
SliderOnChangeEvent,
Dropdown,
DropdownList
} from '@patternfly/react-core';
import EllipsisVIcon from '@patternfly/react-icons/dist/esm/icons/ellipsis-v-icon';

export const OverflowMenuBreakpointOnContainerHeight: React.FunctionComponent = () => {
const [isOpen, setIsOpen] = useState(false);
const [containerHeight, setContainerHeight] = useState(100);
const containerRef = useRef<HTMLDivElement>(null);

const onToggle = () => {
setIsOpen(!isOpen);
};

const onSelect = () => {
setIsOpen(!isOpen);
};

const onChange = (_event: SliderOnChangeEvent, value: number) => {
setContainerHeight(value);
};

const containerStyles = {
height: `${containerHeight}%`,
padding: '1rem',
borderWidth: '2px',
borderStyle: 'dashed'
};

const dropdownItems = [
<OverflowMenuDropdownItem itemId={0} key="item1" isShared>
Item 1
</OverflowMenuDropdownItem>,
<OverflowMenuDropdownItem itemId={1} key="item2" isShared>
Item 2
</OverflowMenuDropdownItem>,
<OverflowMenuDropdownItem itemId={2} key="item3" isShared>
Item 3
</OverflowMenuDropdownItem>,
<OverflowMenuDropdownItem itemId={3} key="item4" isShared>
Item 4
</OverflowMenuDropdownItem>,
<OverflowMenuDropdownItem itemId={4} key="item5" isShared>
Item 5
</OverflowMenuDropdownItem>
];

return (
<>
<div style={{ height: '100%', maxHeight: '400px' }}>
<div>
<span id="overflowMenu-hasBreakpointOnContainer-height-slider-label">Current container width</span>:{' '}
{containerHeight}%
Comment on lines +63 to +64
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 | 🟡 Minor

The label still says width.

This is the height example, so the readout text is currently misleading.

🩹 Suggested fix
-          <span id="overflowMenu-hasBreakpointOnContainer-height-slider-label">Current container width</span>:{' '}
+          <span id="overflowMenu-hasBreakpointOnContainer-height-slider-label">Current container height</span>:{' '}
📝 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
<span id="overflowMenu-hasBreakpointOnContainer-height-slider-label">Current container width</span>:{' '}
{containerHeight}%
<span id="overflowMenu-hasBreakpointOnContainer-height-slider-label">Current container height</span>:{' '}
{containerHeight}%
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@packages/react-core/src/components/OverflowMenu/examples/OverflowMenuBreakpointOnContainerHeight.tsx`
around lines 63 - 64, The displayed label text is wrong: in
OverflowMenuBreakpointOnContainerHeight.tsx update the span with id
"overflowMenu-hasBreakpointOnContainer-height-slider-label" (and any adjacent
text) from "Current container width" to "Current container height" (and ensure
any related aria/accessible labels or variables referencing containerHeight
reflect "height" consistently); keep the existing id and variable
containerHeight unchanged.

</div>
<Slider
value={containerHeight}
onChange={onChange}
max={100}
min={20}
step={20}
showTicks
showBoundaries={false}
aria-labelledby="overflowMenu-hasBreakpointOnContainer-height-slider-label"
/>
</div>
<div ref={containerRef} id="height-breakpoint-reference-container" style={containerStyles}>
<OverflowMenu breakpointReference={containerRef} breakpoint="sm">
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

🧩 Analysis chain

🏁 Script executed:

# First, locate and examine the OverflowMenu.tsx file to verify the height logic
fd -t f "OverflowMenu.tsx" --path "*components/OverflowMenu*" | grep -v example

Repository: patternfly/patternfly-react

Length of output: 304


🏁 Script executed:

# Read the example file around line 78 to verify the prop usage
cat -n packages/react-core/src/components/OverflowMenu/examples/OverflowMenuBreakpointOnContainerHeight.tsx | sed -n '70,85p'

Repository: patternfly/patternfly-react

Length of output: 906


🏁 Script executed:

# Check the OverflowMenu.tsx implementation around lines 79-116
cat -n packages/react-core/src/components/OverflowMenu/OverflowMenu.tsx | sed -n '75,120p'

Repository: patternfly/patternfly-react

Length of output: 1856


🏁 Script executed:

# Check the documentation file
fd -t f "OverflowMenu.md" --path "*OverflowMenu*"

Repository: patternfly/patternfly-react

Length of output: 304


🏁 Script executed:

# Check line 63 for the label issue
cat -n packages/react-core/src/components/OverflowMenu/examples/OverflowMenuBreakpointOnContainerHeight.tsx | sed -n '60,70p'

Repository: patternfly/patternfly-react

Length of output: 506


🏁 Script executed:

# Find and read the OverflowMenu.md documentation
find . -name "OverflowMenu.md" -type f

Repository: patternfly/patternfly-react

Length of output: 145


🏁 Script executed:

# Search for documentation about the height breakpoint example
rg "breakpointReference.*isVertical|isVertical.*breakpointReference|container height" -A 3 -B 3 --type md packages/react-core/src/components/OverflowMenu/

Repository: patternfly/patternfly-react

Length of output: 1023


Pass isVertical here or this example never exercises the height breakpoint path.

OverflowMenu.tsx only switches to clientHeight/window.innerHeight when isVertical is set. As written, this "container height" example is still width-based, which conflicts with the documentation in OverflowMenu.md that explicitly states both breakpointReference and isVertical are required for height-based breakpoints.

Additionally, the slider label on line 63 says "Current container width" but should say "Current container height" for this example.

🩹 Suggested fix
-        <OverflowMenu breakpointReference={containerRef} breakpoint="sm">
+        <OverflowMenu breakpointReference={containerRef} breakpoint="sm" isVertical>

Also update line 63 label from "Current container width" to "Current container height".

📝 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
<OverflowMenu breakpointReference={containerRef} breakpoint="sm">
<OverflowMenu breakpointReference={containerRef} breakpoint="sm" isVertical>
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@packages/react-core/src/components/OverflowMenu/examples/OverflowMenuBreakpointOnContainerHeight.tsx`
at line 78, The example is using height-based breakpoint props but never toggles
OverflowMenu to vertical mode; update the <OverflowMenu> instance (the one with
props breakpointReference={containerRef} breakpoint="sm") to also pass
isVertical so OverflowMenu uses clientHeight for breakpoints, and change the
slider label text currently reading "Current container width" (around the slider
at line ~63) to "Current container height" so the UI and docs match the
height-based example.

<OverflowMenuContent>
<OverflowMenuItem>Item 1</OverflowMenuItem>
<OverflowMenuItem>Item 2</OverflowMenuItem>
<OverflowMenuGroup>
<OverflowMenuItem>Item 3</OverflowMenuItem>
<OverflowMenuItem>Item 4</OverflowMenuItem>
<OverflowMenuItem>Item 5</OverflowMenuItem>
</OverflowMenuGroup>
</OverflowMenuContent>
<OverflowMenuControl>
<Dropdown
onSelect={onSelect}
toggle={(toggleRef) => (
<MenuToggle
ref={toggleRef}
aria-label="Height breakpoint on container example overflow menu"
variant="plain"
onClick={onToggle}
isExpanded={isOpen}
icon={<EllipsisVIcon />}
/>
)}
isOpen={isOpen}
onOpenChange={(isOpen) => setIsOpen(isOpen)}
>
<DropdownList>{dropdownItems}</DropdownList>
</Dropdown>
</OverflowMenuControl>
</OverflowMenu>
</div>
</>
);
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
import { useState } from 'react';
import {
OverflowMenu,
OverflowMenuControl,
OverflowMenuContent,
OverflowMenuGroup,
OverflowMenuItem,
OverflowMenuDropdownItem,
MenuToggle,
Dropdown,
DropdownList
} from '@patternfly/react-core';
import EllipsisVIcon from '@patternfly/react-icons/dist/esm/icons/ellipsis-v-icon';

export const OverflowMenuSimpleVertical: React.FunctionComponent = () => {
const [isOpen, setIsOpen] = useState(false);

const onToggle = () => {
setIsOpen(!isOpen);
};

const onSelect = () => {
setIsOpen(!isOpen);
};

const dropdownItems = [
<OverflowMenuDropdownItem itemId={0} key="item1" isShared>
Item 1
</OverflowMenuDropdownItem>,
<OverflowMenuDropdownItem itemId={1} key="item2" isShared>
Item 2
</OverflowMenuDropdownItem>,
<OverflowMenuDropdownItem itemId={2} key="item3" isShared>
Item 3
</OverflowMenuDropdownItem>,
<OverflowMenuDropdownItem itemId={3} key="item4" isShared>
Item 4
</OverflowMenuDropdownItem>,
<OverflowMenuDropdownItem itemId={5} key="item5" isShared>
Item 5
</OverflowMenuDropdownItem>
];

return (
<OverflowMenu breakpoint="lg" isVertical>
<OverflowMenuContent>
<OverflowMenuItem>Item</OverflowMenuItem>
<OverflowMenuItem>Item</OverflowMenuItem>
<OverflowMenuGroup>
<OverflowMenuItem>Item</OverflowMenuItem>
<OverflowMenuItem>Item</OverflowMenuItem>
<OverflowMenuItem>Item</OverflowMenuItem>
</OverflowMenuGroup>
</OverflowMenuContent>
<OverflowMenuControl>
<Dropdown
onSelect={onSelect}
toggle={(toggleRef) => (
<MenuToggle
ref={toggleRef}
aria-label="Simple example overflow menu"
variant="plain"
onClick={onToggle}
isExpanded={isOpen}
icon={<EllipsisVIcon />}
/>
)}
isOpen={isOpen}
onOpenChange={(isOpen) => setIsOpen(isOpen)}
>
<DropdownList>{dropdownItems}</DropdownList>
</Dropdown>
</OverflowMenuControl>
</OverflowMenu>
);
};
14 changes: 12 additions & 2 deletions packages/react-core/src/components/Toolbar/ToolbarContent.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,22 @@ import { PageContext } from '../Page/PageContext';
export interface ToolbarContentProps extends React.HTMLProps<HTMLDivElement> {
/** Classes applied to root element of the data toolbar content row */
className?: string;
/** Visibility at various breakpoints. */
/** Visibility at various width breakpoints. */
visibility?: {
default?: 'hidden' | 'visible';
md?: 'hidden' | 'visible';
lg?: 'hidden' | 'visible';
xl?: 'hidden' | 'visible';
'2xl'?: 'hidden' | 'visible';
};
/** Visibility at various height breakpoints. */
visibilityAtHeight?: {
default?: 'hidden' | 'visible';
md?: 'hidden' | 'visible';
lg?: 'hidden' | 'visible';
xl?: 'hidden' | 'visible';
'2xl'?: 'hidden' | 'visible';
};
/** Value to set for content wrapping at various breakpoints */
rowWrap?: {
default?: 'wrap' | 'nowrap';
Expand Down Expand Up @@ -59,6 +67,7 @@ class ToolbarContent extends Component<ToolbarContentProps> {
isExpanded,
toolbarId,
visibility,
visibilityAtHeight,
rowWrap,
alignItems,
clearAllFilters,
Expand All @@ -69,11 +78,12 @@ class ToolbarContent extends Component<ToolbarContentProps> {

return (
<PageContext.Consumer>
{({ width, getBreakpoint }) => (
{({ width, getBreakpoint, height, getVerticalBreakpoint }) => (
<div
className={css(
styles.toolbarContent,
formatBreakpointMods(visibility, styles, '', getBreakpoint(width)),
formatBreakpointMods(visibilityAtHeight, styles, '', getVerticalBreakpoint(height), true),
className
)}
ref={this.expandableContentRef}
Expand Down
Loading
Loading