feat: add dock nav responsive #8149
Merged
Merged
Conversation
Contributor
WalkthroughAdds docked/navigation layout and text-expanded modifiers across multiple components, introduces an Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Viewport
participant Masthead
participant DockNav as DockedNav
participant Page
Note over Viewport: Initial (mobile)
User->>DockNav: Tap toggle
DockNav->>DockNav: set `.pf-m-expanded`
DockNav->>DockNav: set `.pf-m-text-expanded` (show text)
DockNav->>Page: overlay content (slide in)
DockNav-->>Masthead: update visibility (hide/show logo/toolbar)
Note over Viewport: Resize to desktop
Viewport->>DockNav: width >= dock-expand
DockNav->>DockNav: revert translate, width -> fit-content
DockNav->>Page: integrate into grid (static dock)
DockNav-->>Masthead: masthead main becomes inline/centered
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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-pr-8149.surge.sh A11y report: https://pf-pr-8149-a11y.surge.sh |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/patternfly/demos/Nav/examples/Nav.md (1)
46-52: 🧹 Nitpick | 🔵 TrivialConsider adding both collapsed and expanded dock demo variants.
The Docked nav example now always renders with
page-dock--IsExpanded=true. Consider adding a separate example showing the collapsed (default) state so that both responsive states are documented.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/patternfly/demos/Nav/examples/Nav.md` around lines 46 - 52, The docked nav demo only shows the expanded state because page-dock--IsExpanded=true is set; add a second demo variant that demonstrates the collapsed/default state by creating a new example block (e.g., page-template--id="nav-docked-collapsed-example") that either omits page-dock--IsExpanded or sets page-dock--IsExpanded=false, and keep masthead-template--HasDockedNav and page-template--HasNoSidebar the same so both collapsed and expanded docked nav states (referencing page-dock--IsExpanded, page-template--id, and masthead-template--HasDockedNav) are documented.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/helpers.mjs`:
- Around line 74-87: The helper function ifAll uses var and a verbose if/else
and has 6-space indentation on lines inside the function; update it to follow
the ifAny style by using const for args and options, ensure 4-space indentation
throughout the function body, and replace the if/else with a single return using
a conditional expression that returns options.fn(this) when args.every(Boolean)
is true or options.inverse(this) otherwise (refer to the ifAll function and the
args/options variables to locate the code).
In `@src/patternfly/components/Button/button-text.hbs`:
- Around line 5-9: Add a short in-file comment above the conditional in the
button-text partial documenting the precedence contract: when context contains
button--text its value is used and any `@partial-block` passed by callers will be
silently discarded; callers should not expect block content to be used if
button--text is present (mention that button.hbs relies on this behavior).
Reference the partial name (button-text), the context key (button--text), and
the Handlebars block slot (`@partial-block`) so future maintainers understand the
intentional priority.
In `@src/patternfly/components/Button/button.scss`:
- Around line 929-933: The properties in the .#{$button}__icon selector are out
of the required order; move the min-width declaration (min-width: 1lh) to appear
before the color declaration to satisfy the stylelint order/properties-order
rule. Update the block for .#{$button}__icon so margin-inline-start,
margin-inline-end, min-width come before color (keep the same values and the CSS
variable usage intact).
In `@src/patternfly/components/Masthead/masthead.scss`:
- Around line 168-179: The CSS property order in the masthead rules violates the
Stylelint order: in .#{$masthead}__main move row-gap before align-items, and in
.#{$masthead}__content move align-items before align-self so properties appear
in the expected order; update the declarations inside the .#{$masthead}__main
and .#{$masthead}__content blocks accordingly to satisfy order/properties-order.
In `@src/patternfly/components/MenuToggle/examples/MenuToggle.md`:
- Line 1056: The Usage table in MenuToggle.md is missing an entry for the new
modifier class pf-m-dock; add a row documenting `.pf-m-dock` under the Usage
table (next to the existing `.pf-m-text-expanded` entry) describing that it
applies to `.pf-v6-c-menu-toggle` to modify the toggle into dock mode. Update
the Usage table in the MenuToggle.md file so the new `.pf-m-dock` modifier is
listed and briefly describes its effect, and ensure this corresponds to the
changes already made in menu-toggle.scss and menu-toggle.hbs.
In `@src/patternfly/components/MenuToggle/menu-toggle.scss`:
- Around line 445-452: The pf-m-text-expanded modifier should defensively reset
alignment: inside the .pf-m-text-expanded rule (the block that sets width: 100%
and reveals .#{$menu-toggle}__text and .#{$menu-toggle}__controls) add
justify-content: flex-start so that revealed text is left-aligned even if
.pf-m-dock (at breakpoints) sets justify-content: center; update the
.pf-m-text-expanded selector in menu-toggle.scss to include this property to
make the modifier self-contained.
In `@src/patternfly/components/Nav/nav.scss`:
- Around line 203-207: Replace the brittle display: revert in the
.pf-m-text-expanded rule for .#{$nav}__link-text with an explicit display value
(e.g., display: inline or display: inline-block) that matches the intended
layout for .nav__link-text, and move this rule inside the existing
dock-breakpoint `@media` block so it only applies at the intended viewport; if you
are using cascade layers and want cascade-aware rollback, use display:
revert-layer instead of revert.
In `@src/patternfly/components/Page/page.scss`:
- Around line 269-283: Add a brief clarifying comment above the reopened
.#{$page} block that explains this section exists solely to scope masthead
grid-template overrides for both default and docked contexts (targets >
.#{$masthead} and the &.pf-m-dock > .#{$masthead} rule), so future readers
understand why .#{$page} is reopened despite an earlier .#{$page} block;
reference the masthead variables used here (e.g.,
--#{$masthead}--m-display-inline--GridTemplateColumns and
--#{$page}--m-dock--c-masthead--m-display-inline--GridTemplateColumns) in the
comment for clarity.
- Around line 288-303: Reorder the CSS properties to satisfy stylelint: in the
dock block (selector using translate: -100% 0 and transition: translate ...)
move the transition declaration (transition: translate
var(--#{$page}__dock--TransitionDuration--slide) ...) so it appears before
translate: -100% 0; and inside the .pf-m-expanded rule move border-inline-end:
var(--#{$page}__dock--BorderInlineEndWidth) solid
var(--#{$page}__dock--BorderInlineEndColor); to appear before translate: 0; so
that transition precedes translate in the base rule and border-inline-end
precedes translate in .pf-m-expanded (selectors: the dock block and
.pf-m-expanded; properties: transition, translate, border-inline-end).
In `@src/patternfly/demos/Masthead/masthead-template-content-toolbar-content.hbs`:
- Around line 66-76: The template mixes string and boolean types for the IsPlain
prop; update the menu-toggle instance that uses menu-toggle--IsPlain="true" to
use a boolean (menu-toggle--IsPlain=true) to match the other occurrences (e.g.,
the earlier menu-toggle with menu-toggle--IsPlain=true and button--IsPlain=true)
so all IsPlain props are consistently booleans in the Masthead template.
In `@src/patternfly/demos/Page/page-template.hbs`:
- Around line 8-29: Remove the redundant nested Handlebars conditional for
masthead-template--HasDockedNav: delete the inner opening "{{`#if`
masthead-template--HasDockedNav}}" and its matching "{{/if}}" so the single
outer conditional governs the entire masthead block (which includes the {{>
masthead}} / {{#> masthead-main}} / {{#> masthead-content}} sections and their
children such as {{> masthead-toggle}} and the toolbar). Ensure the remaining
blocks remain properly balanced and indentation is adjusted accordingly.
---
Outside diff comments:
In `@src/patternfly/demos/Nav/examples/Nav.md`:
- Around line 46-52: The docked nav demo only shows the expanded state because
page-dock--IsExpanded=true is set; add a second demo variant that demonstrates
the collapsed/default state by creating a new example block (e.g.,
page-template--id="nav-docked-collapsed-example") that either omits
page-dock--IsExpanded or sets page-dock--IsExpanded=false, and keep
masthead-template--HasDockedNav and page-template--HasNoSidebar the same so both
collapsed and expanded docked nav states (referencing page-dock--IsExpanded,
page-template--id, and masthead-template--HasDockedNav) are documented.
Comment on lines
+74
to
+87
| export const ifAll = function() { | ||
| // Convert arguments to an array, excluding the last 'options' object | ||
| var args = Array.prototype.slice.call(arguments, 0, arguments.length - 1); | ||
| var options = arguments[arguments.length - 1]; | ||
|
|
||
| // Check if every argument is truthy | ||
| var allTruthy = args.every(Boolean); | ||
|
|
||
| if (allTruthy) { | ||
| return options.fn(this); | ||
| } else { | ||
| return options.inverse(this); | ||
| } | ||
| }; |
Contributor
There was a problem hiding this comment.
Fix ESLint indentation errors and align style with ifAny.
Lines 83 and 85 have 6-space indentation where the ESLint indent rule expects 4. Additionally, using var instead of const and a verbose if/else block is inconsistent with the adjacent ifAny implementation style.
🔧 Proposed fix
-export const ifAll = function() {
- // Convert arguments to an array, excluding the last 'options' object
- var args = Array.prototype.slice.call(arguments, 0, arguments.length - 1);
- var options = arguments[arguments.length - 1];
-
- // Check if every argument is truthy
- var allTruthy = args.every(Boolean);
-
- if (allTruthy) {
- return options.fn(this);
- } else {
- return options.inverse(this);
- }
-};
+export const ifAll = function() {
+ const args = Array.prototype.slice.call(arguments, 0, -1);
+ const options = arguments[arguments.length - 1];
+ const allTruthy = args.every(Boolean);
+
+ return allTruthy ? options.fn(this) : options.inverse(this);
+};📝 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
| export const ifAll = function() { | |
| // Convert arguments to an array, excluding the last 'options' object | |
| var args = Array.prototype.slice.call(arguments, 0, arguments.length - 1); | |
| var options = arguments[arguments.length - 1]; | |
| // Check if every argument is truthy | |
| var allTruthy = args.every(Boolean); | |
| if (allTruthy) { | |
| return options.fn(this); | |
| } else { | |
| return options.inverse(this); | |
| } | |
| }; | |
| export const ifAll = function() { | |
| const args = Array.prototype.slice.call(arguments, 0, -1); | |
| const options = arguments[arguments.length - 1]; | |
| const allTruthy = args.every(Boolean); | |
| return allTruthy ? options.fn(this) : options.inverse(this); | |
| }; |
🧰 Tools
🪛 ESLint
[error] 83-83: Expected indentation of 4 spaces but found 6.
(indent)
[error] 85-85: Expected indentation of 4 spaces but found 6.
(indent)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/helpers.mjs` around lines 74 - 87, The helper function ifAll uses var
and a verbose if/else and has 6-space indentation on lines inside the function;
update it to follow the ifAny style by using const for args and options, ensure
4-space indentation throughout the function body, and replace the if/else with a
single return using a conditional expression that returns options.fn(this) when
args.every(Boolean) is true or options.inverse(this) otherwise (refer to the
ifAll function and the args/options variables to locate the code).
Comment on lines
+5
to
+9
| {{#if button--text}} | ||
| {{button--text}} | ||
| {{else if @partial-block}} | ||
| {{> @partial-block}} | ||
| {{/if}} |
Contributor
There was a problem hiding this comment.
LGTM — note the silent-discard contract for external callers.
The button--text → @partial-block → nothing priority chain is logically correct and consistent with how button.hbs invokes this partial (it calls {{> button-text}} without a block only when button--text is set, and {{#> button-text}}…{{/button-text}} only when button--text is absent, so the two paths are mutually exclusive from that caller).
However, any other template that invokes this partial as {{#> button-text}}…{{/button-text}} while button--text is also in context will silently lose the partial-block content. Consider adding a short comment here (or in the template docs) to document this precedence contract so future callers are not surprised.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/patternfly/components/Button/button-text.hbs` around lines 5 - 9, Add a
short in-file comment above the conditional in the button-text partial
documenting the precedence contract: when context contains button--text its
value is used and any `@partial-block` passed by callers will be silently
discarded; callers should not expect block content to be used if button--text is
present (mention that button.hbs relies on this behavior). Reference the partial
name (button-text), the context key (button--text), and the Handlebars block
slot (`@partial-block`) so future maintainers understand the intentional priority.
| | `.pf-m-warning` | `.pf-v6-c-menu-toggle` | Modifies the menu toggle component for the warning state. | | ||
| | `.pf-m-danger` | `.pf-v6-c-menu-toggle` | Modifies the menu toggle component for the danger state. | | ||
| | `.pf-m-placeholder` | `.pf-v6-c-menu-toggle` | Modifies the menu toggle text for placeholder styles. | | ||
| | `.pf-m-text-expanded` | `.pf-v6-c-menu-toggle` | Modifies a dock menu toggle to show its text. | |
Contributor
There was a problem hiding this comment.
Add documentation row for pf-m-dock in the Usage table.
pf-m-dock was added to menu-toggle.scss and menu-toggle.hbs in this PR but has no corresponding entry in the Usage table, while the paired pf-m-text-expanded is documented. Add an entry:
+| `.pf-m-dock` | `.pf-v6-c-menu-toggle` | Modifies the menu toggle for a docked layout, collapsing text/controls at larger breakpoints. |
| `.pf-m-text-expanded` | `.pf-v6-c-menu-toggle` | Modifies a dock menu toggle to show its text. |📝 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
| | `.pf-m-text-expanded` | `.pf-v6-c-menu-toggle` | Modifies a dock menu toggle to show its text. | | |
| | `.pf-m-dock` | `.pf-v6-c-menu-toggle` | Modifies the menu toggle for a docked layout, collapsing text/controls at larger breakpoints. | | |
| | `.pf-m-text-expanded` | `.pf-v6-c-menu-toggle` | Modifies a dock menu toggle to show its text. | |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/patternfly/components/MenuToggle/examples/MenuToggle.md` at line 1056,
The Usage table in MenuToggle.md is missing an entry for the new modifier class
pf-m-dock; add a row documenting `.pf-m-dock` under the Usage table (next to the
existing `.pf-m-text-expanded` entry) describing that it applies to
`.pf-v6-c-menu-toggle` to modify the toggle into dock mode. Update the Usage
table in the MenuToggle.md file so the new `.pf-m-dock` modifier is listed and
briefly describes its effect, and ensure this corresponds to the changes already
made in menu-toggle.scss and menu-toggle.hbs.
Comment on lines
+203
to
+207
| &.pf-m-text-expanded { | ||
| .#{$nav}__link-text { | ||
| display: revert; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider an explicit display value instead of display: revert.
display: revert rolls back to the UA stylesheet's default, which is element-type-dependent (e.g., inline for <span>, block for <div>). If the markup for .nav__link-text ever changes element type, this silently produces a different layout. An explicit value (or display: revert-layer if cascade layers are in use) is more self-documenting and resilient to future refactoring.
Additionally, this rule is not scoped to the breakpoint media query, so it fires at all viewport widths even though there is no display: none to override below the dock breakpoint—harmless, but consider placing it inside the @media block for clearer intent.
♻️ Suggested refactor
- &.pf-m-text-expanded {
- .#{$nav}__link-text {
- display: revert;
- }
- }
-
.#{$nav}__link-icon { ... }
`@media` (min-width: #{$pf-v6-global--breakpoint--dock--expand}) {
width: fit-content;
.#{$nav}__link-text {
display: none;
}
+
+ &.pf-m-text-expanded {
+ .#{$nav}__link-text {
+ display: inline; // or the appropriate explicit value for the element
+ }
+ }
}📝 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
| &.pf-m-text-expanded { | |
| .#{$nav}__link-text { | |
| display: revert; | |
| } | |
| } | |
| &.pf-m-text-expanded { | |
| .#{$nav}__link-text { | |
| display: revert; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/patternfly/components/Nav/nav.scss` around lines 203 - 207, Replace the
brittle display: revert in the .pf-m-text-expanded rule for .#{$nav}__link-text
with an explicit display value (e.g., display: inline or display: inline-block)
that matches the intended layout for .nav__link-text, and move this rule inside
the existing dock-breakpoint `@media` block so it only applies at the intended
viewport; if you are using cascade layers and want cascade-aware rollback, use
display: revert-layer instead of revert.
Comment on lines
+66
to
+76
| {{#> toolbar-group toolbar-group--IsActionGroupPlain=true toolbar-group--id=(concat masthead-template--id '-icon-group-') toolbar-group--modifier="pf-m-align-end pf-m-spacer-none pf-m-spacer-md-on-md"}} | ||
| {{#> toolbar-item}} | ||
| {{> menu-toggle menu-toggle--IsPlain=true menu-toggle--icon="th" menu-toggle--aria-label="Applications" menu-toggle--text="Applications" menu-toggle--IsDock=true menu-toggle--IsText=true}} | ||
| {{/toolbar-item}} | ||
| {{#> toolbar-item}} | ||
| {{> button button--IsDock=true button--IsPlain=true button--IsSettings=true button--icon="cog" button--aria-label="Settings" button--text="Settings"}} | ||
| {{/toolbar-item}} | ||
| {{#> toolbar-item}} | ||
| {{> menu-toggle menu-toggle--IsPlain="true" menu-toggle--aria-label="Help" menu-toggle--icon="question-circle" menu-toggle--text="Help" menu-toggle--IsDock=true menu-toggle--IsText=true}} | ||
| {{/toolbar-item}} | ||
| {{/toolbar-group}} |
Contributor
There was a problem hiding this comment.
Inconsistent IsPlain value types: string vs boolean.
Line 68 uses menu-toggle--IsPlain=true (boolean), while Line 74 uses menu-toggle--IsPlain="true" (string). Although both are truthy in Handlebars, mixing types is inconsistent and may cause subtle issues if any helper performs strict type checks.
Proposed fix
- {{> menu-toggle menu-toggle--IsPlain="true" menu-toggle--aria-label="Help" menu-toggle--icon="question-circle" menu-toggle--text="Help" menu-toggle--IsDock=true menu-toggle--IsText=true}}
+ {{> menu-toggle menu-toggle--IsPlain=true menu-toggle--aria-label="Help" menu-toggle--icon="question-circle" menu-toggle--text="Help" menu-toggle--IsDock=true menu-toggle--IsText=true}}📝 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
| {{#> toolbar-group toolbar-group--IsActionGroupPlain=true toolbar-group--id=(concat masthead-template--id '-icon-group-') toolbar-group--modifier="pf-m-align-end pf-m-spacer-none pf-m-spacer-md-on-md"}} | |
| {{#> toolbar-item}} | |
| {{> menu-toggle menu-toggle--IsPlain=true menu-toggle--icon="th" menu-toggle--aria-label="Applications" menu-toggle--text="Applications" menu-toggle--IsDock=true menu-toggle--IsText=true}} | |
| {{/toolbar-item}} | |
| {{#> toolbar-item}} | |
| {{> button button--IsDock=true button--IsPlain=true button--IsSettings=true button--icon="cog" button--aria-label="Settings" button--text="Settings"}} | |
| {{/toolbar-item}} | |
| {{#> toolbar-item}} | |
| {{> menu-toggle menu-toggle--IsPlain="true" menu-toggle--aria-label="Help" menu-toggle--icon="question-circle" menu-toggle--text="Help" menu-toggle--IsDock=true menu-toggle--IsText=true}} | |
| {{/toolbar-item}} | |
| {{/toolbar-group}} | |
| {{#> toolbar-group toolbar-group--IsActionGroupPlain=true toolbar-group--id=(concat masthead-template--id '-icon-group-') toolbar-group--modifier="pf-m-align-end pf-m-spacer-none pf-m-spacer-md-on-md"}} | |
| {{#> toolbar-item}} | |
| {{> menu-toggle menu-toggle--IsPlain=true menu-toggle--icon="th" menu-toggle--aria-label="Applications" menu-toggle--text="Applications" menu-toggle--IsDock=true menu-toggle--IsText=true}} | |
| {{/toolbar-item}} | |
| {{#> toolbar-item}} | |
| {{> button button--IsDock=true button--IsPlain=true button--IsSettings=true button--icon="cog" button--aria-label="Settings" button--text="Settings"}} | |
| {{/toolbar-item}} | |
| {{#> toolbar-item}} | |
| {{> menu-toggle menu-toggle--IsPlain=true menu-toggle--aria-label="Help" menu-toggle--icon="question-circle" menu-toggle--text="Help" menu-toggle--IsDock=true menu-toggle--IsText=true}} | |
| {{/toolbar-item}} | |
| {{/toolbar-group}} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/patternfly/demos/Masthead/masthead-template-content-toolbar-content.hbs`
around lines 66 - 76, The template mixes string and boolean types for the
IsPlain prop; update the menu-toggle instance that uses
menu-toggle--IsPlain="true" to use a boolean (menu-toggle--IsPlain=true) to
match the other occurrences (e.g., the earlier menu-toggle with
menu-toggle--IsPlain=true and button--IsPlain=true) so all IsPlain props are
consistently booleans in the Masthead template.
Comment on lines
8
to
+29
| {{#if masthead-template--HasDockedNav}} | ||
| {{#if masthead-template--HasDockedNav}} | ||
| {{#> masthead masthead--IsDisplayInline=true}} | ||
| {{#> masthead-main}} | ||
| {{> masthead-toggle}} | ||
| {{#> masthead-brand}} | ||
| {{> masthead-logo}} | ||
| {{/masthead-brand}} | ||
| {{/masthead-main}} | ||
| {{#> masthead-content}} | ||
| {{#> toolbar toolbar--IsStatic=true}} | ||
| {{#> toolbar-content}} | ||
| {{#> toolbar-content-section}} | ||
| {{#> toolbar-item toolbar-item--modifier="pf-m-align-end"}} | ||
| {{> button button--IsPlain=true button--IsIcon=true button--icon="search" button--aria-label="Search"}} | ||
| {{/toolbar-item}} | ||
| {{/toolbar-content-section}} | ||
| {{/toolbar-content}} | ||
| {{/toolbar}} | ||
| {{/masthead-content}} | ||
| {{/masthead}} | ||
| {{/if}} |
Contributor
There was a problem hiding this comment.
Redundant nested {{#if masthead-template--HasDockedNav}} check.
Line 8 already gates this branch on masthead-template--HasDockedNav, making the identical check on Line 9 redundant. This adds unnecessary nesting and likely was an oversight during development.
Proposed fix — remove the inner duplicate conditional
{{`#unless` page-template--HasNoMasthead}}
{{`#if` masthead-template--HasDockedNav}}
- {{`#if` masthead-template--HasDockedNav}}
{{#> masthead masthead--IsDisplayInline=true}}
{{#> masthead-main}}
{{> masthead-toggle}}
@@
{{/masthead-content}}
{{/masthead}}
- {{/if}}
{{#> page-dock}}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/patternfly/demos/Page/page-template.hbs` around lines 8 - 29, Remove the
redundant nested Handlebars conditional for masthead-template--HasDockedNav:
delete the inner opening "{{`#if` masthead-template--HasDockedNav}}" and its
matching "{{/if}}" so the single outer conditional governs the entire masthead
block (which includes the {{> masthead}} / {{#> masthead-main}} / {{#>
masthead-content}} sections and their children such as {{> masthead-toggle}} and
the toolbar). Ensure the remaining blocks remain properly balanced and
indentation is adjusted accordingly.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/patternfly/components/Masthead/masthead.scss`:
- Around line 196-222: The nested &.pf-m-compact rules under .#{$masthead}__logo
create a 4-level nesting violation; refactor by removing the inner ampersand
selectors and replacing them with flattened selectors such as
.#{$masthead}__logo.pf-m-compact and .#{$masthead}__logo:not(.pf-m-compact) at
the same scope as the parent .#{$masthead}__logo inside the `@media` (min-width:
$pf-v6-global--breakpoint--dock--expand) block and inside the `@at-root`
.#{$page}__dock.pf-m-text-expanded & block, leaving .#{$masthead}__main rules
intact so nesting depth stays ≤3.
In `@src/patternfly/components/Nav/nav.scss`:
- Around line 401-404: The CSS rule for .#{$nav}__link-icon has properties out
of the expected order; move the min-width declaration so it appears before color
to satisfy the order/properties-order rule: update the .#{$nav}__link-icon block
in nav.scss to list min-width: 1lh; before color:
var(--#{$nav}__link-icon--Color); keeping text-align: center as-is.
In `@src/patternfly/components/Page/page.scss`:
- Around line 269-283: Add a brief inline comment above the reopened .#{$page}
block explaining why it’s re-opened: clarify that this scope specifically
targets masthead grid-template variable overrides for the default masthead and
the pf-m-dock variant (reference .#{$page}, > .#{$masthead}, and &.pf-m-dock),
so future readers know the separation is intentional for masthead grid-template
scoping across breakpoints and docked state.
---
Duplicate comments:
In `@src/patternfly/components/Button/button.scss`:
- Around line 933-937: Move the min-width declaration so it comes before color
in the .#{$button}__icon rule to satisfy stylelint order; specifically in the
.#{$button}__icon selector reorder the properties so min-width: 1lh; appears
above color: var(--#{$button}__icon--Color); (do not change the property values,
only their order).
In `@src/patternfly/components/Masthead/masthead.scss`:
- Around line 177-188: Reorder the CSS properties to satisfy stylelint
property-order rules: in the .#{$masthead}__main selector move row-gap so it
appears before align-items (i.e., set row-gap then align-items), and in the
.#{$masthead}__content selector move align-items so it appears before align-self
(i.e., set align-items then align-self); update only the property order within
those selectors (.#{$masthead}__main and .#{$masthead}__content) so the existing
values and other rules remain unchanged.
In `@src/patternfly/components/Page/page.scss`:
- Around line 288-303: The property ordering in the .page__dock block and its
&.pf-m-expanded modifier violates stylelint: move the transition declaration
before translate in the .page__dock selector (ensure transition: ... uses the
existing --#{$page}__dock--TransitionDuration--slide variable), and in
&.pf-m-expanded move border-inline-end before translate so border-inline-end is
declared prior to translate; update the declarations inside .page__dock and
&.pf-m-expanded accordingly to satisfy order/properties-order.
Comment on lines
+196
to
+222
| @media (min-width: $pf-v6-global--breakpoint--dock--expand) { | ||
| .#{$masthead}__main { | ||
| align-items: center; | ||
| } | ||
|
|
||
| .#{$masthead}__logo { | ||
| display: none; | ||
|
|
||
| &.pf-m-compact { | ||
| display: revert; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| @at-root .#{$page}__dock.pf-m-text-expanded & { | ||
| .#{$masthead}__logo { | ||
| display: revert; | ||
|
|
||
| &.pf-m-compact { | ||
| display: none; | ||
| } | ||
| } | ||
|
|
||
| .#{$masthead}__main { | ||
| align-items: flex-start; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Nesting depth exceeds the configured maximum of 3.
Lines 204 and 214 trigger max-nesting-depth violations. The &.pf-m-compact selectors inside @media > .masthead__logo and @at-root > .masthead__logo create a 4th nesting level within .pf-m-docked.
To fix, extract the nested selectors. One approach:
🔧 Proposed restructuring sketch
- `@media` (min-width: $pf-v6-global--breakpoint--dock--expand) {
- .#{$masthead}__main {
- align-items: center;
- }
-
- .#{$masthead}__logo {
- display: none;
-
- &.pf-m-compact {
- display: revert;
- }
- }
- }
-
- `@at-root` .#{$page}__dock.pf-m-text-expanded & {
- .#{$masthead}__logo {
- display: revert;
-
- &.pf-m-compact {
- display: none;
- }
- }
-
- .#{$masthead}__main {
- align-items: flex-start;
- }
- }
+ `@media` (min-width: $pf-v6-global--breakpoint--dock--expand) {
+ .#{$masthead}__main {
+ align-items: center;
+ }
+
+ .#{$masthead}__logo:not(.pf-m-compact) {
+ display: none;
+ }
+
+ .#{$masthead}__logo.pf-m-compact {
+ display: revert;
+ }
+ }
+
+ `@at-root` .#{$page}__dock.pf-m-text-expanded & {
+ .#{$masthead}__logo:not(.pf-m-compact) {
+ display: revert;
+ }
+
+ .#{$masthead}__logo.pf-m-compact {
+ display: none;
+ }
+
+ .#{$masthead}__main {
+ align-items: flex-start;
+ }
+ }This flattens the 4th nesting level by replacing & > &.pf-m-compact with separate .masthead__logo.pf-m-compact and .masthead__logo:not(.pf-m-compact) selectors.
📝 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
| @media (min-width: $pf-v6-global--breakpoint--dock--expand) { | |
| .#{$masthead}__main { | |
| align-items: center; | |
| } | |
| .#{$masthead}__logo { | |
| display: none; | |
| &.pf-m-compact { | |
| display: revert; | |
| } | |
| } | |
| } | |
| @at-root .#{$page}__dock.pf-m-text-expanded & { | |
| .#{$masthead}__logo { | |
| display: revert; | |
| &.pf-m-compact { | |
| display: none; | |
| } | |
| } | |
| .#{$masthead}__main { | |
| align-items: flex-start; | |
| } | |
| } | |
| `@media` (min-width: $pf-v6-global--breakpoint--dock--expand) { | |
| .#{$masthead}__main { | |
| align-items: center; | |
| } | |
| .#{$masthead}__logo:not(.pf-m-compact) { | |
| display: none; | |
| } | |
| .#{$masthead}__logo.pf-m-compact { | |
| display: revert; | |
| } | |
| } | |
| `@at-root` .#{$page}__dock.pf-m-text-expanded & { | |
| .#{$masthead}__logo:not(.pf-m-compact) { | |
| display: revert; | |
| } | |
| .#{$masthead}__logo.pf-m-compact { | |
| display: none; | |
| } | |
| .#{$masthead}__main { | |
| align-items: flex-start; | |
| } | |
| } |
🧰 Tools
🪛 GitHub Actions: pr-preview
[error] 204-204: max-nesting-depth: Expected nesting depth to be no more than 3
[error] 214-214: max-nesting-depth: Expected nesting depth to be no more than 3
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/patternfly/components/Masthead/masthead.scss` around lines 196 - 222, The
nested &.pf-m-compact rules under .#{$masthead}__logo create a 4-level nesting
violation; refactor by removing the inner ampersand selectors and replacing them
with flattened selectors such as .#{$masthead}__logo.pf-m-compact and
.#{$masthead}__logo:not(.pf-m-compact) at the same scope as the parent
.#{$masthead}__logo inside the `@media` (min-width:
$pf-v6-global--breakpoint--dock--expand) block and inside the `@at-root`
.#{$page}__dock.pf-m-text-expanded & block, leaving .#{$masthead}__main rules
intact so nesting depth stays ≤3.
Comment on lines
+269
to
+283
| .#{$page} { | ||
| > .#{$masthead} { | ||
| @media (min-width: $pf-v6-global--breakpoint--xl) { | ||
| --#{$masthead}--m-display-inline--GridTemplateColumns: var(--#{$masthead}--m-display-inline--breakpoint--xl--GridTemplateColumns); | ||
| } | ||
|
|
||
| z-index: var(--#{$page}--c-masthead--ZIndex); | ||
| grid-area: header; | ||
| z-index: var(--#{$page}--c-masthead--ZIndex); | ||
| grid-area: header; | ||
| } | ||
|
|
||
| &.pf-m-dock { | ||
| > .#{$masthead} { | ||
| --#{$masthead}--m-display-inline--GridTemplateColumns: var(--#{$page}--m-dock--c-masthead--m-display-inline--GridTemplateColumns); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding a comment to clarify why .#{$page} is re-opened.
This block re-opens the .#{$page} selector (already defined at line 190) specifically for masthead grid-template scoping in both default and docked contexts. A brief comment would help future readers understand the separation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/patternfly/components/Page/page.scss` around lines 269 - 283, Add a brief
inline comment above the reopened .#{$page} block explaining why it’s re-opened:
clarify that this scope specifically targets masthead grid-template variable
overrides for the default masthead and the pf-m-dock variant (reference
.#{$page}, > .#{$masthead}, and &.pf-m-dock), so future readers know the
separation is intentional for masthead grid-template scoping across breakpoints
and docked state.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/patternfly/components/Masthead/masthead.scss`:
- Around line 212-226: This SCSS couples the masthead to the page component by
using `@at-root` with the selector .#{$page}__dock.pf-m-text-expanded to target
.#{$masthead}__logo and .#{$masthead}__main; remove that context-specific
`@at-root` block from masthead.scss and instead move the entire rule into a
layout/composition stylesheet (for example page.scss or a new
masthead-composition.scss) where you can target
.#{$page}__dock.pf-m-text-expanded .#{$masthead}__logo and
.#{$page}__dock.pf-m-text-expanded .#{$masthead}__main, or alternatively expose
a standalone modifier class on the masthead (e.g., .pf-m-docked) and apply that
class from the page-level stylesheet to keep masthead.scss component-local while
letting page.scss control the docked behavior.
- Around line 190-194: The CSS variable override inside the .#{$toolbar} block
uses a hardcoded name (--pf-v6-c-toolbar--m-vertical--Width) instead of the
interpolated namespace; update that line to use the interpolated variable name
(--#{$toolbar}--m-vertical--Width) so it matches the other overrides (see
.#{$toolbar} selector and the height using
--#{$masthead}--m-docked--c-toolbar--Height) and avoids breaking if $toolbar
changes.
In `@src/patternfly/components/Page/page.scss`:
- Around line 314-316: The rule targeting .#{$toolbar}.pf-m-vertical
:is(.#{$toolbar}__content-section, .#{$toolbar}__group, .#{$toolbar}__item)
should be moved out of the Page stylesheet to avoid coupling; remove this block
from src/patternfly/components/Page/page.scss and add it to the Toolbar
component's SCSS (e.g., toolbar.scss) under the .pf-m-vertical context so the
selector is defined as .#{$toolbar}.pf-m-vertical
:is(.#{$toolbar}__content-section, .#{$toolbar}__group, .#{$toolbar}__item) {
align-items: stretch; }, or if this styling only applies when the toolbar lives
inside the page dock, place the rule into a dedicated cross-component override
file gated by .#{$page}__dock to preserve scope.
- Around line 23-24: The CSS custom property --#{$page}__dock--desktop--Width is
declared but never used; either remove it or wire it up by replacing the literal
desktop reset (width: auto) with the token. In practice update the desktop
media-query block where you currently set width: auto to width:
var(--#{$page}__dock--desktop--Width) so the declared custom property is
consumed (or delete the --#{$page}__dock--desktop--Width declaration if you
prefer to keep the literal).
---
Duplicate comments:
In `@src/patternfly/components/Masthead/masthead.scss`:
- Around line 204-208: The nested selector &.pf-m-compact inside .masthead__logo
is creating a 4th-level nesting violation and was only silenced by
stylelint-disable; instead remove the inline stylelint-disable/enable and
refactor by moving the &.pf-m-compact rules out of the deep nesting and into
sibling selectors at the .masthead__logo level (i.e. create a top-level
.masthead__logo.pf-m-compact { ... } or a sibling .masthead__logo.pf-m-compact
rule matching the same styles), update both occurrences (the block around
&.pf-m-compact at ~204-208 and the duplicate at ~216-220), and ensure no extra
nesting remains so the max-nesting-depth lint passes without suppression.
- Around line 177-188: No changes required: the Stylelint property-order
violations have been resolved—ensure .#{$masthead}__main keeps 'row-gap' before
'align-items' and .#{$masthead}__content keeps 'align-items' before 'align-self'
as shown; leave the rules in masthead.scss (selectors .#{$masthead}__main and
.#{$masthead}__content) as-is and proceed to approve the change.
In `@src/patternfly/components/Nav/nav.scss`:
- Around line 211-218: The selector .#{$nav}__link-text currently uses display:
revert (under .#{$page}__dock.pf-m-text-expanded & / &.pf-m-text-expanded) and
is placed outside the breakpoint, which risks inconsistent layouts and fires at
all widths; change display: revert to an explicit value that matches the
intended markup (e.g., display: inline or display: inline-block depending on
.#{$nav}__link-text's intended element) and move this entire .pf-m-text-expanded
restoration block into (or duplicate it inside) the `@media` (min-width:
#{$pf-v6-global--breakpoint--dock--expand}) rule so the show/hide behavior pairs
with the media-query where display: none is set.
In `@src/patternfly/components/Page/page.scss`:
- Around line 269-283: Add a short inline comment above the second .#{$page}
block explaining that this block is intentionally re-opening .#{$page} solely to
scope masthead grid-template variables and to provide the pf-m-dock override for
> .#{$masthead}; reference the specific symbols .#{$page}, > .#{$masthead},
--#{$masthead}--m-display-inline--GridTemplateColumns and the &.pf-m-dock
override so readers understand this separation from the earlier .#{$page}
declaration.
- Around line 295-302: The property-ordering violations have been fixed in
page.scss: keep the current ordering where transition (using
--#{$page}__dock--TransitionDuration--slide) precedes translate, and within the
.pf-m-expanded block the border-inline-end (using
--#{$page}__dock--BorderInlineEndWidth/Color) appears before the translate: 0
declaration; ensure this ordering is preserved for the selector &.pf-m-expanded
and re-run the stylelint/order/properties-order check to verify no regressions.
Comment on lines
190
to
194
| .#{$toolbar} { | ||
| --#{$masthead}--c-toolbar--Width: fit-content; | ||
| --pf-v6-c-toolbar--m-vertical--Width: auto; | ||
|
|
||
| height: var(--#{$masthead}--m-docked--c-toolbar--Height); | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Hardcoded toolbar namespace on line 191 — use --#{$toolbar}--m-vertical--Width for consistency
Every other toolbar variable override in this file uses --#{$toolbar}--* interpolation (lines 114–116, 193). Line 191 hardcodes the pf-v6-c-toolbar prefix, which silently breaks if the $toolbar namespace is ever updated.
♻️ Proposed fix
.#{$toolbar} {
- --pf-v6-c-toolbar--m-vertical--Width: auto;
+ --#{$toolbar}--m-vertical--Width: auto;
height: var(--#{$masthead}--m-docked--c-toolbar--Height);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/patternfly/components/Masthead/masthead.scss` around lines 190 - 194, The
CSS variable override inside the .#{$toolbar} block uses a hardcoded name
(--pf-v6-c-toolbar--m-vertical--Width) instead of the interpolated namespace;
update that line to use the interpolated variable name
(--#{$toolbar}--m-vertical--Width) so it matches the other overrides (see
.#{$toolbar} selector and the height using
--#{$masthead}--m-docked--c-toolbar--Height) and avoids breaking if $toolbar
changes.
Comment on lines
+212
to
+226
| @at-root .#{$page}__dock.pf-m-text-expanded & { | ||
| .#{$masthead}__logo { | ||
| display: revert; | ||
|
|
||
| // stylelint-disable max-nesting-depth | ||
| &.pf-m-compact { | ||
| display: none; | ||
| } | ||
| // stylelint-enable | ||
| } | ||
|
|
||
| .#{$masthead}__main { | ||
| align-items: flex-start; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
@at-root couples masthead.scss to the page component's DOM ancestry
The selector .#{$page}__dock.pf-m-text-expanded bakes a dependency on $page's BEM structure directly into the masthead component stylesheet. This means masthead's visual behavior silently depends on an ancestor from a separate component, breaking component isolation: the masthead cannot be used in a docked context outside page__dock, and any rename/restructure of page__dock would require updating masthead.scss as well.
Consider whether this context-aware styling belongs in a shared layout file (e.g., page.scss) or a dedicated composition stylesheet, rather than in the masthead component itself.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/patternfly/components/Masthead/masthead.scss` around lines 212 - 226,
This SCSS couples the masthead to the page component by using `@at-root` with the
selector .#{$page}__dock.pf-m-text-expanded to target .#{$masthead}__logo and
.#{$masthead}__main; remove that context-specific `@at-root` block from
masthead.scss and instead move the entire rule into a layout/composition
stylesheet (for example page.scss or a new masthead-composition.scss) where you
can target .#{$page}__dock.pf-m-text-expanded .#{$masthead}__logo and
.#{$page}__dock.pf-m-text-expanded .#{$masthead}__main, or alternatively expose
a standalone modifier class on the masthead (e.g., .pf-m-docked) and apply that
class from the page-level stylesheet to keep masthead.scss component-local while
letting page.scss control the docked behavior.
Comment on lines
+23
to
+24
| --#{$page}__dock--Width: #{pf-size-prem(250px)}; | ||
| --#{$page}__dock--desktop--Width: auto; |
Contributor
There was a problem hiding this comment.
--#{$page}__dock--desktop--Width is declared but never consumed.
The token at line 24 is never referenced in the file. The desktop width reset at line 310 uses a literal width: auto instead. Either reference the token there or remove it to keep the public API surface clean.
🧹 Proposed fix
- --#{$page}__dock--desktop--Width: auto;
--#{$page}__dock--ZIndex: var(--pf-t--global--z-index--md);And in the desktop media-query block:
- width: auto;
+ width: var(--#{$page}__dock--desktop--Width);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/patternfly/components/Page/page.scss` around lines 23 - 24, The CSS
custom property --#{$page}__dock--desktop--Width is declared but never used;
either remove it or wire it up by replacing the literal desktop reset (width:
auto) with the token. In practice update the desktop media-query block where you
currently set width: auto to width: var(--#{$page}__dock--desktop--Width) so the
declared custom property is consumed (or delete the
--#{$page}__dock--desktop--Width declaration if you prefer to keep the literal).
Comment on lines
+314
to
+316
| .#{$toolbar}.pf-m-vertical :is(.#{$toolbar}__content-section, .#{$toolbar}__group, .#{$toolbar}__item) { | ||
| align-items: stretch; | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Cross-component Toolbar styling inside the Page Dock selector.
Styling .#{$toolbar} internals from within .#{$page}__dock creates a direct coupling between the Page and Toolbar components. Consider moving this rule to the Toolbar component's own SCSS (gated by a context class such as .pf-m-vertical already being used), or to a dedicated cross-component override file if the project has one.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/patternfly/components/Page/page.scss` around lines 314 - 316, The rule
targeting .#{$toolbar}.pf-m-vertical :is(.#{$toolbar}__content-section,
.#{$toolbar}__group, .#{$toolbar}__item) should be moved out of the Page
stylesheet to avoid coupling; remove this block from
src/patternfly/components/Page/page.scss and add it to the Toolbar component's
SCSS (e.g., toolbar.scss) under the .pf-m-vertical context so the selector is
defined as .#{$toolbar}.pf-m-vertical :is(.#{$toolbar}__content-section,
.#{$toolbar}__group, .#{$toolbar}__item) { align-items: stretch; }, or if this
styling only applies when the toolbar lives inside the page dock, place the rule
into a dedicated cross-component override file gated by .#{$page}__dock to
preserve scope.
Member
|
3 questions/comments -
Alignment in the demo (hi-c is on) Video of the collapse: |
Contributor
Author
|
From meeting w/ @andrew-ronaldson on 3.3.26
|
Contributor
Author
|
Going to merge as is and follow-up with making these updates
|
andrew-ronaldson
approved these changes
Mar 13, 2026
Collaborator
|
🎉 This PR is included in version 6.5.0-prerelease.52 🎉 The release is available on: Your semantic-release bot 📦🚀 |
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
4 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.
fixes #8040
fixes #8093
Summary by CodeRabbit
New Features
Documentation