feat(Toolbar,OverflowMenu): support responsive height vis breakpoints#12347
Draft
kmcfaul wants to merge 1 commit intopatternfly:mainfrom
Draft
feat(Toolbar,OverflowMenu): support responsive height vis breakpoints#12347kmcfaul wants to merge 1 commit intopatternfly:mainfrom
kmcfaul wants to merge 1 commit intopatternfly:mainfrom
Conversation
WalkthroughThis PR adds height-responsive breakpoint support to OverflowMenu and Toolbar components. OverflowMenu gains an Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Collaborator
|
Preview: https://pf-react-pr-12347.surge.sh A11y report: https://pf-react-pr-12347-a11y.surge.sh |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/react-core/src/components/OverflowMenu/OverflowMenu.tsx (1)
63-72:⚠️ Potential issue | 🟠 MajorRecompute the breakpoint when
isVerticalorbreakpointprops change.
handleResize()branches onisVertical(lines 79–86) to use either height or width calculations, butcomponentDidUpdate()only reruns the calculation whenbreakpointRefchanges. FlippingisVerticalafter mount or changing thebreakpointprop leavesisBelowBreakpointstale until a resize event fires.Suggested fix
componentDidUpdate(prevProps: Readonly<OverflowMenuProps>, prevState: Readonly<OverflowMenuState>): void { const reference = this.props.breakpointReference ? this.getBreakpointRef() : undefined; if (prevState.breakpointRef !== reference) { // To remove any previous observer/event listener from componentDidMount before adding a new one this.observer(); this.setState({ breakpointRef: reference }); this.observer = getResizeObserver(reference, this.handleResizeWithDelay); this.handleResize(); + } else if ( + prevProps.isVertical !== this.props.isVertical || + prevProps.breakpoint !== this.props.breakpoint + ) { + this.handleResize(); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-core/src/components/OverflowMenu/OverflowMenu.tsx` around lines 63 - 72, componentDidUpdate currently only reacts to breakpointRef changes and so flipping isVertical or changing breakpoint leaves isBelowBreakpoint stale; update componentDidUpdate (in OverflowMenu) to also detect prop changes (prevProps.isVertical !== this.props.isVertical || prevProps.breakpoint !== this.props.breakpoint) and when they change recompute the reference if needed (use this.getBreakpointRef() like you do when computing reference) and call this.handleResize() (and reattach observer if the breakpointReference-related ref changed using this.observer = getResizeObserver(...)). Ensure you still call this.observer() to cleanup and setState/update breakpointRef when the breakpointReference-derived ref changes, but always invoke handleResize() when isVertical or breakpoint props change so isBelowBreakpoint is updated immediately.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/react-core/src/components/OverflowMenu/examples/OverflowMenuBreakpointOnContainerHeight.tsx`:
- Around line 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.
- 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.
In `@packages/react-core/src/components/Toolbar/examples/ToolbarVertical.tsx`:
- Around line 20-29: The example uses width-based props but should use height
breakpoints: replace the width-based visibility props with height-aware ones by
changing ToolbarGroup's visibility to visibilityAtHeight and each ToolbarItem's
visibility to visibilityAtHeight (i.e., keep the same breakpoint keys/values but
use visibilityAtHeight on ToolbarGroup and the two ToolbarItem instances with
lg/md/default as shown); update any prop names in ToolbarGroup and ToolbarItem
to visibilityAtHeight so the example demonstrates height breakpoints correctly.
---
Outside diff comments:
In `@packages/react-core/src/components/OverflowMenu/OverflowMenu.tsx`:
- Around line 63-72: componentDidUpdate currently only reacts to breakpointRef
changes and so flipping isVertical or changing breakpoint leaves
isBelowBreakpoint stale; update componentDidUpdate (in OverflowMenu) to also
detect prop changes (prevProps.isVertical !== this.props.isVertical ||
prevProps.breakpoint !== this.props.breakpoint) and when they change recompute
the reference if needed (use this.getBreakpointRef() like you do when computing
reference) and call this.handleResize() (and reattach observer if the
breakpointReference-related ref changed using this.observer =
getResizeObserver(...)). Ensure you still call this.observer() to cleanup and
setState/update breakpointRef when the breakpointReference-derived ref changes,
but always invoke handleResize() when isVertical or breakpoint props change so
isBelowBreakpoint is updated immediately.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4109d74c-080f-4724-9a72-c5cccf04a3cd
📒 Files selected for processing (9)
packages/react-core/src/components/OverflowMenu/OverflowMenu.tsxpackages/react-core/src/components/OverflowMenu/examples/OverflowMenu.mdpackages/react-core/src/components/OverflowMenu/examples/OverflowMenuBreakpointOnContainerHeight.tsxpackages/react-core/src/components/OverflowMenu/examples/OverflowMenuSimpleVertical.tsxpackages/react-core/src/components/Toolbar/ToolbarContent.tsxpackages/react-core/src/components/Toolbar/ToolbarGroup.tsxpackages/react-core/src/components/Toolbar/ToolbarItem.tsxpackages/react-core/src/components/Toolbar/examples/Toolbar.mdpackages/react-core/src/components/Toolbar/examples/ToolbarVertical.tsx
Comment on lines
+63
to
+64
| <span id="overflowMenu-hasBreakpointOnContainer-height-slider-label">Current container width</span>:{' '} | ||
| {containerHeight}% |
There was a problem hiding this comment.
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> | ||
| <div ref={containerRef} id="height-breakpoint-reference-container" style={containerStyles}> | ||
| <OverflowMenu breakpointReference={containerRef} breakpoint="sm"> |
There was a problem hiding this comment.
🧩 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 exampleRepository: 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 fRepository: 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.
Comment on lines
+20
to
+29
| <ToolbarGroup variant="action-group-plain" visibility={{ default: 'hidden', md: 'visible' }}> | ||
| <ToolbarItem> | ||
| <Button variant="plain" aria-label="edit" icon={<EditIcon />} /> | ||
| </ToolbarItem> | ||
| <ToolbarItem visibility={{ default: 'hidden', lg: 'visible' }}> | ||
| <Button variant="plain" aria-label="clone" icon={<CloneIcon />} /> | ||
| </ToolbarItem> | ||
| <ToolbarItem visibility={{ default: 'hidden', lg: 'visible' }}> | ||
| <Button variant="plain" aria-label="sync" icon={<SyncIcon />} /> | ||
| </ToolbarItem> |
There was a problem hiding this comment.
Use visibilityAtHeight here; visibility is width-based.
This example is meant to demonstrate height breakpoints, but the current props are width-responsive only.
Proposed fix
- <ToolbarGroup variant="action-group-plain" visibility={{ default: 'hidden', md: 'visible' }}>
+ <ToolbarGroup variant="action-group-plain" visibilityAtHeight={{ default: 'hidden', md: 'visible' }}>
<ToolbarItem>
<Button variant="plain" aria-label="edit" icon={<EditIcon />} />
</ToolbarItem>
- <ToolbarItem visibility={{ default: 'hidden', lg: 'visible' }}>
+ <ToolbarItem visibilityAtHeight={{ default: 'hidden', lg: 'visible' }}>
<Button variant="plain" aria-label="clone" icon={<CloneIcon />} />
</ToolbarItem>
- <ToolbarItem visibility={{ default: 'hidden', lg: 'visible' }}>
+ <ToolbarItem visibilityAtHeight={{ default: 'hidden', lg: 'visible' }}>
<Button variant="plain" aria-label="sync" icon={<SyncIcon />} />
</ToolbarItem>
</ToolbarGroup>📝 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
| <ToolbarGroup variant="action-group-plain" visibility={{ default: 'hidden', md: 'visible' }}> | |
| <ToolbarItem> | |
| <Button variant="plain" aria-label="edit" icon={<EditIcon />} /> | |
| </ToolbarItem> | |
| <ToolbarItem visibility={{ default: 'hidden', lg: 'visible' }}> | |
| <Button variant="plain" aria-label="clone" icon={<CloneIcon />} /> | |
| </ToolbarItem> | |
| <ToolbarItem visibility={{ default: 'hidden', lg: 'visible' }}> | |
| <Button variant="plain" aria-label="sync" icon={<SyncIcon />} /> | |
| </ToolbarItem> | |
| <ToolbarGroup variant="action-group-plain" visibilityAtHeight={{ default: 'hidden', md: 'visible' }}> | |
| <ToolbarItem> | |
| <Button variant="plain" aria-label="edit" icon={<EditIcon />} /> | |
| </ToolbarItem> | |
| <ToolbarItem visibilityAtHeight={{ default: 'hidden', lg: 'visible' }}> | |
| <Button variant="plain" aria-label="clone" icon={<CloneIcon />} /> | |
| </ToolbarItem> | |
| <ToolbarItem visibilityAtHeight={{ default: 'hidden', lg: 'visible' }}> | |
| <Button variant="plain" aria-label="sync" icon={<SyncIcon />} /> | |
| </ToolbarItem> | |
| </ToolbarGroup> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/react-core/src/components/Toolbar/examples/ToolbarVertical.tsx`
around lines 20 - 29, The example uses width-based props but should use height
breakpoints: replace the width-based visibility props with height-aware ones by
changing ToolbarGroup's visibility to visibilityAtHeight and each ToolbarItem's
visibility to visibilityAtHeight (i.e., keep the same breakpoint keys/values but
use visibilityAtHeight on ToolbarGroup and the two ToolbarItem instances with
lg/md/default as shown); update any prop names in ToolbarGroup and ToolbarItem
to visibilityAtHeight so the example demonstrates height breakpoints correctly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What: Closes #12335
Needs patternfly/patternfly#8295 to be pulled in when merged for styling
visibilityAtHeightprops toToolbarItem,ToolbarGroup,ToolbarContentthat allows visibility to be changed based on specific height breakpointsisVerticaltoOverflowMenuthat changes the layout orientation for vertical toolbars/elements that use OverflowMenuSummary by CodeRabbit
New Features
visibilityAtHeightprop to control element visibility based on viewport/container height breakpointsDocumentation