feat(Toolbar,OverflowMenu): support responsive height vis breakpoints#12347
Open
kmcfaul wants to merge 4 commits intopatternfly:mainfrom
Open
feat(Toolbar,OverflowMenu): support responsive height vis breakpoints#12347kmcfaul wants to merge 4 commits intopatternfly:mainfrom
kmcfaul wants to merge 4 commits intopatternfly:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis pull request introduces vertical orientation support to OverflowMenu with height-based breakpoint responsiveness and adds height-responsive visibility properties ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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
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
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.
be35ea3 to
dbb03c6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
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/Toolbar/ToolbarItem.tsx (1)
181-223:⚠️ Potential issue | 🔴 CriticalHeight visibility modifiers are missing from the CSS dependency—
visibilityAtHeightwill silently fail to apply modifiers.The code calls
formatBreakpointMods(visibilityAtHeight, styles, '', getVerticalBreakpoint(height), true)expecting CSS classes likehiddenOnMdHeightandvisibleOnLgHeight, but these do not exist in the current@patternfly/react-stylesor@patternfly/patternflypackages. TheformatBreakpointModsfunction filters out missing modifier keys with.filter(Boolean), so components usingvisibilityAtHeightwill render without responsive height-based visibility—no error is raised, just silent failure.Although upstream PR patternfly/patternfly#8295 is merged, the height visibility classes have not yet landed in a published release. Before merge, either:
- Confirm a new
@patternfly/patternflyrelease with height modifiers is published and pinned, or- Run
npm run generateinpackages/react-stylesto verify the workspace CSS contains the modifiers🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-core/src/components/Toolbar/ToolbarItem.tsx` around lines 181 - 223, ToolbarItem uses formatBreakpointMods(visibilityAtHeight, styles, '', getVerticalBreakpoint(height), true) but the corresponding height-based modifier classes are missing from styles, so visibilityAtHeight silently fails; fix by ensuring the compiled CSS contains those modifiers—either update/pin the `@patternfly/patternfly/`@patternfly/react-styles dependency to a release that includes the height visibility classes or regenerate the local style bundle (run the workspace style generation, e.g., npm run generate in the react-styles package) so styles includes keys like hiddenOnMdHeight/visibleOnLgHeight referenced by formatBreakpointMods; verify by checking styles object entries used by ToolbarItem and formatBreakpointMods/getVerticalBreakpoint resolve to non-empty class names.
🧹 Nitpick comments (1)
packages/react-core/src/components/OverflowMenu/OverflowMenu.tsx (1)
103-115: Consider keying the "sm" warning on the breakpoint name rather than=== 0.The early-return warning assumes any
0pixel value corresponds to thesmbreakpoint. This coupling is implicit: if another breakpoint token ever resolves to0(orsmchanges), the warning text and the effective behavior drift. Gating the branch onbreakpoint === 'sm'makes the intent self-documenting and robust to token changes:♻️ Proposed refactor
handleResizeHeight = () => { - const breakpointHeight = globalHeightBreakpoints[this.props.breakpoint]; - if (breakpointHeight === 0) { + const { breakpoint } = this.props; + if (breakpoint === 'sm') { // eslint-disable-next-line no-console console.warn('The "sm" breakpoint does not apply to vertical overflow menus.'); return; } + const breakpointHeight = globalHeightBreakpoints[breakpoint]; if (!breakpointHeight) { // eslint-disable-next-line no-console console.error('OverflowMenu will not be visible without a valid breakpoint.'); return; }🤖 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 103 - 115, The warning in handleResizeHeight currently checks breakpointHeight === 0 to detect the "sm" case, which couples logic to a token value; change the guard to check the breakpoint name instead (this.props.breakpoint === 'sm') so the intent is explicit and robust; locate handleResizeHeight and globalHeightBreakpoints in OverflowMenu.tsx and replace the breakpointHeight === 0 branch with a condition that checks this.props.breakpoint === 'sm', keeping the warning text and early return behavior otherwise preserving the existing subsequent validation for missing/invalid breakpointHeight.
🤖 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/OverflowMenuSimpleVertical.tsx`:
- Around line 36-41: The last OverflowMenuDropdownItem in
OverflowMenuSimpleVertical.tsx has a mismatched itemId (itemId={5}) skipping 4;
update the final OverflowMenuDropdownItem's itemId to 4 so the sequence matches
the preceding items (0..4) and aligns with the sibling example; locate the
OverflowMenuDropdownItem instances and change the unique symbol itemId value for
the last item from 5 to 4.
---
Outside diff comments:
In `@packages/react-core/src/components/Toolbar/ToolbarItem.tsx`:
- Around line 181-223: ToolbarItem uses formatBreakpointMods(visibilityAtHeight,
styles, '', getVerticalBreakpoint(height), true) but the corresponding
height-based modifier classes are missing from styles, so visibilityAtHeight
silently fails; fix by ensuring the compiled CSS contains those modifiers—either
update/pin the `@patternfly/patternfly/`@patternfly/react-styles dependency to a
release that includes the height visibility classes or regenerate the local
style bundle (run the workspace style generation, e.g., npm run generate in the
react-styles package) so styles includes keys like
hiddenOnMdHeight/visibleOnLgHeight referenced by formatBreakpointMods; verify by
checking styles object entries used by ToolbarItem and
formatBreakpointMods/getVerticalBreakpoint resolve to non-empty class names.
---
Nitpick comments:
In `@packages/react-core/src/components/OverflowMenu/OverflowMenu.tsx`:
- Around line 103-115: The warning in handleResizeHeight currently checks
breakpointHeight === 0 to detect the "sm" case, which couples logic to a token
value; change the guard to check the breakpoint name instead
(this.props.breakpoint === 'sm') so the intent is explicit and robust; locate
handleResizeHeight and globalHeightBreakpoints in OverflowMenu.tsx and replace
the breakpointHeight === 0 branch with a condition that checks
this.props.breakpoint === 'sm', keeping the warning text and early return
behavior otherwise preserving the existing subsequent validation for
missing/invalid breakpointHeight.
🪄 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: b45444ad-4b6c-44b1-ae4f-82a3522c321e
📒 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
✅ Files skipped from review due to trivial changes (3)
- packages/react-core/src/components/OverflowMenu/examples/OverflowMenu.md
- packages/react-core/src/components/Toolbar/examples/ToolbarVertical.tsx
- packages/react-core/src/components/Toolbar/examples/Toolbar.md
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/react-core/src/components/Toolbar/ToolbarContent.tsx
- packages/react-core/src/components/Toolbar/ToolbarGroup.tsx
Comment on lines
+36
to
+41
| <OverflowMenuDropdownItem itemId={3} key="item4" isShared> | ||
| Item 4 | ||
| </OverflowMenuDropdownItem>, | ||
| <OverflowMenuDropdownItem itemId={5} key="item5" isShared> | ||
| Item 5 | ||
| </OverflowMenuDropdownItem> |
There was a problem hiding this comment.
Inconsistent itemId sequence — itemId={5} skips 4.
The final dropdown item uses itemId={5} while the preceding items are 0..3. The value 4 is skipped. This is cosmetic but inconsistent with the sibling example OverflowMenuBreakpointOnContainerHeight.tsx (which correctly uses 0..4) and could surprise anyone relying on ids.
🩹 Proposed fix
- <OverflowMenuDropdownItem itemId={5} key="item5" isShared>
+ <OverflowMenuDropdownItem itemId={4} key="item5" isShared>
Item 5
</OverflowMenuDropdownItem>📝 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
| <OverflowMenuDropdownItem itemId={3} key="item4" isShared> | |
| Item 4 | |
| </OverflowMenuDropdownItem>, | |
| <OverflowMenuDropdownItem itemId={5} key="item5" isShared> | |
| Item 5 | |
| </OverflowMenuDropdownItem> | |
| <OverflowMenuDropdownItem itemId={3} key="item4" isShared> | |
| Item 4 | |
| </OverflowMenuDropdownItem>, | |
| <OverflowMenuDropdownItem itemId={4} key="item5" isShared> | |
| Item 5 | |
| </OverflowMenuDropdownItem> |
🤖 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/OverflowMenuSimpleVertical.tsx`
around lines 36 - 41, The last OverflowMenuDropdownItem in
OverflowMenuSimpleVertical.tsx has a mismatched itemId (itemId={5}) skipping 4;
update the final OverflowMenuDropdownItem's itemId to 4 so the sequence matches
the preceding items (0..4) and aligns with the sibling example; locate the
OverflowMenuDropdownItem instances and change the unique symbol itemId value for
the last item from 5 to 4.
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