feat(Toolbar,OverflowMenu): support responsive height vis breakpoints#12347
Open
kmcfaul wants to merge 5 commits intopatternfly:mainfrom
Open
feat(Toolbar,OverflowMenu): support responsive height vis breakpoints#12347kmcfaul wants to merge 5 commits intopatternfly:mainfrom
kmcfaul wants to merge 5 commits intopatternfly:mainfrom
Conversation
WalkthroughAdds height-responsive behavior: OverflowMenu gains an Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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
.../react-core/src/components/OverflowMenu/examples/OverflowMenuBreakpointOnContainerHeight.tsx
Outdated
Show resolved
Hide resolved
.../react-core/src/components/OverflowMenu/examples/OverflowMenuBreakpointOnContainerHeight.tsx
Outdated
Show resolved
Hide resolved
packages/react-core/src/components/Toolbar/examples/ToolbarVertical.tsx
Outdated
Show resolved
Hide resolved
kmcfaul
force-pushed
the
responsive-heights
branch
from
08742ba to
fdd97ac
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/OverflowMenu.tsx`:
- Line 8: The import in OverflowMenu.tsx uses an absolute path for PickOptional
which is inconsistent with the project's relative import convention; change the
import of PickOptional to use the same relative path style as other helpers
(e.g., adjust the import statement referencing PickOptional in OverflowMenu.tsx
to the appropriate relative path such as ../../helpers or
../../helpers/typeUtils to match nearby files).
🪄 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: 545c30bc-6141-4039-bbd5-f785f11951ee
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (14)
packages/react-core/package.jsonpackages/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.tsxpackages/react-docs/package.jsonpackages/react-icons/package.jsonpackages/react-styles/package.jsonpackages/react-tokens/package.json
✅ Files skipped from review due to trivial changes (7)
- packages/react-docs/package.json
- packages/react-tokens/package.json
- packages/react-core/package.json
- packages/react-icons/package.json
- packages/react-styles/package.json
- packages/react-core/src/components/OverflowMenu/examples/OverflowMenu.md
- packages/react-core/src/components/Toolbar/examples/Toolbar.md
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/react-core/src/components/Toolbar/examples/ToolbarVertical.tsx
- packages/react-core/src/components/Toolbar/ToolbarGroup.tsx
- packages/react-core/src/components/Toolbar/ToolbarContent.tsx
- packages/react-core/src/components/OverflowMenu/examples/OverflowMenuSimpleVertical.tsx
- packages/react-core/src/components/Toolbar/ToolbarItem.tsx
packages/react-core/src/components/OverflowMenu/OverflowMenu.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/react-core/src/components/OverflowMenu/OverflowMenu.tsx (1)
79-116: Clean refactor with symmetric width/height handling.The dispatch logic and
handleResizeHeightimplementation are well-structured. One edge case to consider: ifisVerticalchanges dynamically after mount,isBelowBreakpointwon't recalculate until the next resize event occurs sincecomponentDidUpdateonly checks forbreakpointRefchanges.If dynamic
isVerticalswitching is a supported use case, consider adding a check incomponentDidUpdate:♻️ Optional enhancement
componentDidUpdate(prevProps: Readonly<OverflowMenuProps>, prevState: Readonly<OverflowMenuState>): void { const reference = this.props.breakpointReference ? this.getBreakpointRef() : undefined; - if (prevState.breakpointRef !== reference) { + if (prevState.breakpointRef !== reference || prevProps.isVertical !== this.props.isVertical) { // 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(); } }🤖 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 79 - 116, If isVertical can change at runtime, componentDidUpdate should also detect a change in this.props.isVertical (compare prevProps.isVertical !== this.props.isVertical) and trigger a recalculation of isBelowBreakpoint by calling handleResize (or the appropriate handler: handleResizeWidth / handleResizeHeight) so the state updates immediately when orientation switches; update componentDidUpdate to include this check and call the same resize logic used on window resize to keep isBelowBreakpoint in sync.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/react-core/src/components/OverflowMenu/OverflowMenu.tsx`:
- Around line 79-116: If isVertical can change at runtime, componentDidUpdate
should also detect a change in this.props.isVertical (compare
prevProps.isVertical !== this.props.isVertical) and trigger a recalculation of
isBelowBreakpoint by calling handleResize (or the appropriate handler:
handleResizeWidth / handleResizeHeight) so the state updates immediately when
orientation switches; update componentDidUpdate to include this check and call
the same resize logic used on window resize to keep isBelowBreakpoint in sync.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9ae93b45-9e22-4e8d-ad26-fc74e2687dce
📒 Files selected for processing (3)
packages/react-core/src/components/OverflowMenu/OverflowMenu.tsxpackages/react-core/src/components/OverflowMenu/examples/OverflowMenuBreakpointOnContainerHeight.tsxpackages/react-core/src/components/Toolbar/examples/ToolbarVertical.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/react-core/src/components/Toolbar/examples/ToolbarVertical.tsx
- packages/react-core/src/components/OverflowMenu/examples/OverflowMenuBreakpointOnContainerHeight.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/Toolbar/examples/ToolbarVertical.tsx`:
- Around line 16-18: The example's label for ToolbarItem is inconsistent with
its visibilityAtHeight config: the prop visibilityAtHeight={{ default: 'hidden',
lg: 'visible' }} makes the item visible at the lg breakpoint, but the text says
"visible above md breakpoint." Update either the prop on the ToolbarItem or the
label text so they match; e.g., change the label to "Item 6 (visible at lg
breakpoint)" or change the visibilityAtHeight value to reflect "above md" (e.g.,
make it visible at md) — locate the ToolbarItem instance with the
visibilityAtHeight prop to apply the fix.
🪄 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: 5aabe15c-53ba-4d84-aa22-9028e87159c8
📒 Files selected for processing (4)
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/Toolbar/examples/ToolbarVertical.tsx
✅ Files skipped from review due to trivial changes (1)
- packages/react-core/src/components/OverflowMenu/examples/OverflowMenu.md
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/react-core/src/components/OverflowMenu/examples/OverflowMenuBreakpointOnContainerHeight.tsx
Comment on lines
+16
to
+18
| <ToolbarItem visibilityAtHeight={{ default: 'hidden', lg: 'visible' }}> | ||
| Item 6 (visible above md breakpoint) | ||
| </ToolbarItem> |
There was a problem hiding this comment.
Item 6 description conflicts with its breakpoint config.
Line 16 makes Item 6 visible at lg, but Line 17 says “visible above md breakpoint.” Please align the label (or the prop) so the example isn’t misleading.
Suggested fix (label aligned to current behavior)
- <ToolbarItem visibilityAtHeight={{ default: 'hidden', lg: 'visible' }}>
- Item 6 (visible above md breakpoint)
+ <ToolbarItem visibilityAtHeight={{ default: 'hidden', lg: 'visible' }}>
+ Item 6 (visible above lg breakpoint)
</ToolbarItem>📝 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
| <ToolbarItem visibilityAtHeight={{ default: 'hidden', lg: 'visible' }}> | |
| Item 6 (visible above md breakpoint) | |
| </ToolbarItem> | |
| <ToolbarItem visibilityAtHeight={{ default: 'hidden', lg: 'visible' }}> | |
| Item 6 (visible above lg breakpoint) | |
| </ToolbarItem> |
🤖 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 16 - 18, The example's label for ToolbarItem is inconsistent with
its visibilityAtHeight config: the prop visibilityAtHeight={{ default: 'hidden',
lg: 'visible' }} makes the item visible at the lg breakpoint, but the text says
"visible above md breakpoint." Update either the prop on the ToolbarItem or the
label text so they match; e.g., change the label to "Item 6 (visible at lg
breakpoint)" or change the visibilityAtHeight value to reflect "above md" (e.g.,
make it visible at md) — locate the ToolbarItem instance with the
visibilityAtHeight prop to apply the fix.
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 OverflowMenuSome notes:
I changed the OverflowMenu container example to full page to allow enough height for the breakpoint to change.
I also added a warning for vertical OverflowMenus that the
smbreakpoint won't do anything (it evaluates to 0 from the token and cannot be calc'd - in practicesmmeans that something is always visible/above the breakpoint for OverflowMenu).Summary by CodeRabbit
New Features
Documentation
Chores