feat(docked nav): add support for docked nav layout #12175
Conversation
WalkthroughAdds docked-navigation support across multiple UI components (Compass, Nav, Masthead, Page, Toolbar), refactors NavItem to forward refs, updates demos/examples and CSS for docked layouts, adds tests, and bumps Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
Preview: https://pf-react-pr-12175.surge.sh A11y report: https://pf-react-pr-12175-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: 7
♻️ Duplicate comments (4)
packages/react-docs/package.json (1)
26-26: Version update verified.This dependency update aligns with the coordinated PatternFly prerelease version bump across all packages. The verification comment in
packages/react-styles/package.jsoncovers this update as well.packages/react-icons/package.json (1)
36-36: Version update verified.This devDependency update is consistent with the coordinated PatternFly prerelease version bump across all packages.
packages/react-tokens/package.json (1)
33-33: Version update verified.This devDependency update is consistent with the coordinated PatternFly prerelease version bump across all packages.
packages/react-core/package.json (1)
57-57: Version update verified.This devDependency update is consistent with the coordinated PatternFly prerelease version bump across all packages.
🧹 Nitpick comments (8)
packages/react-core/src/demos/Page.md (1)
23-27: Consider retaining explanatory context.The removed paragraph provided helpful context about the
isManagedSidebarprop behavior. While the numbered list below still provides manual control instructions, the introductory explanation clarified why these examples useisManagedSidebar. Consider whether this context loss impacts user understanding.packages/react-core/src/components/Compass/__tests__/Compass.test.tsx (1)
174-182: Nice focused dock coverage; consider adding adock={null}edge-case test if supported. This will enforce the intended semantics of “provided” vs “not provided”.packages/react-core/src/components/Compass/examples/CompassDockLayout.tsx (1)
10-25: Rename export to match the example intent (DockLayout vs “Basic”) and docs references.
Right now the file isCompassDockLayout.tsxbut it exportsCompassBasic, which is easy to mis-wire in docs and imports.-export const CompassBasic: React.FunctionComponent = () => { +export const CompassDockLayout: 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} />; };packages/react-core/src/components/Page/Page.tsx (1)
335-362: Avoid rendering an empty “dock” wrapper whenmastheadis not provided.
Right now{variant === 'docked' ? <div ...>{masthead}</div> : masthead}will still render an empty dock container whenmastheadisundefined/null.- {variant === 'docked' ? <div className={css(styles.pageDock)}>{masthead}</div> : masthead} + {variant === 'docked' && masthead ? <div className={css(styles.pageDock)}>{masthead}</div> : masthead}packages/react-core/src/components/Nav/Nav.tsx (1)
38-40: Update thevariantdoc comment (no longer “for horizontal navs” only).
Now thatvariantincludes docked (and already included default/subnav), the comment should describe it as the general layout variant.packages/react-core/src/demos/examples/Page/PageDockedNav.tsx (2)
55-97: Remove or complete the unusedisOpen+ global window listeners.
As written,isOpenis never set true and the refs aren’t attached, so the event listeners don’t provide value and add noise to the demo.
222-235: TypehandleClickand avoid implicitany.- const handleClick = (event) => { + const handleClick = (event: React.MouseEvent<HTMLAnchorElement>) => { event.preventDefault(); const mainContentElement = document.getElementById(mainContainerId); if (mainContentElement) { mainContentElement.focus(); } };packages/react-core/src/components/Masthead/__tests__/Masthead.test.tsx (1)
1-3: Good targeted coverage for the new variant behavior.
These class-based assertions are a nice complement to snapshots. Note: this test file usestoHaveClasswithout explicitly importing@testing-library/jest-dom(unlike other test files in the codebase)—consider adding the import for consistency:import '@testing-library/jest-dom';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
packages/react-core/src/demos/assets/PF-IconLogo-color.svgis excluded by!**/*.svgyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (21)
packages/react-core/package.json(1 hunks)packages/react-core/src/components/Compass/Compass.tsx(3 hunks)packages/react-core/src/components/Compass/__tests__/Compass.test.tsx(1 hunks)packages/react-core/src/components/Compass/examples/Compass.md(2 hunks)packages/react-core/src/components/Compass/examples/CompassDockDemo.tsx(1 hunks)packages/react-core/src/components/Compass/examples/CompassDockLayout.tsx(1 hunks)packages/react-core/src/components/Compass/examples/compass.css(1 hunks)packages/react-core/src/components/Masthead/Masthead.tsx(2 hunks)packages/react-core/src/components/Masthead/__tests__/Masthead.test.tsx(2 hunks)packages/react-core/src/components/Nav/Nav.tsx(2 hunks)packages/react-core/src/components/Nav/NavItem.tsx(4 hunks)packages/react-core/src/components/Nav/__tests__/Nav.test.tsx(1 hunks)packages/react-core/src/components/Page/Page.tsx(4 hunks)packages/react-core/src/components/Page/__tests__/Page.test.tsx(1 hunks)packages/react-core/src/components/Toolbar/Toolbar.tsx(3 hunks)packages/react-core/src/demos/Page.md(3 hunks)packages/react-core/src/demos/examples/Page/PageDockedNav.tsx(1 hunks)packages/react-docs/package.json(1 hunks)packages/react-icons/package.json(1 hunks)packages/react-styles/package.json(1 hunks)packages/react-tokens/package.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
packages/react-core/src/components/Masthead/__tests__/Masthead.test.tsx (1)
packages/react-core/src/components/Masthead/Masthead.tsx (1)
Masthead(34-59)
packages/react-core/src/components/Compass/__tests__/Compass.test.tsx (1)
packages/react-core/src/components/Compass/Compass.tsx (1)
Compass(41-123)
packages/react-core/src/components/Page/__tests__/Page.test.tsx (1)
packages/react-core/src/components/Page/Page.tsx (2)
render(227-364)Page(367-367)
packages/react-core/src/components/Compass/examples/CompassDockLayout.tsx (5)
packages/react-core/src/components/Compass/CompassContent.tsx (1)
CompassContent(16-40)packages/react-core/src/components/Compass/CompassMainHeader.tsx (1)
CompassMainHeader(27-52)packages/react-core/src/components/Compass/CompassPanel.tsx (1)
CompassPanel(23-49)packages/react-core/src/components/Compass/CompassMainHeaderContent.tsx (1)
CompassMainHeaderContent(15-23)packages/react-core/src/components/Compass/Compass.tsx (1)
Compass(41-123)
packages/react-core/src/components/Nav/NavItem.tsx (3)
packages/react-core/src/demos/examples/JumpLinks/JumpLinksWithDrawer.js (1)
useContext(24-24)packages/react-core/src/components/Nav/Nav.tsx (1)
NavContext(63-63)packages/react-core/src/components/Page/PageSidebar.tsx (1)
PageSidebarContext(28-28)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build, test & deploy
🔇 Additional comments (19)
packages/react-core/src/demos/Page.md (3)
6-6: LGTM!The additional React hooks (
useRef,useEffect) are appropriate for the new docked navigation demo functionality.
16-21: LGTM!The new icon imports (
CubeIcon,FolderIcon,CloudIcon,CodeIcon) andpfIconLogoasset support the docked navigation demo implementation.
58-62: LGTM!The new "Docked nav" section aligns with the PR objectives to add support for docked navigation layout. The placement after the context selector section is logical.
packages/react-core/src/components/Toolbar/Toolbar.tsx (2)
43-44: LGTM!The new
isVerticalprop is appropriately marked as@betaand follows the existing pattern for orientation/layout modifier props in the Toolbar component.
148-148: LGTM!The implementation correctly destructures the
isVerticalprop and conditionally applies thestyles.modifiers.verticalclass when truthy, following the established pattern for other boolean modifiers in this component.Also applies to: 173-173
packages/react-core/src/components/Compass/examples/compass.css (3)
17-23: LGTM!The expansion of the alternate-footer rule with full positioning and border properties provides consistent styling across the compass variants.
25-35: LGTM!The new
ws-react-a-compass-with-docked-navstyling block appropriately supports the docked navigation layout feature by applying positioning and decorative borders, consistent with the existing compass variant patterns.
1-1: The selector prefix change fromws-react-p-tows-react-a-is intentional and complete.No remaining references to the old prefix exist in the codebase. This CSS file contains demo-specific selectors used by the documentation system for Compass examples. The migration is already fully applied across all three example containers.
packages/react-styles/package.json (1)
22-22: Verify the prerelease version before merging.The
@patternfly/patternflydependency is being updated to a prerelease version6.5.0-prerelease.32. While this version exists on npm, prerelease versions are less stable than stable releases. Ensure this version does not introduce breaking changes or known issues that could affect the docked navigation feature before proceeding with the update.packages/react-core/src/components/Compass/Compass.tsx (3)
8-39: Public API:dock?: React.ReactNodelooks fine, but clarify “provided” semantics. The prop addition is straightforward; just ensure docs/examples align on whethernullmeans “not provided”.
76-108: Verify CSS/layout expectations after making regions conditional. Ifcompass.cssuses named grid-areas that assume the header/sidebar/footer nodes always exist, removing them could change layout (even when content is absent).
69-109: Fix dock modifier/render condition mismatch (dock !== undefinedvs{dock && …}). Todaydock={null}(or0/'') applies the dock modifier but may not render the dock region. Prefer a singledock != nullcheck for both.- <div - className={css(styles.compass, dock !== undefined && styles.modifiers.dock, className)} + <div + className={css(styles.compass, dock != null && styles.modifiers.dock, className)} {...props} style={{ ...props.style, ...backgroundImageStyles }} > - {dock && <div className={css(`${styles.compass}__dock`)}>{dock}</div>} + {dock != null && <div className={css(`${styles.compass}__dock`)}>{dock}</div>} {header && (packages/react-core/src/components/Nav/NavItem.tsx (2)
98-101: Double-checksetFlyoutRef(ref)expectation after ref changes. IfNavContextcompares ref object identity (not DOM node identity), switching to a stable internalrefis important; avoid storing a callback ref in context.
285-287: The current prop order is correct and doesn't need to be changed.NavItemBase explicitly destructures
innerReffrom props (line 76), which removes it from the remaining props object before spreading. Therefore,{...props}cannot overrideinnerRef={ref}regardless of the order. The prop precedence concern doesn't apply to this implementation.Likely an incorrect or invalid review comment.
packages/react-core/src/components/Nav/__tests__/Nav.test.tsx (1)
263-276: Good coverage for docked variant class. The test cleanly asserts the modifier application without overcoupling to markup.packages/react-core/src/components/Page/__tests__/Page.test.tsx (1)
393-408: Dock variant tests look solid and match implementation (styles.modifiers.dock).packages/react-core/src/components/Page/Page.tsx (1)
18-25: Variant prop addition looks good.
The union type +@betadoc is consistent with the rest of the PR’s “docked” rollout.packages/react-core/src/components/Masthead/Masthead.tsx (1)
7-32: CSS modifier verification confirmed. Thevariant === 'docked' && styles.modifiers.dockedpattern is correct and the modifier is available—confirmed by test coverage that explicitly validates the docked class is applied when variant is set to 'docked' and omitted otherwise.export const Masthead: React.FunctionComponent<MastheadProps> = ({ children, className, display = { 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 )} {...props} > {children} </header> ); };packages/react-core/src/components/Nav/Nav.tsx (1)
126-169: Docked modifier is properly wired and tested. Thestyles.modifiers.dockedreference in the className is correct and has explicit test coverage inNav.test.tsxconfirming the class is applied whenvariant="docked". The modifier is imported from the PatternFly react-styles package and is used consistently across Nav, Masthead, and Page components.
packages/react-core/src/components/Compass/examples/CompassDockDemo.tsx
Outdated
Show resolved
Hide resolved
| const onNavSelect = (_event: React.FormEvent<HTMLInputElement>, selectedItem: NavOnSelectProps) => { | ||
| typeof selectedItem.itemId === 'number' && setActiveItem(selectedItem.itemId); | ||
| }; |
There was a problem hiding this comment.
Nav selection is incorrect: duplicated itemId values + mismatched isActive.
Items 3/4 currently can’t become active because they use itemId={0} while checking activeItem === 2/3.
- const onNavSelect = (_event: React.FormEvent<HTMLInputElement>, selectedItem: NavOnSelectProps) => {
- typeof selectedItem.itemId === 'number' && setActiveItem(selectedItem.itemId);
- };
+ const onNavSelect = (_event: React.FormEvent<HTMLInputElement>, selectedItem: NavOnSelectProps) => {
+ if (typeof selectedItem.itemId === 'number') {
+ setActiveItem(selectedItem.itemId);
+ }
+ };
...
<NavItem
key="nav-icon-link3"
preventDefault
id="nav-icon-link3"
to="#nav-icon-link3"
- itemId={0}
+ itemId={2}
isActive={activeItem === 2}
icon={<CloudIcon />}
ref={navItem3Ref}
+ aria-label="Link 3"
/>
...
<NavItem
key="nav-icon-link4"
preventDefault
id="nav-icon-link4"
to="#nav-icon-link4"
- itemId={0}
+ itemId={3}
isActive={activeItem === 3}
icon={<CodeIcon />}
ref={navItem4Ref}
+ aria-label="Link 4"
/>(Also consider adding aria-label for Link 1/2 as well, since these are icon-only.)
Also applies to: 121-167
🤖 Prompt for AI Agents
packages/react-core/src/components/Compass/examples/CompassDockDemo.tsx lines
53-55: the Nav items have duplicated/mismatched itemId values causing items 3
and 4 to never become active (they use itemId={0} while active checks compare to
2/3); update the itemId props so each Nav item has a unique numeric id that
matches the activeItem comparisons (e.g., set the third and fourth items to
itemId={2} and itemId={3} or adjust activeItem logic to match intended ids),
ensure the onNavSelect continues to setActiveItem(selectedItem.itemId) for
number ids, and apply the same corrections to the duplicated block at lines
121-167; additionally add appropriate aria-label attributes to the icon-only
Link components for Link 1 and Link 2.
packages/react-core/src/components/Compass/examples/CompassDockLayout.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
packages/react-core/src/components/Compass/examples/CompassDockDemo.tsx (2)
46-92: Dead/ineffective “menu open” logic + hook deps warning — remove it or wire it fully.
isOpenis never settrue,menuRef/ outertoggleRefaren’t attached to DOM nodes, and the effect registers global listeners regardless; lint correctly warns about missing deps (because handlers are recreated every render). If this demo doesn’t actually have a dock flyout/drawer yet, simplest is to deleteisOpen,menuRef,toggleRef, both handlers, and theuseEffect.-import { useRef, useState, useEffect } from 'react'; +import { useRef, useState } from 'react'; @@ const [activeItem, setActiveItem] = useState<number>(0); const [isDropdownOpen, setIsDropdownOpen] = useState(false); - const [isOpen, setIsOpen] = useState<boolean>(false); - const menuRef = useRef<HTMLDivElement>(null); - const toggleRef = useRef<HTMLButtonElement>(null); @@ - const handleMenuKeys = (event: KeyboardEvent) => { - if (!isOpen) { - return; - } - if (menuRef.current?.contains(event.target as Node) || toggleRef.current?.contains(event.target as Node)) { - if (event.key === 'Escape') { - setIsOpen(!isOpen); - toggleRef.current?.focus(); - } - } - }; - - const handleClickOutside = (event: MouseEvent) => { - if (isOpen && !menuRef.current?.contains(event.target as Node)) { - setIsOpen(false); - } - }; - - useEffect(() => { - window.addEventListener('keydown', handleMenuKeys); - window.addEventListener('click', handleClickOutside); - - return () => { - window.removeEventListener('keydown', handleMenuKeys); - window.removeEventListener('click', handleClickOutside); - }; - }, [isOpen, menuRef]);
53-55: Nav selection is broken (duplicateitemId+ mismatchedisActive) and icon-only items need names.
Items 3/4 can’t become active because they setitemId={0}but compareactiveItem === 2/3. Also these are icon-only; addaria-label(orchildrentext) so the links are accessible.@@ const onNavSelect = (_event: React.FormEvent<HTMLInputElement>, selectedItem: NavOnSelectProps) => { - typeof selectedItem.itemId === 'number' && setActiveItem(selectedItem.itemId); + if (typeof selectedItem.itemId === 'number') { + setActiveItem(selectedItem.itemId); + } }; @@ <NavItem key="nav-icon-link1" @@ itemId={0} isActive={activeItem === 0} icon={<CubeIcon />} ref={navItem1Ref} + aria-label="Link 1" /> @@ <NavItem key="nav-icon-link2" @@ itemId={1} isActive={activeItem === 1} icon={<FolderIcon />} ref={navItem2Ref} + aria-label="Link 2" /> @@ <NavItem key="nav-icon-link3" @@ - itemId={0} + itemId={2} isActive={activeItem === 2} icon={<CloudIcon />} ref={navItem3Ref} + aria-label="Link 3" /> @@ <NavItem key="nav-icon-link4" @@ - itemId={0} + itemId={3} isActive={activeItem === 3} icon={<CodeIcon />} ref={navItem4Ref} + aria-label="Link 4" />Also applies to: 141-160
packages/react-core/src/demos/examples/Page/PageDockedNav.tsx (3)
55-98: Remove or wire the unusedisOpenmenu state + global listeners (currently dead code).
Same issue as the Compass dock demo:isOpenis never settrue, refs aren’t attached, and the effect will re-register handlers as they change. If there’s no flyout/drawer yet, delete this block.-import { useEffect, useRef, useState } from 'react'; +import { useRef, useState } from 'react'; @@ - const [isOpen, setIsOpen] = useState<boolean>(false); - const menuRef = useRef<HTMLDivElement>(null); - const toggleRef = useRef<HTMLButtonElement>(null); @@ - const handleMenuKeys = (event: KeyboardEvent) => { - if (!isOpen) { - return; - } - if (menuRef.current?.contains(event.target as Node) || toggleRef.current?.contains(event.target as Node)) { - if (event.key === 'Escape') { - setIsOpen(!isOpen); - toggleRef.current?.focus(); - } - } - }; - - const handleClickOutside = (event: MouseEvent) => { - if (isOpen && !menuRef.current?.contains(event.target as Node)) { - setIsOpen(false); - } - }; - - useEffect(() => { - window.addEventListener('keydown', handleMenuKeys); - window.addEventListener('click', handleClickOutside); - - return () => { - window.removeEventListener('keydown', handleMenuKeys); - window.removeEventListener('click', handleClickOutside); - }; - }, [isOpen, menuRef]);
42-43: Avoid@patternfly/react-core/src/...asset self-imports in demos.
These internal paths are brittle for consumers and can break under different bundlers/resolution. Prefer relative imports within the repo (or copy assets into the demo folder).-import imgAvatar from '@patternfly/react-core/src/components/assets/avatarImg.svg'; -import pfIconLogo from '@patternfly/react-core/src/demos/assets/PF-IconLogo-color.svg'; +import imgAvatar from '../../../components/assets/avatarImg.svg'; +import pfIconLogo from '../../assets/PF-IconLogo-color.svg';
157-175: Fix broken Nav selection + duplicate DOM ids (id/to) for items 3/4.
Items 3/4 reuseid="nav-icon-link1"/to="#nav-icon-link1"anditemId={0}, so you get duplicate DOM ids and the active state can’t work.<NavItem preventDefault - id="nav-icon-link1" - to="#nav-icon-link1" - itemId={0} + id="nav-icon-link3" + to="#nav-icon-link3" + itemId={2} isActive={activeItem === 2} icon={<CloudIcon />} ref={navItem3Ref} + aria-label="Link 3" /> <NavItem preventDefault - id="nav-icon-link1" - to="#nav-icon-link1" - itemId={0} + id="nav-icon-link4" + to="#nav-icon-link4" + itemId={3} isActive={activeItem === 3} icon={<CodeIcon />} ref={navItem4Ref} + aria-label="Link 4" />packages/react-core/src/components/Nav/NavItem.tsx (1)
55-57: Fix ref-forwarding implementation: current code can crash with callback refs and lets props override the forwarded ref.
Right nowinnerRefisReact.Ref<any>(callback refs are valid), but it’s cast toMutableRefObjectand then used asref.current, which will throw if a callback ref is passed. Also<NavItemBase innerRef={ref} {...props} />allows{...props}to overrideinnerRef.@@ export interface NavItemProps extends Omit<React.HTMLProps<HTMLAnchorElement>, 'onClick'>, OUIAProps { @@ /** @hide Forwarded ref */ - innerRef?: React.Ref<any>; + innerRef?: React.Ref<HTMLLIElement>; } @@ - const _ref = useRef<HTMLLIElement>(undefined); - const ref = (innerRef as MutableRefObject<HTMLLIElement>) || _ref; - const flyoutVisible = ref === flyoutRef; + const _ref = useRef<HTMLLIElement | null>(null); + const setRefs = (node: HTMLLIElement | null) => { + // keep internal ref updated + _ref.current = node; + // forward to innerRef (callback or object) + if (typeof innerRef === 'function') innerRef(node); + else if (innerRef && 'current' in innerRef) (innerRef as React.MutableRefObject<HTMLLIElement | null>).current = node; + }; + const ref = innerRef ?? _ref; + const flyoutVisible = (flyoutRef as React.RefObject<HTMLLIElement | null>)?.current === _ref.current; @@ <li @@ - ref={ref} + ref={setRefs} {...ouiaProps} > @@ -export const NavItem = forwardRef((props: NavItemProps, ref: React.Ref<any>) => ( - <NavItemBase innerRef={ref} {...props} /> +export const NavItem = forwardRef<HTMLLIElement, NavItemProps>((props, ref) => ( + <NavItemBase {...props} innerRef={ref} /> ));If
NavContextProps.flyoutRefis not aRefObject<HTMLLIElement | null>, you’ll need to align its type too soflyoutVisibleremains correct (compare nodes, not ref object identity).Also applies to: 83-86, 245-260, 264-269, 285-287
🧹 Nitpick comments (3)
packages/react-core/src/components/Page/__tests__/Page.test.tsx (1)
393-408: LGTM! Comprehensive coverage for the docked variant.The three test cases correctly verify all scenarios for the new
variantprop: when it's set to "docked" (applies the modifier), when it's "default" (doesn't apply), and when it's omitted (doesn't apply). The tests follow the same pattern as existing modifier tests in this file.Optional: Consider consolidating these three similar tests using
test.eachfor slightly better maintainability:+ test.each([ + ['docked', true], + ['default', false], + [undefined, false] + ] as const)(`Renders with ${styles.modifiers.dock} class when variant is %s: %s`, (variant, shouldHaveClass) => { + render(<Page {...props} variant={variant} data-testid="page"></Page>); + + if (shouldHaveClass) { + expect(screen.getByTestId('page')).toHaveClass(styles.modifiers.dock); + } else { + expect(screen.getByTestId('page')).not.toHaveClass(styles.modifiers.dock); + } + });However, the current approach is perfectly fine and consistent with the rest of the test suite.
packages/react-core/src/components/Compass/__tests__/Compass.test.tsx (1)
173-182: Consider also asserting dock content renders (not just the modifier class).
Right now these tests can pass even if the dock slot fails to render but the modifier class is applied. Adding agetByText('Dock content')in the “dock is passed” case would tighten coverage.packages/react-core/src/demos/examples/Page/PageDockedNav.tsx (1)
222-229: TypehandleClickevent parameter (avoid implicitany).- const handleClick = (event) => { + const handleClick = (event: React.MouseEvent<HTMLAnchorElement>) => { event.preventDefault();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
packages/react-core/src/demos/assets/PF-IconLogo-color.svgis excluded by!**/*.svgyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (21)
packages/react-core/package.json(1 hunks)packages/react-core/src/components/Compass/Compass.tsx(3 hunks)packages/react-core/src/components/Compass/__tests__/Compass.test.tsx(1 hunks)packages/react-core/src/components/Compass/examples/Compass.md(2 hunks)packages/react-core/src/components/Compass/examples/CompassDockDemo.tsx(1 hunks)packages/react-core/src/components/Compass/examples/CompassDockLayout.tsx(1 hunks)packages/react-core/src/components/Compass/examples/compass.css(1 hunks)packages/react-core/src/components/Masthead/Masthead.tsx(2 hunks)packages/react-core/src/components/Masthead/__tests__/Masthead.test.tsx(2 hunks)packages/react-core/src/components/Nav/Nav.tsx(2 hunks)packages/react-core/src/components/Nav/NavItem.tsx(4 hunks)packages/react-core/src/components/Nav/__tests__/Nav.test.tsx(1 hunks)packages/react-core/src/components/Page/Page.tsx(4 hunks)packages/react-core/src/components/Page/__tests__/Page.test.tsx(1 hunks)packages/react-core/src/components/Toolbar/Toolbar.tsx(3 hunks)packages/react-core/src/demos/Page.md(3 hunks)packages/react-core/src/demos/examples/Page/PageDockedNav.tsx(1 hunks)packages/react-docs/package.json(1 hunks)packages/react-icons/package.json(1 hunks)packages/react-styles/package.json(1 hunks)packages/react-tokens/package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- packages/react-tokens/package.json
- packages/react-docs/package.json
- packages/react-core/src/demos/Page.md
- packages/react-core/src/components/Toolbar/Toolbar.tsx
- packages/react-styles/package.json
- packages/react-core/src/components/Compass/examples/Compass.md
- packages/react-core/src/components/Nav/Nav.tsx
- packages/react-core/src/components/Page/Page.tsx
🧰 Additional context used
🧬 Code graph analysis (7)
packages/react-core/src/components/Page/__tests__/Page.test.tsx (1)
packages/react-core/src/components/Page/Page.tsx (2)
render(227-364)Page(367-367)
packages/react-core/src/components/Nav/__tests__/Nav.test.tsx (3)
packages/react-core/src/components/Nav/Nav.tsx (1)
Nav(174-174)packages/react-core/src/components/Nav/NavList.tsx (1)
NavList(174-174)packages/react-core/src/components/Nav/NavItem.tsx (1)
NavItem(285-287)
packages/react-core/src/components/Compass/examples/CompassDockLayout.tsx (5)
packages/react-core/src/components/Compass/CompassContent.tsx (1)
CompassContent(16-40)packages/react-core/src/components/Compass/CompassMainHeader.tsx (1)
CompassMainHeader(27-52)packages/react-core/src/components/Compass/CompassPanel.tsx (1)
CompassPanel(23-49)packages/react-core/src/components/Compass/CompassMainHeaderContent.tsx (1)
CompassMainHeaderContent(15-23)packages/react-core/src/components/Compass/Compass.tsx (1)
Compass(41-123)
packages/react-core/src/components/Compass/__tests__/Compass.test.tsx (1)
packages/react-core/src/components/Compass/Compass.tsx (1)
Compass(41-123)
packages/react-core/src/components/Compass/Compass.tsx (1)
packages/react-core/scripts/copyStyles.mjs (1)
css(32-32)
packages/react-core/src/components/Masthead/__tests__/Masthead.test.tsx (1)
packages/react-core/src/components/Masthead/Masthead.tsx (1)
Masthead(34-59)
packages/react-core/src/components/Nav/NavItem.tsx (2)
packages/react-core/src/components/Nav/Nav.tsx (1)
NavContext(63-63)packages/react-core/src/components/Page/PageSidebar.tsx (1)
PageSidebarContext(28-28)
🪛 GitHub Check: Lint
packages/react-core/src/components/Compass/examples/CompassDockDemo.tsx
[warning] 91-91:
React Hook useEffect has missing dependencies: 'handleClickOutside' and 'handleMenuKeys'. Either include them or remove the dependency array
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Unit tests
- GitHub Check: Build demo app
- GitHub Check: Build, test & deploy
🔇 Additional comments (12)
packages/react-icons/package.json (1)
37-37: LGTM! PatternFly version bump aligns with docked navigation feature.The devDependency update from prerelease.27 to prerelease.32 is consistent with similar bumps across other packages in this PR and supports the new docked navigation styling.
packages/react-core/package.json (1)
57-57: LGTM! Consistent PatternFly version update.This devDependency bump matches the version update across the monorepo and pulls in the necessary styling for docked navigation components.
packages/react-core/src/components/Masthead/Masthead.tsx (3)
30-31: LGTM! Well-designed variant prop with appropriate beta annotation.The new
variantprop follows established PatternFly patterns and is properly marked as@betato signal its evolving nature.
41-41: Good default prevents breaking changes.Setting
variant = 'default'ensures backward compatibility for existing consumers.
49-49: LGTM! Conditional modifier class is correctly applied.The docked modifier is applied only when
variant === 'docked', following the standard PatternFly styling pattern.packages/react-core/src/components/Nav/__tests__/Nav.test.tsx (1)
263-276: LGTM! Well-structured test for docked variant.The test correctly verifies that the docked modifier class is applied when
variant="docked". It follows the existing test patterns in the file and uses appropriate assertions.packages/react-core/src/components/Compass/examples/CompassDockLayout.tsx (1)
1-25: LGTM! Clean example demonstrating dock prop usage.This example correctly demonstrates passing ReactNode to the
dockprop. The component structure is clear and appropriate for a basic demonstration.packages/react-core/src/components/Masthead/__tests__/Masthead.test.tsx (1)
76-97: LGTM! Comprehensive test coverage for docked variant.These three tests properly verify:
- Docked modifier is applied when
variant="docked"- Docked modifier is not applied when
variant="default"- Docked modifier is not applied when variant is omitted
The tests follow existing patterns and provide good coverage of the new functionality.
packages/react-core/src/components/Compass/examples/compass.css (1)
25-35: LGTM! Consistent demo styling for docked navigation.This CSS block follows the established pattern in the file, providing visual layout boundaries for the docked navigation demo. The use of PatternFly design tokens and the
:not([class*="footer"])selector are both appropriate.packages/react-core/src/components/Compass/Compass.tsx (3)
11-12: LGTM! Well-documented dock prop.The
dockprop is properly typed asReact.ReactNodewith clear documentation describing its purpose.
71-75: LGTM! Correct conditional rendering and modifier application.The implementation properly:
- Applies the
dockmodifier class when dock content is provided- Conditionally renders the dock container only when
dockis defined- Uses the BEM-style class naming convention
76-108: LGTM! Conditional rendering improves efficiency.The conversion to conditional rendering for all layout sections (header, sidebarStart, main, sidebarEnd, footer) is more efficient and semantically correct. The
inertattribute logic is properly preserved within each conditional block.Note: This is a minor behavioral change—previously these sections may have rendered empty containers. If any consumers relied on empty divs being present in the DOM, they would need to adjust. However, this change aligns with React best practices.
There was a problem hiding this comment.
Actionable comments posted: 4
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/demos/Page.md (1)
11-22: Remove duplicateimgAvatarimport (line 21).Line 11 and line 21 both import
imgAvatarbut from different paths, creating a naming collision that would cause a compilation error if copied into an actual component.Apply this diff to remove the duplicate:
import imgAvatar from '@patternfly/react-core/src/components/assets/avatarImg.svg'; import BarsIcon from '@patternfly/react-icons/dist/esm/icons/bars-icon'; import AttentionBellIcon from '@patternfly/react-icons/dist/esm/icons/attention-bell-icon'; import LightbulbIcon from '@patternfly/react-icons/dist/esm/icons/lightbulb-icon'; import EllipsisVIcon from '@patternfly/react-icons/dist/esm/icons/ellipsis-v-icon'; import CubeIcon from '@patternfly/react-icons/dist/esm/icons/cube-icon'; import FolderIcon from '@patternfly/react-icons/dist/esm/icons/folder-icon'; import CloudIcon from '@patternfly/react-icons/dist/esm/icons/cloud-icon'; import CodeIcon from '@patternfly/react-icons/dist/esm/icons/code-icon'; import pfLogo from '@patternfly/react-core/src/demos/assets/PF-HorizontalLogo-Color.svg'; -import imgAvatar from '../../../components/assets/avatarImg.svg'; import pfIconLogo from '../../assets/PF-IconLogo-color.svg';
🧹 Nitpick comments (2)
packages/react-core/src/components/Compass/examples/CompassDockDemo.tsx (2)
54-60: Make dropdown open/close handlers deterministic (avoid “toggle” everywhere)
Right now you toggle state inonDropdownToggleandonDropdownSelectwhile also controlling state viaonOpenChange(Line 159). Prefer a functional toggle for clicks and an explicit close on select.- const onDropdownToggle = () => { - setIsDropdownOpen(!isDropdownOpen); - }; + const onDropdownToggle = () => setIsDropdownOpen((prev) => !prev); @@ - const onDropdownSelect = () => { - setIsDropdownOpen(!isDropdownOpen); - }; + const onDropdownSelect = () => setIsDropdownOpen(false);Also applies to: 156-160
62-69: SimplifyuserDropdownItems(avoid array-of-fragment)
userDropdownItemsis an array with a single fragment, then rendered intoDropdownList(Line 62–68, Line 170). This is noisier than needed and can complicate keying later.- const userDropdownItems = [ - <> - <DropdownItem key="group 2 profile">My profile</DropdownItem> - <DropdownItem key="group 2 user">User management</DropdownItem> - <DropdownItem key="group 2 logout">Logout</DropdownItem> - </> - ]; + const userDropdownItems = ( + <> + <DropdownItem key="profile">My profile</DropdownItem> + <DropdownItem key="user-management">User management</DropdownItem> + <DropdownItem key="logout">Logout</DropdownItem> + </> + ); @@ - <DropdownList>{userDropdownItems}</DropdownList> + <DropdownList>{userDropdownItems}</DropdownList>Also applies to: 170-170
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/react-core/src/components/Compass/examples/Compass.md(2 hunks)packages/react-core/src/components/Compass/examples/CompassDockDemo.tsx(1 hunks)packages/react-core/src/components/Nav/NavItem.tsx(7 hunks)packages/react-core/src/demos/Page.md(3 hunks)packages/react-core/src/demos/examples/Page/PageDockedNav.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/react-core/src/components/Compass/examples/Compass.md
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build
- GitHub Check: Build, test & deploy
🔇 Additional comments (9)
packages/react-core/src/components/Nav/NavItem.tsx (4)
1-11: Excellent fix to the type safety issues!The imports now include
forwardRefanduseCallback, and theinnerRefprop is properly typed asReact.Ref<HTMLLIElement>instead of the previously unsafeReact.Ref<any>. This addresses the critical type safety concern from the previous review.Also applies to: 55-56
83-94: Perfect ref handling implementation!All the critical ref handling issues from the previous review have been properly addressed:
flyoutRefnow correctly initialized withnullinstead ofundefined- The ref callback pattern safely handles both callback refs and object refs with a proper type guard
- The forwarding logic ensures the DOM node is available both locally (via
flyoutRef) and to the parent (viainnerRef)This implementation is type-safe and will not crash with either ref type.
95-97: Flyout visibility comparison correctly fixed!The
flyoutVisiblelogic now properly compares DOM nodes (flyoutRef?.current === contextFlyoutRefCurrent) instead of ref object identity. This addresses the critical comparison issue flagged in the previous review.
303-305: Critical prop spread order issue resolved!The forwardRef wrapper now correctly spreads props first, then explicitly sets
innerRef={ref}, ensuring the forwarded ref takes precedence over anyinnerRefthat might be passed in props. This fixes the critical prop override vulnerability from the previous review.The entire ref-forwarding architecture is now sound and follows React best practices.
packages/react-core/src/demos/examples/Page/PageDockedNav.tsx (2)
42-43: LGTM: Relative imports correctly address previous feedback.The asset imports now use relative paths instead of internal package paths, resolving the brittleness flagged in the previous review.
138-177: LGTM: Nav items now have unique IDs and itemIds.Each NavItem correctly uses distinct
id,to,itemId, andaria-labelattributes, fixing the duplicate ID and selection issues from the previous review.packages/react-core/src/demos/Page.md (1)
59-63: LGTM: Docked nav documentation section added appropriately.The new "Docked nav" section correctly references the new demo component and fits well within the existing documentation structure.
packages/react-core/src/components/Compass/examples/CompassDockDemo.tsx (2)
136-139: VerifyTooltip+triggerRefbehavior in this PF version
You’re rendering tooltips withtriggerRefand no children (Line 136–139). Please confirm in the preview/a11y report that tooltips actually attach to theNavItemanchors and behave as expected (hover/focus).
50-52: Nav selection +itemId/isActivealignment looks correct now
UniqueitemIds and matchingisActivechecks are consistent, and icon-only items havearia-labels.Also applies to: 90-134
| @@ -0,0 +1,196 @@ | |||
| import { useRef, useState } from 'react'; | |||
There was a problem hiding this comment.
Fix missing React identifier (likely TS compile error)
You reference React.FunctionComponent, React.FormEvent, and React.Ref, but React isn’t imported (Line 46, Line 50, Line 160). Prefer importing the needed types from react (or import * as React).
-import { useRef, useState } from 'react';
+import { useRef, useState, type FormEvent, type FunctionComponent, type Ref } from 'react';
@@
-export const CompassDockDemo: React.FunctionComponent = () => {
+export const CompassDockDemo: FunctionComponent = () => {
@@
- const onNavSelect = (_event: React.FormEvent<HTMLInputElement>, selectedItem: NavOnSelectProps) => {
+ const onNavSelect = (_event: FormEvent<HTMLInputElement>, selectedItem: NavOnSelectProps) => {
typeof selectedItem.itemId === 'number' && setActiveItem(selectedItem.itemId);
};
@@
- toggle={(toggleRef: React.Ref<MenuToggleElement>) => (
+ toggle={(toggleRef: Ref<MenuToggleElement>) => (
<MenuToggle
ref={toggleRef}Also applies to: 46-52, 160-168
packages/react-core/src/demos/Compass/examples/CompassDockDemo.tsx
Outdated
Show resolved
Hide resolved
|
|
||
| const mainContainerId = 'main-content-page-layout-tertiary-nav'; | ||
|
|
||
| const handleClick = (event) => { |
There was a problem hiding this comment.
Add type annotation for event parameter.
The event parameter lacks a type annotation.
Apply this diff:
- const handleClick = (event) => {
+ const handleClick = (event: React.MouseEvent<HTMLAnchorElement>) => {
event.preventDefault();📝 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.
| const handleClick = (event) => { | |
| const handleClick = (event: React.MouseEvent<HTMLAnchorElement>) => { | |
| event.preventDefault(); |
🤖 Prompt for AI Agents
In packages/react-core/src/demos/examples/Page/PageDockedNav.tsx around line
225, the handleClick function's event parameter is missing a type; annotate it
(e.g. event: React.MouseEvent<HTMLElement> or the more specific element type
like HTMLButtonElement/HTMLAnchorElement) and ensure React is imported for the
type (or reference JSX types via global namespace). Update the function
signature to use the chosen React.MouseEvent generic so the event has proper
typing.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/react-core/src/components/Nav/NavItem.tsx (1)
83-85: Critical: Unsafe ref handling will crash with callback refs.Line 84 unsafely casts
innerReftoMutableRefObjectwithout checking if it's a callback function. When a callback ref is forwarded through theforwardRefwrapper (line 285), this cast will fail because callback refs don't have a.currentproperty. The|| _reffallback logic is also broken since functions are truthy.Additionally:
- Line 83: Use
nullinstead ofundefinedfor ref initialization (TypeScript best practice)- Line 85: After fixing ref handling, compare
.currentvalues instead of ref object identityApply this pattern to safely handle both callback and object refs:
- const _ref = useRef<HTMLLIElement>(undefined); - const ref = (innerRef as MutableRefObject<HTMLLIElement>) || _ref; + const _ref = useRef<HTMLLIElement | null>(null); + + const setRef = useCallback((node: HTMLLIElement | null) => { + _ref.current = node; + if (typeof innerRef === 'function') { + innerRef(node); + } else if (innerRef) { + (innerRef as MutableRefObject<HTMLLIElement | null>).current = node; + } + }, [innerRef]);Then update line 267 and 247:
- ref={ref} + ref={setRef}- triggerRef={ref} + triggerRef={_ref}And line 85:
- const flyoutVisible = ref === flyoutRef; + const flyoutVisible = _ref.current === flyoutRef?.current;
🧹 Nitpick comments (1)
packages/react-core/src/demos/examples/Page/PageDockedNav.tsx (1)
110-116: Remove unnecessary fragment wrapper.The
userDropdownItemsarray contains a single fragment wrapping the dropdown items. Since arrays can directly contain elements, the fragment is redundant.Apply this diff:
const userDropdownItems = [ - <> - <DropdownItem key="group 2 profile">My profile</DropdownItem> - <DropdownItem key="group 2 user">User management</DropdownItem> - <DropdownItem key="group 2 logout">Logout</DropdownItem> - </> + <DropdownItem key="group 2 profile">My profile</DropdownItem>, + <DropdownItem key="group 2 user">User management</DropdownItem>, + <DropdownItem key="group 2 logout">Logout</DropdownItem> ];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/react-core/src/components/Nav/NavItem.tsx(4 hunks)packages/react-core/src/demos/Page.md(3 hunks)packages/react-core/src/demos/examples/Page/PageDockedNav.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/react-core/src/demos/Page.md
🧰 Additional context used
🧬 Code graph analysis (1)
packages/react-core/src/components/Nav/NavItem.tsx (2)
packages/react-core/src/components/Nav/Nav.tsx (1)
NavContext(63-63)packages/react-core/src/components/Page/PageSidebar.tsx (1)
PageSidebarContext(28-28)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build
- GitHub Check: Build, test & deploy
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
packages/react-core/src/components/Nav/NavItem.tsx (2)
1-11: Avoid importingMutableRefObjectfor unsafe casts; merge refs safely instead.
MutableRefObjectis only used to force-castinnerRef, which breaks wheninnerRefis a callback ref (validReact.Refcase). Prefer a merged-ref callback and keep a stable localRefObjectfor internal logic/Popper.
55-56: Fix callback-ref crashes + incorrectuseRef(undefined)+ fragile flyout visibility check.
Current code will crash wheninnerRefis a callback ref (e.g., whenNavItemis used viaforwardRef), because it’s cast toMutableRefObjectand later read via.current(Line 131 / Popper triggerRef). AlsouseRef<HTMLLIElement>(undefined)should benull, andflyoutVisible = ref === flyoutRefis fragile once refs are merged.Proposed patch (keeps a stable internal ref object for Popper + context, while still forwarding to consumer ref safely):
import { cloneElement, Fragment, isValidElement, useContext, useEffect, + useCallback, useRef, useState, forwardRef, - MutableRefObject } from 'react'; @@ icon, innerRef, ...props }: NavItemProps) => { const { flyoutRef, setFlyoutRef, navRef } = useContext(NavContext); @@ - const _ref = useRef<HTMLLIElement>(undefined); - const ref = (innerRef as MutableRefObject<HTMLLIElement>) || _ref; - const flyoutVisible = ref === flyoutRef; - const popperRef = useRef<HTMLDivElement>(undefined); + const localRef = useRef<HTMLLIElement | null>(null); + const popperRef = useRef<HTMLDivElement | null>(null); + + const setMergedRef = useCallback( + (node: HTMLLIElement | null) => { + localRef.current = node; + if (!innerRef) return; + if (typeof innerRef === 'function') { + innerRef(node); + } else { + (innerRef as React.MutableRefObject<HTMLLIElement | null>).current = node; + } + }, + [innerRef] + ); + + const flyoutVisible = !!localRef.current && flyoutRef?.current === localRef.current; @@ - setFlyoutRef(ref); + setFlyoutRef(localRef); @@ - } else if ((flyoutVisible || override) && !show) { + } else if ((flyoutVisible || override) && !show) { setFlyoutRef(null); } @@ - if ((key === ' ' || key === 'Enter' || key === 'ArrowRight') && hasFlyout && ref?.current?.contains(target)) { + if ( + (key === ' ' || key === 'Enter' || key === 'ArrowRight') && + hasFlyout && + localRef.current?.contains(target) + ) { @@ const flyoutPopper = ( <Popper - triggerRef={ref} + triggerRef={localRef} popper={ <div ref={popperRef} onMouseEnter={handleMouseEnter} onMouseLeave={handleMouseLeave}> {flyout} </div> } popperRef={popperRef} @@ const navItem = ( <> <li onMouseOver={onMouseOver} className={css(styles.navItem, hasFlyout && styles.modifiers.flyout, className)} - ref={ref} + ref={setMergedRef} {...ouiaProps} >Note: this assumes
NavContextProps['flyoutRef']is aRefObject<HTMLLIElement> | null(or compatible). If it’s currentlyMutableRefObject<HTMLLIElement>, it should be widened toRefObject<HTMLLIElement>and usecurrentcomparisons consistently.Also applies to: 83-86, 131-138, 245-260, 264-268
packages/react-core/src/components/Compass/examples/CompassDockDemo.tsx (1)
1-1: Fix missingReactidentifier (TypeScript compile error).
React.FunctionComponent(line 46),React.FormEvent(line 50), andReact.Ref(line 160) reference theReactnamespace, but only named exports are imported. This will fail to compile.-import { useRef, useState } from 'react'; +import React, { useRef, useState } from 'react';Alternatively, import only the needed types:
-import { useRef, useState } from 'react'; +import { useRef, useState, type FunctionComponent, type FormEvent, type Ref } from 'react';Then update usages accordingly (e.g.,
FunctionComponentinstead ofReact.FunctionComponent).
🧹 Nitpick comments (1)
packages/react-core/src/components/Compass/examples/CompassDockDemo.tsx (1)
62-68: Consider simplifying the dropdown items declaration.Wrapping a fragment inside a single-element array is an unusual pattern that could trigger React key warnings. If
DropdownListexpects children, a direct fragment or inline children would be cleaner.- const userDropdownItems = [ - <> - <DropdownItem key="group 2 profile">My profile</DropdownItem> - <DropdownItem key="group 2 user">User management</DropdownItem> - <DropdownItem key="group 2 logout">Logout</DropdownItem> - </> - ]; + const userDropdownItems = ( + <> + <DropdownItem key="group 2 profile">My profile</DropdownItem> + <DropdownItem key="group 2 user">User management</DropdownItem> + <DropdownItem key="group 2 logout">Logout</DropdownItem> + </> + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/react-core/src/components/Compass/examples/CompassDockDemo.tsx(1 hunks)packages/react-core/src/components/Nav/NavItem.tsx(4 hunks)packages/react-core/src/demos/examples/Page/PageDockedNav.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/react-core/src/demos/examples/Page/PageDockedNav.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
packages/react-core/src/components/Nav/NavItem.tsx (2)
packages/react-core/src/components/Nav/Nav.tsx (1)
NavContext(63-63)packages/react-core/src/components/Page/PageSidebar.tsx (1)
PageSidebarContext(28-28)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build
- GitHub Check: Build, test & deploy
🔇 Additional comments (4)
packages/react-core/src/components/Nav/NavItem.tsx (1)
285-287:forwardRef<HTMLLIElement, NavItemProps>wrapper looks correct; prop spread order is safe.
<NavItemBase {...props} innerRef={ref} />ensures the forwardedrefcan’t be overridden by a consumer-providedinnerRef(even if someone uses it despite@hide).packages/react-core/src/components/Compass/examples/CompassDockDemo.tsx (3)
88-139: NavItem configuration and tooltips look correct.The
itemIdvalues (0, 1, 2, 3) now correctly match their correspondingisActivechecks, and all icon-only NavItems have appropriatearia-labelattributes. The ref-based Tooltip wiring is properly implemented.
156-173: User menu dropdown is properly accessible.The
MenuTogglenow includesaria-label="User menu"for the icon-only toggle, addressing the accessibility concern.
189-196: Component structure is well-organized.The
Compasscomponent properly receivesdockandmaincontent, demonstrating the new docked navigation layout feature as intended by the PR objectives.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/react-integration/cypress/integration/drawer.spec.ts (1)
49-50: Make thisflex-basisassertion tolerant to equivalent CSS normalizations (avoid brittle exact-string match).
min(100% + 0px, 100%)is semantically equivalent tomin(100%, 100%), but different engines/versions may stringify these differently, causing flaky runs.Suggested tweak (accept either form):
cy.viewport(800, 660); -cy.get('#basic-drawer .pf-v6-c-drawer__panel').should('have.css', 'flex-basis', 'max(24px, min(100% + 0px, 100%))'); +const mediumFlexBasis = new Set([ + 'max(24px, min(100%, 100%))', + 'max(24px, min(100% + 0px, 100%))' +]); +cy.get('#basic-drawer .pf-v6-c-drawer__panel') + .invoke('css', 'flex-basis') + .then((v) => expect(mediumFlexBasis.has(v), `unexpected flex-basis: ${v}`).to.eq(true));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/react-integration/cypress/integration/drawer.spec.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build
- GitHub Check: Build, test & deploy
mcoker
requested review from
andrew-ronaldson,
lboehling,
mcoker,
nicolethoen and
thatblindgeye
mcoker
left a comment
mcoker
left a comment
There was a problem hiding this comment.
A couple of nav items aren't updating as "current" correctly, and the logo should be 37px, which matches the default button/form element size.
packages/react-core/src/components/Compass/examples/CompassDockDemo.tsx
Outdated
Show resolved
Hide resolved
andrew-ronaldson
left a comment
andrew-ronaldson
left a comment
There was a problem hiding this comment.
Visually good but defer to the dev reviewers
Co-authored-by: Eric Olkowski <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/react-core/src/components/Compass/examples/CompassDockDemo.tsx (1)
1-1: Prefer imported React types over globalReact.*namespaceYou’re using
React.FunctionComponent,React.FormEvent, andReact.Refwithout importing theReactidentifier. This relies on the global React namespace and can break under stricter TS configs or future typings. It’s safer and clearer to import the needed types directly fromreactand drop theReact.prefix.Example refactor:
-import { useRef, useState } from 'react'; +import { useRef, useState, type FormEvent, type FunctionComponent, type Ref } from 'react'; -export const CompassDockDemo: React.FunctionComponent = () => { +export const CompassDockDemo: FunctionComponent = () => { @@ - const onNavSelect = (_event: React.FormEvent<HTMLInputElement>, selectedItem: NavOnSelectProps) => { - typeof selectedItem.itemId === 'number' && setActiveItem(selectedItem.itemId); - }; + const onNavSelect = (_event: FormEvent<HTMLElement>, selectedItem: NavOnSelectProps) => { + if (typeof selectedItem.itemId === 'number') { + setActiveItem(selectedItem.itemId); + } + }; @@ - toggle={(toggleRef: React.Ref<MenuToggleElement>) => ( + toggle={(toggleRef: Ref<MenuToggleElement>) => (This keeps everything type-safe, avoids depending on a global
React, and addresses the earlier “missing React identifier” concern in a more robust way.Also applies to: 46-52, 160-168
🧹 Nitpick comments (1)
packages/react-core/src/components/Compass/examples/CompassDockDemo.tsx (1)
40-52: Reuse the libraryNavOnSelectPropstype instead of redefining itThe local
NavOnSelectPropsinterface is structurally fine, but it shadows the PatternFly type of the same name and can drift from upstream if that type changes. Prefer importing the type from@patternfly/react-core(or deriving fromNavProps['onSelect']) so your handler stays aligned with the component’s official signature.Sketch:
-import { - Compass, - ... - Nav, - ... -} from '@patternfly/react-core'; +import { + Compass, + ... + Nav, + type NavOnSelectProps, + ... +} from '@patternfly/react-core'; - -interface NavOnSelectProps { - groupId: number | string; - itemId: number | string; - to: string; -} +// Use the library NavOnSelectProps so this stays in sync with NavIf for some reason your current PatternFly version does not export
NavOnSelectProps, the existing local interface is acceptable; this is mainly about future-proofing and consistency with the rest of the codebase.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/react-core/src/components/Compass/examples/CompassDockDemo.tsx(1 hunks)packages/react-core/src/demos/examples/Page/PageDockedNav.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/react-core/src/demos/examples/Page/PageDockedNav.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Unit tests
- GitHub Check: Build demo app
- GitHub Check: Build, test & deploy
🔇 Additional comments (1)
packages/react-core/src/components/Compass/examples/CompassDockDemo.tsx (1)
70-177: Docked masthead/nav structure and accessibility look solidThe overall docked masthead + vertical toolbar/nav composition looks good:
- Nav item
itemIdandisActiveare now consistent (0–3), so selection state behaves correctly.useRef+triggerRefwiring for the four tooltips is clean and idiomatic.- Icon-only controls have accessible names (
aria-labelon NavItems, settings/help buttons, and the userMenuToggle), which aligns well with a11y expectations for a docked, icon-only layout.No functional or accessibility issues stand out in this block.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/react-core/src/demos/Compass/examples/CompassDockDemo.tsx (1)
1-1: Import and use React types correctly (current usage will not compile).
React.FunctionComponent,React.FormEvent, andReact.Refare used without importingReactor those types, which will cause TS compile errors. Prefer importing the needed types fromreactand dropping theReact.prefix.Suggested fix:
-import { useRef, useState } from 'react'; +import { useRef, useState, type FormEvent, type FunctionComponent, type Ref } from 'react'; -export const CompassDockDemo: React.FunctionComponent = () => { +export const CompassDockDemo: FunctionComponent = () => { - const onNavSelect = (_event: React.FormEvent<HTMLInputElement>, selectedItem: NavOnSelectProps) => { + const onNavSelect = (_event: FormEvent<HTMLInputElement>, selectedItem: NavOnSelectProps) => { - toggle={(toggleRef: React.Ref<MenuToggleElement>) => ( + toggle={(toggleRef: Ref<MenuToggleElement>) => (Please confirm this matches the repo’s preferred style (named type imports vs.
import * as React).Also applies to: 46-52, 160-169
🧹 Nitpick comments (1)
packages/react-core/src/demos/Compass/examples/CompassDockDemo.tsx (1)
54-60: Simplify dropdown open/close logic to avoid double-toggling and stale state (optional).The dropdown state is toggled using
!isDropdownOpenin multiple handlers while also controlled viaonOpenChange. This works but is slightly brittle; using a functional updater and closing explicitly on select is clearer.Consider:
- const onDropdownToggle = () => { - setIsDropdownOpen(!isDropdownOpen); - }; - - const onDropdownSelect = () => { - setIsDropdownOpen(!isDropdownOpen); - }; + const onDropdownToggle = () => { + setIsDropdownOpen(prev => !prev); + }; + + const onDropdownSelect = () => { + setIsDropdownOpen(false); + };This keeps the component fully controlled and avoids any race with
onOpenChange. Please confirm this matches howDropdownis expected to be used in the current PatternFly docs.Also applies to: 155-172
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/react-core/src/components/Compass/examples/Compass.md(2 hunks)packages/react-core/src/demos/Compass/Compass.md(2 hunks)packages/react-core/src/demos/Compass/examples/CompassDockDemo.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/react-core/src/components/Compass/examples/Compass.md
🧰 Additional context used
🧬 Code graph analysis (1)
packages/react-core/src/demos/Compass/examples/CompassDockDemo.tsx (19)
packages/react-core/src/components/Dropdown/DropdownItem.tsx (1)
DropdownItem(68-72)packages/react-core/src/components/Masthead/Masthead.tsx (1)
Masthead(34-59)packages/react-core/src/components/Masthead/MastheadLogo.tsx (1)
MastheadLogo(14-33)packages/react-core/src/components/Brand/Brand.tsx (1)
Brand(37-82)packages/react-core/src/components/Divider/Divider.tsx (1)
Divider(38-60)packages/react-core/src/components/Toolbar/Toolbar.tsx (1)
Toolbar(226-226)packages/react-core/src/components/Toolbar/ToolbarContent.tsx (1)
ToolbarContent(127-127)packages/react-core/src/components/Nav/Nav.tsx (1)
Nav(174-174)packages/react-core/src/components/Nav/NavList.tsx (1)
NavList(174-174)packages/react-core/src/components/Nav/NavItem.tsx (1)
NavItem(285-287)packages/react-core/src/components/Tooltip/Tooltip.tsx (1)
Tooltip(142-373)packages/react-core/src/components/Toolbar/ToolbarGroup.tsx (1)
ToolbarGroup(237-239)packages/react-core/src/components/Dropdown/Dropdown.tsx (1)
Dropdown(215-217)packages/react-core/src/components/MenuToggle/MenuToggle.tsx (2)
MenuToggleElement(23-23)MenuToggle(246-248)packages/react-core/src/components/Avatar/Avatar.tsx (1)
Avatar(18-32)packages/react-core/src/components/Dropdown/DropdownList.tsx (1)
DropdownList(11-19)packages/react-core/src/components/Compass/CompassContent.tsx (1)
CompassContent(16-40)packages/react-core/src/components/Compass/CompassPanel.tsx (1)
CompassPanel(23-49)packages/react-core/src/components/Compass/Compass.tsx (1)
Compass(41-123)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build
- GitHub Check: Build, test & deploy
🔇 Additional comments (2)
packages/react-core/src/demos/Compass/Compass.md (2)
36-40: New demo section is properly structured and follows existing patterns.The "Docked nav demo" section is well-formatted and consistent with the existing "Compass layout" demo. Ensure the referenced CompassDockDemo.tsx file exists (verified in the previous check).
The verification script above will confirm that CompassDockDemo.tsx exists.
12-18: All imported icons and assets are properly used in the referenced example files. The verification confirms:
- CubeIcon, FolderIcon, QuestionCircleIcon, CloudIcon, and CodeIcon are all imported and actively used in CompassDockDemo.tsx
- imgAvatar and pfLogo asset imports are both used in CompassDockDemo.tsx
- All asset files (avatarImg.svg and PF-IconLogo-color.svg) exist at the expected locations
No issues found with the imports or asset references.
| const navItem1Ref = useRef<HTMLAnchorElement>(null); | ||
| const navItem2Ref = useRef<HTMLAnchorElement>(null); | ||
| const navItem3Ref = useRef<HTMLAnchorElement>(null); | ||
| const navItem4Ref = useRef<HTMLAnchorElement>(null); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and examine NavItem.tsx
find . -name "NavItem.tsx" -type fRepository: patternfly/patternfly-react
Length of output: 123
🏁 Script executed:
# Find the CompassDockDemo.tsx file and check its location
find . -name "CompassDockDemo.tsx" -type fRepository: patternfly/patternfly-react
Length of output: 139
🏁 Script executed:
# Search for Tooltip component definition
find . -name "Tooltip.tsx" -o -name "Tooltip.ts" -o -name "Tooltip.d.ts" | head -20Repository: patternfly/patternfly-react
Length of output: 127
🏁 Script executed:
cat -n ./packages/react-core/src/components/Nav/NavItem.tsx | head -100Repository: patternfly/patternfly-react
Length of output: 4793
🏁 Script executed:
cat -n ./packages/react-core/src/demos/Compass/examples/CompassDockDemo.tsx | sed -n '65,145p'Repository: patternfly/patternfly-react
Length of output: 3796
🏁 Script executed:
cat -n ./packages/react-core/src/components/Tooltip/Tooltip.tsx | head -150Repository: patternfly/patternfly-react
Length of output: 7072
🏁 Script executed:
cat -n ./packages/react-core/src/components/Nav/NavItem.tsx | sed -n '100,250p'Repository: patternfly/patternfly-react
Length of output: 5613
🏁 Script executed:
# Check the export at the end of NavItem.tsx
tail -50 ./packages/react-core/src/components/Nav/NavItem.tsxRepository: patternfly/patternfly-react
Length of output: 1253
Fix NavItem ref types to match HTMLLIElement forward.
NavItem forwards its ref as HTMLLIElement (via forwardRef<HTMLLIElement, NavItemProps>), but the refs here are incorrectly typed as HTMLAnchorElement. Change to HTMLLIElement | null to align with the actual DOM element being referenced:
- const navItem1Ref = useRef<HTMLAnchorElement>(null);
- const navItem2Ref = useRef<HTMLAnchorElement>(null);
- const navItem3Ref = useRef<HTMLAnchorElement>(null);
- const navItem4Ref = useRef<HTMLAnchorElement>(null);
+ const navItem1Ref = useRef<HTMLLIElement | null>(null);
+ const navItem2Ref = useRef<HTMLLIElement | null>(null);
+ const navItem3Ref = useRef<HTMLLIElement | null>(null);
+ const navItem4Ref = useRef<HTMLLIElement | null>(null);This applies to lines 70–73 and also to lines 90–139 where these refs are used.
📝 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.
| const navItem1Ref = useRef<HTMLAnchorElement>(null); | |
| const navItem2Ref = useRef<HTMLAnchorElement>(null); | |
| const navItem3Ref = useRef<HTMLAnchorElement>(null); | |
| const navItem4Ref = useRef<HTMLAnchorElement>(null); | |
| const navItem1Ref = useRef<HTMLLIElement | null>(null); | |
| const navItem2Ref = useRef<HTMLLIElement | null>(null); | |
| const navItem3Ref = useRef<HTMLLIElement | null>(null); | |
| const navItem4Ref = useRef<HTMLLIElement | null>(null); |
🤖 Prompt for AI Agents
In packages/react-core/src/demos/Compass/examples/CompassDockDemo.tsx around
lines 70–73 (and also where those refs are used around lines 90–139), the
NavItem refs are typed as HTMLAnchorElement but NavItem.forwardRef returns an
HTMLLIElement; change the useRef types to useRef<HTMLLIElement | null>(null)
(and update any other declarations/usages of these refs within lines 90–139 to
reflect HTMLLIElement | null) so the ref types match the forwarded element and
TypeScript no longer errors.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/react-core/src/demos/Compass/examples/CompassDockDemo.tsx (1)
1-2: Prefer explicit React type imports overReact.*namespace usageYou’re using
React.FunctionComponent,React.FormEvent, andReact.Refwithout importingReactor these types. This often relies on implicit global React typings and can break with stricter TS/react configs. Importing the types directly is clearer and more robust.Suggested diff:
-import { useRef, useState } from 'react'; +import { useRef, useState, type FormEvent, type FunctionComponent, type Ref } from 'react'; -export const CompassDockDemo: React.FunctionComponent = () => { +export const CompassDockDemo: FunctionComponent = () => { @@ - const onNavSelect = (_event: React.FormEvent<HTMLInputElement>, selectedItem: NavOnSelectProps) => { + const onNavSelect = (_event: FormEvent<HTMLInputElement>, selectedItem: NavOnSelectProps) => { @@ - toggle={(toggleRef: React.Ref<MenuToggleElement>) => ( + toggle={(toggleRef: Ref<MenuToggleElement>) => (Please confirm this aligns with your repo’s TS/React version and style.
Also applies to: 46-52, 160-168
🧹 Nitpick comments (2)
packages/react-core/src/demos/Compass/examples/CompassDockDemo.tsx (1)
54-60: Use functional state updates and a shared dropdown toggle handlerBoth dropdown handlers flip the same boolean state using the current closure value, which is slightly brittle under concurrent rendering and duplicates logic. A small refactor improves correctness and readability.
- const onDropdownToggle = () => { - setIsDropdownOpen(!isDropdownOpen); - }; - - const onDropdownSelect = () => { - setIsDropdownOpen(!isDropdownOpen); - }; + const toggleDropdown = () => { + setIsDropdownOpen(prevIsOpen => !prevIsOpen); + }; + + const onDropdownToggle = toggleDropdown; + + const onDropdownSelect = toggleDropdown;packages/react-core/src/demos/examples/Page/PageDockedNav.tsx (1)
78-84: Simplify array structure by removing unnecessary fragment.The single fragment wrapping all dropdown items is unnecessary. The array can directly contain the elements.
Apply this diff:
const userDropdownItems = [ - <> - <DropdownItem key="group 2 profile">My profile</DropdownItem> - <DropdownItem key="group 2 user">User management</DropdownItem> - <DropdownItem key="group 2 logout">Logout</DropdownItem> - </> + <DropdownItem key="group 2 profile">My profile</DropdownItem>, + <DropdownItem key="group 2 user">User management</DropdownItem>, + <DropdownItem key="group 2 logout">Logout</DropdownItem> ];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/react-core/src/components/Nav/NavItem.tsx(6 hunks)packages/react-core/src/demos/Compass/examples/CompassDockDemo.tsx(1 hunks)packages/react-core/src/demos/examples/Page/PageDockedNav.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/react-core/src/demos/examples/Page/PageDockedNav.tsx (24)
packages/react-core/src/components/Breadcrumb/Breadcrumb.tsx (1)
Breadcrumb(19-42)packages/react-core/src/components/Dropdown/DropdownItem.tsx (1)
DropdownItem(68-72)packages/react-core/src/components/Masthead/Masthead.tsx (1)
Masthead(34-59)packages/react-core/src/components/Masthead/MastheadBrand.tsx (1)
MastheadBrand(11-19)packages/react-core/src/components/Masthead/MastheadLogo.tsx (1)
MastheadLogo(14-33)packages/react-core/src/components/Brand/Brand.tsx (1)
Brand(37-82)packages/react-core/src/components/Divider/Divider.tsx (1)
Divider(38-60)packages/react-core/src/components/Toolbar/Toolbar.tsx (1)
Toolbar(226-226)packages/react-core/src/components/Toolbar/ToolbarContent.tsx (1)
ToolbarContent(127-127)packages/react-core/src/components/Toolbar/ToolbarItem.tsx (1)
ToolbarItem(173-237)packages/react-core/src/components/Nav/Nav.tsx (1)
Nav(174-174)packages/react-core/src/components/Nav/NavList.tsx (1)
NavList(174-174)packages/react-core/src/components/Tooltip/Tooltip.tsx (1)
Tooltip(142-373)packages/react-core/src/components/Dropdown/Dropdown.tsx (1)
Dropdown(215-217)packages/react-core/src/components/MenuToggle/MenuToggle.tsx (2)
MenuToggleElement(23-23)MenuToggle(246-248)packages/react-core/src/components/Avatar/Avatar.tsx (1)
Avatar(18-32)packages/react-core/src/components/Dropdown/DropdownList.tsx (1)
DropdownList(11-19)packages/react-core/src/components/SkipToContent/SkipToContent.tsx (1)
SkipToContent(14-25)packages/react-core/src/components/Page/Page.tsx (1)
Page(367-367)packages/react-core/src/components/Page/PageSection.tsx (1)
PageSection(85-137)packages/react-core/src/components/Content/Content.tsx (1)
Content(85-118)packages/react-core/src/layouts/Gallery/Gallery.tsx (1)
Gallery(34-72)packages/react-core/src/components/Card/Card.tsx (1)
Card(155-155)packages/react-core/src/components/Card/CardBody.tsx (1)
CardBody(17-30)
packages/react-core/src/components/Nav/NavItem.tsx (2)
packages/react-core/src/components/Nav/Nav.tsx (1)
NavContext(63-63)packages/react-core/src/components/Page/PageSidebar.tsx (1)
PageSidebarContext(28-28)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build, test & deploy
🔇 Additional comments (2)
packages/react-core/src/demos/Compass/examples/CompassDockDemo.tsx (2)
70-139: Icon-only docked nav items + tooltips look correct and accessibleThe docked
Navwiring withanchorRefandTooltipplus per-itemaria-labels is coherent: IDs,itemId/isActivevalues, and tooltip trigger refs all line up and give each icon-only item an accessible name. No issues from my side here.
189-195: Compass dock/main composition cleanly demonstrates the docked layoutThe
Compassusage withdock={dockContent}andmain={mainContent}plus background assets is straightforward and clearly showcases the new docked nav layout.
| const _ref = useRef<HTMLLIElement>(undefined); | ||
| const ref = (innerRef as MutableRefObject<HTMLLIElement>) || _ref; | ||
| const flyoutVisible = ref === flyoutRef; |
There was a problem hiding this comment.
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:
-
Line 86:
useRef<HTMLLIElement>(undefined)should initialize withnull(React convention). -
Line 87: Unsafe cast of
innerRef as MutableRefObject<HTMLLIElement>assumesinnerRefhas a.currentproperty. 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'sref?.current?.contains(target)will evaluate toundefined, silently breaking flyout keyboard navigation and hover detection. -
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.
| @@ -0,0 +1,242 @@ | |||
| import { useRef, useState } from 'react'; | |||
There was a problem hiding this comment.
Add missing React import for type references.
The code references React.FunctionComponent (line 51), React.FormEvent (line 55), React.Ref (line 173), and React.MouseEvent (line 195) without importing React. This will cause a TypeScript compilation error.
Apply this diff:
-import { useRef, useState } from 'react';
+import React, { useRef, useState } from 'react';📝 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.
| import { useRef, useState } from 'react'; | |
| import React, { useRef, useState } from 'react'; |
🤖 Prompt for AI Agents
In packages/react-core/src/demos/examples/Page/PageDockedNav.tsx around line 1,
the file uses React types (React.FunctionComponent, React.FormEvent, React.Ref,
React.MouseEvent) but only imports useRef and useState; add a React import so
the type names resolve. Update the top-level imports to include React (for
example import * as React from 'react' or import React from 'react') or
alternatively add explicit type-only imports (import type { FunctionComponent,
FormEvent, Ref, MouseEvent } from 'react') so TypeScript can compile the
referenced types.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/react-core/src/components/Toolbar/__tests__/Toolbar.test.tsx (1)
223-240: Consider using template literal for consistency.The test logic is correct and provides good coverage for the
isVerticalprop. However, for consistency with other modifier tests (lines 46, 60, 209), consider using template literals (backticks) instead of single quotes in the test description.Apply this diff to align with the established pattern:
- it('Renders with class ${styles.modifiers.vertical} when isVertical is true', () => { + it(`Renders with class ${styles.modifiers.vertical} when isVertical is true`, () => {This ensures the test output displays the actual CSS class name rather than the literal text.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/react-core/src/components/Toolbar/__tests__/Toolbar.test.tsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/react-core/src/components/Toolbar/__tests__/Toolbar.test.tsx (1)
packages/react-core/src/components/Toolbar/ToolbarGroup.tsx (1)
render(178-234)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build
- GitHub Check: Build, test & deploy
🔇 Additional comments (2)
packages/react-core/src/components/Toolbar/__tests__/Toolbar.test.tsx (2)
57-57: Good improvement for maintainability.Referencing the modifier via
styles.modifiers.noPaddinginstead of a hardcoded string is more maintainable and consistent with the pattern used elsewhere in the test file.
209-221: LGTM! Well-structured test for default behavior.The test correctly verifies that the vertical modifier is not applied by default and follows the established patterns in the file.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
packages/react-core/src/demos/examples/Page/PageDockedNav.tsx (2)
41-42: Avoid importing demo assets via@patternfly/react-core/src/...; use relative paths.These imports reach into the package’s internal
srcstructure, which is brittle and may break with bundler/module-resolution changes or when the package layout shifts. Prefer relative imports from the demo assets directory instead.For example:
-import imgAvatar from '@patternfly/react-core/src/components/assets/avatarImg.svg'; -import pfIconLogo from '@patternfly/react-core/src/demos/assets/PF-IconLogo-color.svg'; +import imgAvatar from '../../../components/assets/avatarImg.svg'; +import pfIconLogo from '../../assets/PF-IconLogo-color.svg';
1-1: Import React (or React types) when using theReact.*namespace.This file uses
React.FunctionComponent,React.FormEvent, andReact.MouseEventbut only imports hooks. Unless your TS config intentionally exposes a globalReactnamespace, this can cause type errors; it’s safer to explicitly import React (or the relevant types) from'react'.For example:
-import { useRef, useState } from 'react'; +import React, { useRef, useState } from 'react';Or, if you prefer type-only imports, adjust the annotations and add:
import type { FunctionComponent, FormEvent, MouseEvent } from 'react';#!/bin/bash # Compare with other demos that use React.FunctionComponent to ensure consistency. rg -n "React\\.FunctionComponent" packages/react-core/src/demos/examples -C2packages/react-core/src/demos/Compass/examples/CompassDockDemo.tsx (1)
1-68: Import React types explicitly and reuse them in signatures.You’re using
React.FunctionComponentandReact.FormEventwithout importingReactor those types. In modern TS/React setups this often relies on ambient globals and can break with stricter configs or future React typings. Prefer importing the types and using them directly.For example:
-import { useRef, useState } from 'react'; +import { useRef, useState, type FormEvent, type FunctionComponent } from 'react'; @@ -export const CompassDockDemo: React.FunctionComponent = () => { +export const CompassDockDemo: FunctionComponent = () => { @@ - const onNavSelect = (_event: React.FormEvent<HTMLInputElement>, selectedItem: NavOnSelectProps) => { + const onNavSelect = (_event: FormEvent<HTMLInputElement>, selectedItem: NavOnSelectProps) => { typeof selectedItem.itemId === 'number' && setActiveItem(selectedItem.itemId); };Optionally, if
NavOnSelectProps(or an equivalent) is exported from@patternfly/react-core, consider importing that instead of re-declaring the interface here to avoid future drift.
🧹 Nitpick comments (2)
packages/react-core/src/demos/examples/Page/PageDockedNav.tsx (1)
44-55: Consolidate the localNavOnSelectPropsinterface to avoid code duplication across demos.PatternFly's
Navcomponent doesn't export aNavOnSelectPropstype—only an inline object shape withinNavProps. The event type (React.FormEvent<HTMLInputElement>) is correct and matches the library's actual signature. However, this interface is redeclared identically across at least 8 demo files (PageDockedNav, PageStickySectionGroup, PageStickySectionGroupAlternate, PageStickySectionBreadcrumb, PageContextSelector, MastheadWithUtilitiesAndUserDropdownMenu, MastheadWithHorizontalNav, CompassDockDemo).Rather than importing (no export exists), consolidate by:
- Creating a shared types file in the demos directory and importing from there, or
- Removing the interface and inlining the type where needed
This reduces duplication and maintenance burden if the Nav component's shape evolves.
packages/react-core/src/demos/Compass/examples/CompassDockDemo.tsx (1)
167-190: Simplify Dropdown open-state handling to rely ononOpenChangeonly.Right now the user menu dropdown is controlled in two places:
onOpenChange={(isOpen: boolean) => setIsDropdownOpen(isOpen)}MenuToggle’sonClick={onDropdownToggle}which also flipsisDropdownOpen.Depending on how PatternFly wires
MenuToggle’s click handler, this dual control can lead to redundant or conflicting state updates. You can letDropdownown the toggle logic and treatonOpenChangeas the single source of truth:- const [isDropdownOpen, setIsDropdownOpen] = useState(false); - - const onDropdownToggle = () => { - setIsDropdownOpen((prevIsOpen) => !prevIsOpen); - }; + const [isDropdownOpen, setIsDropdownOpen] = useState(false); @@ - <Dropdown - isOpen={isDropdownOpen} - shouldFocusToggleOnSelect - onSelect={onDropdownSelect} - onOpenChange={(isOpen: boolean) => setIsDropdownOpen(isOpen)} + <Dropdown + isOpen={isDropdownOpen} + shouldFocusToggleOnSelect + onSelect={onDropdownSelect} + onOpenChange={setIsDropdownOpen} @@ - <MenuToggle - ref={userMenuRef} - onClick={onDropdownToggle} - isExpanded={isDropdownOpen} + <MenuToggle + ref={userMenuRef} + isExpanded={isDropdownOpen} icon={<Avatar src={imgAvatar} alt="" size="sm" />} variant="plain" aria-label="User menu"This keeps the open state fully controlled via the PatternFly
Dropdown’sonOpenChangecallback and avoids potential double-toggles, while still allowingonDropdownSelectto explicitly close on selection.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/react-core/src/demos/Compass/examples/CompassDockDemo.tsx(1 hunks)packages/react-core/src/demos/examples/Page/PageDockedNav.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build
- GitHub Check: Build, test & deploy
🔇 Additional comments (4)
packages/react-core/src/demos/examples/Page/PageDockedNav.tsx (1)
227-257: Page layout, docked masthead, and skip-to-content wiring look solid.
Pagewithvariant="docked", the composedmasthead,skipToContenttied tomainContainerId, and the card gallery content all follow expected PatternFly layout and a11y patterns. Nothing blocking here.packages/react-core/src/demos/Compass/examples/CompassDockDemo.tsx (3)
69-143: Nav item refs, selection, and a11y look solid for the docked variant.The docked
Navblock is wired cleanly:
itemIdvalues (0–3) now correctly align withactiveItemcomparisons and theonNavSelecthandler.- Using
anchorRef={navItemXRef}plusTooltip triggerRef={navItemXRef}is a good way to keep icon-only items focusable with hover/focus hints, whilearia-labelon eachNavItemprovides the accessible name.No changes needed here from a behavior or accessibility standpoint.
144-166: Settings/help toolbar icons are accessible and responsive.The settings and help buttons:
- Have explicit
aria-labels, which is important for icon-only controls.- Are wrapped in tooltips without overloading the accessible name.
- Use
visibility={{ default: 'hidden', lg: 'visible' }}to avoid cluttering smaller viewports.This section looks good as-is.
198-215: Compassdock/mainwiring matches the new docked layout API.Splitting the layout into
dockContentandmainContentand passing them intoCompassasdockandmainprops is a clean use of the new API and clearly demonstrates the docked navigation pattern. Background image usage is consistent for light/dark variants.No changes needed here.
thatblindgeye
left a comment
thatblindgeye
left a comment
There was a problem hiding this comment.
LGTM re: the updates made from my comments
(and obviously the updates I made look good to me 😎 )
5 participants
What: Closes #11146, #12149
Compass Demo: https://pf-react-pr-12175.surge.sh/ai/generative-uis/compass/react-demos/docked-nav-demo/
Page Demo: https://pf-react-pr-12175.surge.sh/components/page/react-demos/docked-nav/
Summary by CodeRabbit
New Features
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.