Adjust new subnav bar to UX designs#356
Conversation
WalkthroughRefactors header layout to use Masthead/MastheadContent and removes the Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant PN as PageNavigation
participant OC as OrgContext
participant MH as Masthead
participant OD as OrganizationDropdown
participant OS as OrganizationSelector
U->>PN: Render header
PN->>OC: Check organizations
alt Multiple organizations
PN->>MH: Render Masthead / MastheadContent
MH->>OD: Render organization dropdown (popperProps -> right)
U-->>OD: Interact with dropdown
OD->>OS: Open OrganizationSelector (modal)
else Single or none
PN-->>U: Render no org controls
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
libs/ui-components/src/components/common/PageNavigation.tsx (1)
77-86: Redundant condition check.The
showOrganizationSelectioncheck on line 77 is redundant because there's already an early return on line 64 whenshowOrganizationSelectionis false. This condition will always be true at this point.Apply this diff to remove the redundant condition:
<Toolbar isFullHeight isStatic className="fctl-subnav_toolbar"> <ToolbarContent> - {showOrganizationSelection && ( - <ToolbarItem> - <OrganizationDropdown - organizationName={currentOrgDisplayName} - onSwitchOrganization={() => { - setShowOrganizationModal(true); - }} - /> - </ToolbarItem> - )} + <ToolbarItem> + <OrganizationDropdown + organizationName={currentOrgDisplayName} + onSwitchOrganization={() => { + setShowOrganizationModal(true); + }} + /> + </ToolbarItem> </ToolbarContent> </Toolbar>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/standalone/src/app/components/AppLayout/AppNavigation.tsx(1 hunks)apps/standalone/src/app/components/AppLayout/AppToolbar.css(1 hunks)libs/ui-components/src/components/common/PageNavigation.tsx(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
libs/ui-components/src/components/common/PageNavigation.tsx (1)
libs/ui-components/src/components/common/OrganizationGuard.tsx (1)
useOrganizationGuardContext(17-23)
⏰ 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). (4)
- GitHub Check: integration-tests
- GitHub Check: Build
- GitHub Check: Build ocp plugin
- GitHub Check: Lint
🔇 Additional comments (9)
apps/standalone/src/app/components/AppLayout/AppNavigation.tsx (1)
13-13: LGTM! Zero padding aligns navigation with UX specs.The inline style removes padding from the NavList, which helps the subnav height match the left navigation menu's first element as specified in the PR objectives.
apps/standalone/src/app/components/AppLayout/AppToolbar.css (3)
1-4: LGTM! Consolidates toolbar alignment.The shared selector for both toolbar classes ensures consistent right-alignment of toolbar content.
6-11: LGTM! Height variable matches PR objective.The 3.5rem minimum height for the masthead content aligns with the PR objective to match the height of the first element in the left navigation menu.
13-15: Verify toolbar background matches left nav
TheNavcomponent withtheme="dark"defaults to its own PatternFly dark background variable—no custom override found—so confirm that--pf-v5-global--BackgroundColor--dark-300here matches the left navigation’s background (or adjust to use the exact PF variable the Nav uses).libs/ui-components/src/components/common/PageNavigation.tsx (5)
6-8: LGTM! New imports support Masthead layout.The added imports enable the migration from Toolbar-based to Masthead-based header structure.
50-50: LGTM! Right positioning prevents text cutoff.The
popperProps={{ position: 'right' }}ensures the dropdown opens to the right, addressing the PR objective about text in the "Change organization" dropdown being cut off.
64-66: LGTM! Simplified early return.The early return when organization selection is not needed is cleaner without the children check, since children are no longer supported.
73-90: LGTM! Masthead structure aligns with CSS changes.The new Masthead/MastheadContent wrapper replaces the previous Toolbar-based layout and correctly uses the
global-actions-mastheadID that's styled in AppToolbar.css. The structure supports the PR objectives for matching background color and height.
59-59: No PageNavigation usages with children; breaking change is safe.
celdrake
force-pushed
the
improve-subnav-bar-styles
branch
from
f6a9114 to
db852c1
Compare
| <MastheadContent> | ||
| <Toolbar isFullHeight isStatic className="fctl-subnav_toolbar"> | ||
| <ToolbarContent> | ||
| {showOrganizationSelection && ( |
There was a problem hiding this comment.
Keeping this condition despite being redundant, since it won't be once we add the new "Copy login command" option to the navigation bar.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/standalone/src/app/components/AppLayout/AppToolbar.css (1)
1-4: Useflex-endfor broader browser support.
justify-content: endonly works in relatively recent browser releases (e.g., Safari ≥15.4, Edge/Chrome ≥93); swapping toflex-endkeeps the same visual result but avoids regressions for users on slightly older clients. (caniuse.com)-.fctl-app_toolbar, -.fctl-subnav_toolbar { - justify-content: end; -} +.fctl-app_toolbar, +.fctl-subnav_toolbar { + justify-content: flex-end; +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/standalone/src/app/components/AppLayout/AppNavigation.tsx(1 hunks)apps/standalone/src/app/components/AppLayout/AppToolbar.css(1 hunks)libs/ui-components/src/components/common/PageNavigation.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/standalone/src/app/components/AppLayout/AppNavigation.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
libs/ui-components/src/components/common/PageNavigation.tsx (1)
libs/ui-components/src/components/common/OrganizationGuard.tsx (1)
useOrganizationGuardContext(17-23)
⏰ 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). (4)
- GitHub Check: integration-tests
- GitHub Check: Build
- GitHub Check: Build ocp plugin
- GitHub Check: Lint
🔇 Additional comments (1)
libs/ui-components/src/components/common/PageNavigation.tsx (1)
58-99: Drop children prop: no usages found Verified all<PageNavigation>instances across the codebase are self-closing (<PageNavigation />); no callers pass children.
celdrake
force-pushed
the
improve-subnav-bar-styles
branch
from
db852c1 to
351344b
Compare
celdrake
force-pushed
the
improve-subnav-bar-styles
branch
from
351344b to
77c8a6a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
libs/ui-components/src/components/common/PageNavigation.tsx (1)
76-85: Redundant conditional inside already-gated branch (or remove outer gate per Option A)Given the early return (Lines 63-65),
showOrganizationSelectionis always true here. Either:
- remove this inner condition now, or
- adopt Option A from the earlier comment (always render container; gate only dropdown here).
Minimal cleanup if keeping early return:
- {showOrganizationSelection && ( - <ToolbarItem> - <OrganizationDropdown - organizationName={currentOrgDisplayName} - onSwitchOrganization={() => { - setShowOrganizationModal(true); - }} - /> - </ToolbarItem> - )} + <ToolbarItem> + <OrganizationDropdown + organizationName={currentOrgDisplayName} + onSwitchOrganization={() => setShowOrganizationModal(true)} + /> + </ToolbarItem>
🧹 Nitpick comments (6)
apps/standalone/src/app/components/AppLayout/AppToolbar.css (2)
8-11: Masthead min-height token may be variant-scoped; prefer the base content token (and set display inline in JSX)
--pf-v5-c-masthead--m-display-inline__content--MinHeightapplies under the “display inline” modifier. Either:
- set Masthead
display="inline"in JSX, or- use the base content token to avoid modifier coupling.
Suggested CSS change:
#global-actions-masthead { padding: 0; - --pf-v5-c-masthead--m-display-inline__content--MinHeight: 3.5rem; + /* Ensure height regardless of display mode */ + --pf-v5-c-masthead__content--MinHeight: 3.5rem; }And in JSX (see PageNavigation.tsx Lines 72-72):
-<Masthead id="global-actions-masthead"> +<Masthead id="global-actions-masthead" display="inline">Based on learnings
13-15: Ensure readable foreground on dark backgroundYou set Toolbar background to
--pf-v5-global--BackgroundColor--dark-300. Also set the toolbar text color token for contrast.#global-actions-masthead .fctl-subnav_toolbar { --pf-v5-c-toolbar--BackgroundColor: var(--pf-v5-global--BackgroundColor--dark-300); + --pf-v5-c-toolbar--Color: var(--pf-v5-global--Color--light-100); }Based on learnings
libs/ui-components/src/components/common/PageNavigation.tsx (4)
49-49: Verify Dropdown positioning APIUsing
popperProps={{ position: 'right' }}may be unnecessary ifDropdownsupports a directposition="right"prop. Confirm against current PatternFly API and use the canonical prop to avoid silent no-ops.If needed:
- popperProps={{ position: 'right' }} + position="right"Based on learnings
71-90: Tie CSS token and structure together: set Masthead display="inline"To make the height token effective and align with the left nav item height, set the Masthead to “inline” display (pairs with the token change suggested in CSS review).
- <Masthead id="global-actions-masthead"> + <Masthead id="global-actions-masthead" display="inline">Based on learnings
38-47: Add an accessible label to the organization togglePlain text toggle is fine, but an explicit aria-label helps SR users.
<MenuToggle ref={toggleRef} onClick={onDropdownToggle} id="organizationMenu" isFullHeight isExpanded={isDropdownOpen} variant="plainText" + aria-label={t('Organization menu')} >
58-66: PageNavigation children removal verified; consider gating-only refactor
- The
childrenprop was removed from PageNavigation’s signature; no JSX call sites with<PageNavigation>…</PageNavigation>were found in.tsxfiles.- The early return (
if (!showOrganizationSelection) return null) hides the entire Masthead when there’s only one organization. If you plan to add “global actions” here in the future, remove that return and wrap only the dropdown in theshowOrganizationSelectioncheck.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/standalone/src/app/components/AppLayout/AppNavigation.tsx(1 hunks)apps/standalone/src/app/components/AppLayout/AppToolbar.css(1 hunks)libs/ui-components/src/components/common/PageNavigation.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/standalone/src/app/components/AppLayout/AppNavigation.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
libs/ui-components/src/components/common/PageNavigation.tsx (1)
libs/ui-components/src/components/common/OrganizationGuard.tsx (1)
useOrganizationGuardContext(17-23)
⏰ 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). (4)
- GitHub Check: integration-tests
- GitHub Check: Build
- GitHub Check: Lint
- GitHub Check: Build ocp plugin
🔇 Additional comments (1)
apps/standalone/src/app/components/AppLayout/AppToolbar.css (1)
1-4: Selector consolidation LGTMSharing justify-content across .fctl-app_toolbar and .fctl-subnav_toolbar is clean and correct.
2 participants
The current design had some misalignments to the UX designs:
Before the changes:
After the changes:
Summary by CodeRabbit
Refactor
Style
Enhancements