feat(Page): added responsive docked nav#12327
Open
thatblindgeye wants to merge 5 commits intopatternfly:mainfrom
Open
feat(Page): added responsive docked nav#12327thatblindgeye wants to merge 5 commits intopatternfly:mainfrom
thatblindgeye wants to merge 5 commits intopatternfly:mainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds dock and text-expanded props across Button, MenuToggle, Nav, MastheadLogo, and Page; restructures Page docked rendering to accept dockContent and expansion flags; updates docked demo to use new state and masthead variants; and adds unit tests for new modifier classes. Changes
Sequence DiagramsequenceDiagram
actor User
participant PageDockedDemo as PageDockedNav
participant Page as Page
participant Nav as Nav
participant Button as Button
participant MenuToggle as MenuToggle
User->>PageDockedDemo: Click dock/text toggle
PageDockedDemo->>PageDockedDemo: Update isDockExpanded / isDockTextExpanded state
PageDockedDemo->>Page: Pass isDockExpanded, isDockTextExpanded, dockContent, masthead
Page->>Page: Apply .pf-m-expanded / .pf-m-text-expanded on dock container
PageDockedDemo->>Nav: Pass isTextExpanded
Nav->>Nav: Apply textExpanded modifier to nav
PageDockedDemo->>Button: Render Buttons with isDock / isTextExpanded
Button->>Button: Apply dock/textExpanded modifiers
PageDockedDemo->>MenuToggle: Render MenuToggles with isDock / isTextExpanded
MenuToggle->>MenuToggle: Apply dock/textExpanded modifiers
PageDockedDemo->>User: UI updates (dock expanded/collapsed, text shown/hidden)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
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-12327.surge.sh A11y report: https://pf-react-pr-12327-a11y.surge.sh |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/react-core/src/demos/examples/Page/PageDockedNav.tsx (1)
427-427: Remove unnecessary fragment wrapper.The
Pagecomponent is the only child element, so the wrapping fragment (<>...</>) is unnecessary.♻️ Proposed fix
return ( - <> - <Page + <Page variant="docked" isDockExpanded={isDockExpanded} ... - </Page> - </> + </Page> );Also applies to: 460-461
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-core/src/demos/examples/Page/PageDockedNav.tsx` at line 427, The fragment wrapper around the single child Page component is unnecessary; remove the surrounding <>...</> so that Page is returned/rendered directly. Update both locations where a fragment encloses Page in PageDockedNav.tsx (the JSX that currently wraps <Page ... />) to eliminate the fragment and leave just the Page element, ensuring any props or surrounding JSX remain intact.packages/react-core/src/components/Page/Page.tsx (1)
31-31: Add JSDoc description and@betatag fordockedMastheadprop.The
dockedMastheadprop is missing both a JSDoc description and the@betatag, unlike the other new dock-related props (isDockExpanded,isDockTextExpanded).📝 Proposed fix
- dockedMasthead?: React.ReactNode; + /** `@beta` Docked masthead component rendered inside the dock container. Only applies when variant is docked. */ + dockedMasthead?: React.ReactNode;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-core/src/components/Page/Page.tsx` at line 31, The prop declaration dockedMasthead?: React.ReactNode is missing a JSDoc comment and the `@beta` tag; add a short JSDoc description explaining it renders the masthead when the dock is anchored (matching style of isDockExpanded/isDockTextExpanded) and prepend `@beta` so it appears in generated docs, ensuring formatting/comments follow the existing JSDoc conventions used in Page.tsx.
🤖 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/Page/Page.tsx`:
- Line 31: The prop declaration dockedMasthead?: React.ReactNode is missing a
JSDoc comment and the `@beta` tag; add a short JSDoc description explaining it
renders the masthead when the dock is anchored (matching style of
isDockExpanded/isDockTextExpanded) and prepend `@beta` so it appears in generated
docs, ensuring formatting/comments follow the existing JSDoc conventions used in
Page.tsx.
In `@packages/react-core/src/demos/examples/Page/PageDockedNav.tsx`:
- Line 427: The fragment wrapper around the single child Page component is
unnecessary; remove the surrounding <>...</> so that Page is returned/rendered
directly. Update both locations where a fragment encloses Page in
PageDockedNav.tsx (the JSX that currently wraps <Page ... />) to eliminate the
fragment and leave just the Page element, ensuring any props or surrounding JSX
remain intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d29e4335-40ec-48f1-a7ba-77f905c7e5a5
📒 Files selected for processing (7)
packages/react-core/src/components/Button/Button.tsxpackages/react-core/src/components/Masthead/MastheadLogo.tsxpackages/react-core/src/components/MenuToggle/MenuToggle.tsxpackages/react-core/src/components/Nav/Nav.tsxpackages/react-core/src/components/Page/Page.tsxpackages/react-core/src/demos/Page.mdpackages/react-core/src/demos/examples/Page/PageDockedNav.tsx
thatblindgeye
commented
Apr 9, 2026
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/react-core/src/components/Page/Page.tsx (1)
25-32: Usedockedconsistently in new prop descriptions.The new JSDoc uses “dock”/“variant of dock” while the actual variant literal is
'docked'. Tightening this wording will avoid ambiguity in API docs.Suggested wording update
- /** `@beta` Flag indicating the dock nav is expanded on mobile. Only applies when variant is dock. */ + /** `@beta` Flag indicating the dock nav is expanded on mobile. Only applies when variant is docked. */ isDockExpanded?: boolean; - /** `@beta` Flag indicating the dock nav should display text. Only applies when variant is dock. */ + /** `@beta` Flag indicating the dock nav should display text. Only applies when variant is docked. */ isDockTextExpanded?: boolean; @@ - /** `@beta` Content to render in the vertical dock when variant of dock is used. At mobile viewports, this content will be replaced with the content passed to masthead. */ + /** `@beta` Content to render in the vertical dock when variant="docked" is used. At mobile viewports, this content will be replaced with the content passed to masthead. */ dockContent?: React.ReactNode;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-core/src/components/Page/Page.tsx` around lines 25 - 32, Update the JSDoc wording to use the variant literal 'docked' consistently: change occurrences of "dock", "variant is dock", and "variant of dock" in the prop comments for isDockExpanded, isDockTextExpanded, masthead, and dockContent to refer to "'docked' variant" or "variant 'docked'"; keep intent the same (mobile behavior and when the text/masthead/dock content applies) but replace ambiguous "dock" wording with the exact literal 'docked' everywhere.
🤖 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/Button/__tests__/Button.test.tsx`:
- Around line 569-577: Tests assert the styles.modifiers.textExpanded class for
isTextExpanded without setting dock context; update the two tests in
Button.test.tsx to reflect dock-only behavior by passing isDock={true} when
expecting the class (render(<Button isDock isTextExpanded>...)) and keeping a
test that omits isDock to assert the class is not present (render(<Button
isTextExpanded>...) expect.not.toHaveClass(styles.modifiers.textExpanded)),
referencing the Button component and props isTextExpanded and isDock and the
styles.modifiers.textExpanded symbol so the assertions match the documented
dock-only contract.
In `@packages/react-core/src/components/Nav/__tests__/Nav.test.tsx`:
- Around line 278-306: The tests for isTextExpanded are asserting docked-only
behavior but render a non-docked Nav; update both tests to render the Nav in the
docked variant context so they validate the correct contract—e.g. in the two
tests that call renderNav and render a <Nav ...> component, include the docked
variant (pass variant="docked" or the equivalent docked prop your Nav API
accepts) when constructing the Nav (the tests referencing Nav, isTextExpanded,
renderNav, and styles.modifiers.textExpanded) so the assertions run against a
docked Nav.
In `@packages/react-core/src/components/Page/__tests__/Page.test.tsx`:
- Around line 395-411: In the two tests "Does not render with dock classes when
variant is default" and "Does not render with dock classes when variant is not
passed" update the DOM queries that check for absence of the dock wrappers:
change the querySelector calls that use styles.pageDock and styles.pageDockMain
to use class selectors (prefix with '.') so they actually select by class (e.g.
querySelector('.' + styles.pageDock) and querySelector('.' +
styles.pageDockMain')); these are the assertions after obtaining the Page
element via screen.getByTestId('page') and should be adjusted in the
Page.test.tsx tests referencing styles.pageDock and styles.pageDockMain.
---
Nitpick comments:
In `@packages/react-core/src/components/Page/Page.tsx`:
- Around line 25-32: Update the JSDoc wording to use the variant literal
'docked' consistently: change occurrences of "dock", "variant is dock", and
"variant of dock" in the prop comments for isDockExpanded, isDockTextExpanded,
masthead, and dockContent to refer to "'docked' variant" or "variant 'docked'";
keep intent the same (mobile behavior and when the text/masthead/dock content
applies) but replace ambiguous "dock" wording with the exact literal 'docked'
everywhere.
🪄 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: 5d607a3f-deed-4941-b2e3-4f46cf904336
⛔ Files ignored due to path filters (1)
packages/react-core/src/components/Button/__tests__/__snapshots__/Button.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (6)
packages/react-core/src/components/Button/__tests__/Button.test.tsxpackages/react-core/src/components/Masthead/__tests__/Masthead.test.tsxpackages/react-core/src/components/MenuToggle/__tests__/MenuToggle.test.tsxpackages/react-core/src/components/Nav/__tests__/Nav.test.tsxpackages/react-core/src/components/Page/Page.tsxpackages/react-core/src/components/Page/__tests__/Page.test.tsx
✅ Files skipped from review due to trivial changes (1)
- packages/react-core/src/components/MenuToggle/tests/MenuToggle.test.tsx
Comment on lines
+569
to
+577
| test(`Renders with class ${styles.modifiers.textExpanded} when isTextExpanded = true`, () => { | ||
| render(<Button isTextExpanded>Text Expanded Button</Button>); | ||
| expect(screen.getByRole('button')).toHaveClass(styles.modifiers.textExpanded); | ||
| }); | ||
|
|
||
| test(`Does not render with class ${styles.modifiers.textExpanded} when isTextExpanded is not passed`, () => { | ||
| render(<Button>Button</Button>); | ||
| expect(screen.getByRole('button')).not.toHaveClass(styles.modifiers.textExpanded); | ||
| }); |
There was a problem hiding this comment.
Align isTextExpanded assertions with dock-only usage.
This case currently verifies isTextExpanded without isDock, which drifts from the documented dock-only behavior and can lock in an unintended contract.
Suggested test adjustment
- test(`Renders with class ${styles.modifiers.textExpanded} when isTextExpanded = true`, () => {
- render(<Button isTextExpanded>Text Expanded Button</Button>);
+ test(`Renders with class ${styles.modifiers.textExpanded} when isDock and isTextExpanded are true`, () => {
+ render(<Button isDock isTextExpanded>Text Expanded Button</Button>);
expect(screen.getByRole('button')).toHaveClass(styles.modifiers.textExpanded);
});
- test(`Does not render with class ${styles.modifiers.textExpanded} when isTextExpanded is not passed`, () => {
- render(<Button>Button</Button>);
+ test(`Does not render with class ${styles.modifiers.textExpanded} when isDock is true and isTextExpanded is not passed`, () => {
+ render(<Button isDock>Button</Button>);
expect(screen.getByRole('button')).not.toHaveClass(styles.modifiers.textExpanded);
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/react-core/src/components/Button/__tests__/Button.test.tsx` around
lines 569 - 577, Tests assert the styles.modifiers.textExpanded class for
isTextExpanded without setting dock context; update the two tests in
Button.test.tsx to reflect dock-only behavior by passing isDock={true} when
expecting the class (render(<Button isDock isTextExpanded>...)) and keeping a
test that omits isDock to assert the class is not present (render(<Button
isTextExpanded>...) expect.not.toHaveClass(styles.modifiers.textExpanded)),
referencing the Button component and props isTextExpanded and isDock and the
styles.modifiers.textExpanded symbol so the assertions match the documented
dock-only contract.
Contributor
Author
There was a problem hiding this comment.
@mcoker is the expectation that we only allow consumers to render the pf-m-text-expanded class only for dock variant buttons/nav/etc? Right now the various components that have both a pf-m-dock and pf-m-text-expanded class, either modifier can be applied independently (you don't need isDock in order to render the class from isTextExpanded)
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
Comment on lines
+278
to
+306
| test(`Renders with ${styles.modifiers.textExpanded} class when isTextExpanded is true`, () => { | ||
| renderNav( | ||
| <Nav isTextExpanded data-testid="text-expanded-nav"> | ||
| <NavList> | ||
| {props.items.map((item) => ( | ||
| <NavItem to={item.to} key={item.to}> | ||
| {item.label} | ||
| </NavItem> | ||
| ))} | ||
| </NavList> | ||
| </Nav> | ||
| ); | ||
| expect(screen.getByTestId('text-expanded-nav')).toHaveClass(styles.modifiers.textExpanded); | ||
| }); | ||
|
|
||
| test(`Does not render with ${styles.modifiers.textExpanded} class when isTextExpanded is not passed`, () => { | ||
| renderNav( | ||
| <Nav data-testid="nav"> | ||
| <NavList> | ||
| {props.items.map((item) => ( | ||
| <NavItem to={item.to} key={item.to}> | ||
| {item.label} | ||
| </NavItem> | ||
| ))} | ||
| </NavList> | ||
| </Nav> | ||
| ); | ||
| expect(screen.getByTestId('nav')).not.toHaveClass(styles.modifiers.textExpanded); | ||
| }); |
There was a problem hiding this comment.
isTextExpanded tests should run in docked variant context.
These tests currently validate isTextExpanded on a non-docked Nav, which conflicts with the docked-only contract and risks cementing unsupported behavior.
Suggested test correction
- test(`Renders with ${styles.modifiers.textExpanded} class when isTextExpanded is true`, () => {
+ test(`Renders with ${styles.modifiers.textExpanded} class when variant is docked and isTextExpanded is true`, () => {
renderNav(
- <Nav isTextExpanded data-testid="text-expanded-nav">
+ <Nav variant="docked" isTextExpanded data-testid="text-expanded-nav">
<NavList>
{props.items.map((item) => (
<NavItem to={item.to} key={item.to}>
{item.label}
</NavItem>
))}
</NavList>
</Nav>
);
expect(screen.getByTestId('text-expanded-nav')).toHaveClass(styles.modifiers.textExpanded);
});
- test(`Does not render with ${styles.modifiers.textExpanded} class when isTextExpanded is not passed`, () => {
+ test(`Does not render with ${styles.modifiers.textExpanded} class when variant is docked and isTextExpanded is not passed`, () => {
renderNav(
- <Nav data-testid="nav">
+ <Nav variant="docked" data-testid="nav">
<NavList>
{props.items.map((item) => (
<NavItem to={item.to} key={item.to}>
{item.label}
</NavItem>
))}
</NavList>
</Nav>
);
expect(screen.getByTestId('nav')).not.toHaveClass(styles.modifiers.textExpanded);
});📝 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
| test(`Renders with ${styles.modifiers.textExpanded} class when isTextExpanded is true`, () => { | |
| renderNav( | |
| <Nav isTextExpanded data-testid="text-expanded-nav"> | |
| <NavList> | |
| {props.items.map((item) => ( | |
| <NavItem to={item.to} key={item.to}> | |
| {item.label} | |
| </NavItem> | |
| ))} | |
| </NavList> | |
| </Nav> | |
| ); | |
| expect(screen.getByTestId('text-expanded-nav')).toHaveClass(styles.modifiers.textExpanded); | |
| }); | |
| test(`Does not render with ${styles.modifiers.textExpanded} class when isTextExpanded is not passed`, () => { | |
| renderNav( | |
| <Nav data-testid="nav"> | |
| <NavList> | |
| {props.items.map((item) => ( | |
| <NavItem to={item.to} key={item.to}> | |
| {item.label} | |
| </NavItem> | |
| ))} | |
| </NavList> | |
| </Nav> | |
| ); | |
| expect(screen.getByTestId('nav')).not.toHaveClass(styles.modifiers.textExpanded); | |
| }); | |
| test(`Renders with ${styles.modifiers.textExpanded} class when variant is docked and isTextExpanded is true`, () => { | |
| renderNav( | |
| <Nav variant="docked" isTextExpanded data-testid="text-expanded-nav"> | |
| <NavList> | |
| {props.items.map((item) => ( | |
| <NavItem to={item.to} key={item.to}> | |
| {item.label} | |
| </NavItem> | |
| ))} | |
| </NavList> | |
| </Nav> | |
| ); | |
| expect(screen.getByTestId('text-expanded-nav')).toHaveClass(styles.modifiers.textExpanded); | |
| }); | |
| test(`Does not render with ${styles.modifiers.textExpanded} class when variant is docked and isTextExpanded is not passed`, () => { | |
| renderNav( | |
| <Nav variant="docked" data-testid="nav"> | |
| <NavList> | |
| {props.items.map((item) => ( | |
| <NavItem to={item.to} key={item.to}> | |
| {item.label} | |
| </NavItem> | |
| ))} | |
| </NavList> | |
| </Nav> | |
| ); | |
| expect(screen.getByTestId('nav')).not.toHaveClass(styles.modifiers.textExpanded); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/react-core/src/components/Nav/__tests__/Nav.test.tsx` around lines
278 - 306, The tests for isTextExpanded are asserting docked-only behavior but
render a non-docked Nav; update both tests to render the Nav in the docked
variant context so they validate the correct contract—e.g. in the two tests that
call renderNav and render a <Nav ...> component, include the docked variant
(pass variant="docked" or the equivalent docked prop your Nav API accepts) when
constructing the Nav (the tests referencing Nav, isTextExpanded, renderNav, and
styles.modifiers.textExpanded) so the assertions run against a docked Nav.
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 #12195
Some notes:
Additional issues:
Summary by CodeRabbit
New Features
Documentation
Tests