feat(Compass): added responsive docked compass#12382
Merged
kmcfaul merged 3 commits intopatternfly:mainfrom Apr 28, 2026
Merged
feat(Compass): added responsive docked compass#12382kmcfaul merged 3 commits intopatternfly:mainfrom
kmcfaul merged 3 commits intopatternfly:mainfrom
Conversation
WalkthroughAdds dock-related props and an optional masthead slot to Compass, introduces a new CompassDockMain component and tests, refactors Compass demos and Nav for responsive dock behavior, and bumps Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Demo as CompassDockDemo
participant Compass as Compass
participant DockMain as CompassDockMain
participant UI as Rendered UI
User->>Demo: Interact (hamburger / dock toggle)
Demo->>Demo: Update isMobile / isDockExpanded / isDockTextExpanded
Demo->>Compass: Pass props (dock, masthead, isDockExpanded, isDockTextExpanded)
Compass->>DockMain: Render dock content via CompassDockMain
Compass->>UI: Compute/apply modifier classes (expanded / textExpanded)
UI->>User: Render expanded labels or collapsed icons accordingly
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Collaborator
|
Preview: https://pf-react-pr-12382.surge.sh A11y report: https://pf-react-pr-12382-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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/react-core/src/demos/examples/Nav/NavDockedNav.tsx (1)
174-180:⚠️ Potential issue | 🟠 MajorOnly move focus into the dock when it opens.
This handler always focuses
dockedToggleRefafter toggling. If the mobile toggle is used to close the dock, focus gets pushed into content that has just been hidden, which breaks keyboard navigation.Suggested fix
const onMobileToggle = () => { - setIsDockExpanded(!isDockExpanded); - - setTimeout(() => { - dockedToggleRef.current?.focus(); - }, 200); + const nextIsDockExpanded = !isDockExpanded; + setIsDockExpanded(nextIsDockExpanded); + + if (nextIsDockExpanded) { + setTimeout(() => { + dockedToggleRef.current?.focus(); + }, 200); + } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-core/src/demos/examples/Nav/NavDockedNav.tsx` around lines 174 - 180, onMobileToggle currently always moves focus to dockedToggleRef after toggling, which incorrectly shifts focus when closing; change setIsDockExpanded to the functional form (setIsDockExpanded(prev => { const next = !prev; ... return next; })) or compute the next state before calling setIsDockExpanded, and only run the setTimeout + dockedToggleRef.current?.focus() when next is true (i.e., when the dock is opening), leaving closing behavior untouched.packages/react-core/src/components/Compass/Compass.tsx (1)
80-97:⚠️ Potential issue | 🟠 MajorNormalize the dock-presence check before classing and rendering.
Line 82 treats
dock={false}as “present”, but Lines 86-97 drop the dock because they rely on truthiness. A common pattern likedock={isEnabled && <Nav />}will putCompassinto docked mode with no dock content rendered.Suggested fix
}: CompassProps) => { const hasDrawer = drawerContent !== undefined; + const hasDock = dock !== null && dock !== undefined && dock !== false; const backgroundImageStyles: { [key: string]: string } = {}; @@ <div - className={css(styles.compass, dock !== undefined && styles.modifiers.docked, className)} + className={css(styles.compass, hasDock && styles.modifiers.docked, className)} {...props} style={{ ...props.style, ...backgroundImageStyles }} > - {dock && masthead} - {dock && ( + {hasDock && masthead} + {hasDock && ( <div className={css( `${styles.compass}__dock`,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-core/src/components/Compass/Compass.tsx` around lines 80 - 97, The component treats dock presence inconsistently: className uses dock !== undefined but the JSX render uses truthy checks (dock && ...) so dock={false} marks the component as docked but renders no dock content; fix by normalizing to an explicit presence check (e.g. const hasDock = dock !== undefined) and replace the truthy checks with hasDock in the places that currently read {dock && masthead} and {dock && (<div ...>{dock}</div>)} and use hasDock for the className decision so dock presence, expansion flags (isDockExpanded, isDockTextExpanded), and rendering are consistent.
🤖 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/demos/Compass/examples/CompassDockDemo.tsx`:
- Around line 65-71: onMobileToggle currently always moves focus to
dockedToggleRef after toggling; change it to only set focus when the mobile nav
is opening (i.e., when the dock is transitioning from collapsed to expanded). In
the onMobileToggle function, check the current isDockExpanded value (or compute
the new state) and only run the setTimeout(... dockedToggleRef.current?.focus())
when opening (when isDockExpanded is false before toggling or newExpandedState
=== true); leave the state flip via setIsDockExpanded unchanged.
---
Outside diff comments:
In `@packages/react-core/src/components/Compass/Compass.tsx`:
- Around line 80-97: The component treats dock presence inconsistently:
className uses dock !== undefined but the JSX render uses truthy checks (dock &&
...) so dock={false} marks the component as docked but renders no dock content;
fix by normalizing to an explicit presence check (e.g. const hasDock = dock !==
undefined) and replace the truthy checks with hasDock in the places that
currently read {dock && masthead} and {dock && (<div ...>{dock}</div>)} and use
hasDock for the className decision so dock presence, expansion flags
(isDockExpanded, isDockTextExpanded), and rendering are consistent.
In `@packages/react-core/src/demos/examples/Nav/NavDockedNav.tsx`:
- Around line 174-180: onMobileToggle currently always moves focus to
dockedToggleRef after toggling, which incorrectly shifts focus when closing;
change setIsDockExpanded to the functional form (setIsDockExpanded(prev => {
const next = !prev; ... return next; })) or compute the next state before
calling setIsDockExpanded, and only run the setTimeout +
dockedToggleRef.current?.focus() when next is true (i.e., when the dock is
opening), leaving closing behavior untouched.
🪄 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: 64959357-27a8-4e62-89d3-be13dc55c4ef
⛔ Files ignored due to path filters (2)
packages/react-core/src/components/Compass/__tests__/__snapshots__/CompassDockMain.test.tsx.snapis excluded by!**/*.snapyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (13)
packages/react-core/package.jsonpackages/react-core/src/components/Compass/Compass.tsxpackages/react-core/src/components/Compass/CompassDockMain.tsxpackages/react-core/src/components/Compass/__tests__/Compass.test.tsxpackages/react-core/src/components/Compass/__tests__/CompassDockMain.test.tsxpackages/react-core/src/components/Compass/index.tspackages/react-core/src/demos/Compass/Compass.mdpackages/react-core/src/demos/Compass/examples/CompassDockDemo.tsxpackages/react-core/src/demos/examples/Nav/NavDockedNav.tsxpackages/react-docs/package.jsonpackages/react-icons/package.jsonpackages/react-styles/package.jsonpackages/react-tokens/package.json
Comment on lines
+65
to
+71
| const onMobileToggle = () => { | ||
| setIsDockExpanded(!isDockExpanded); | ||
|
|
||
| setTimeout(() => { | ||
| dockedToggleRef.current?.focus(); | ||
| }, 200); | ||
| }; |
There was a problem hiding this comment.
Only send focus to the dock when the mobile nav is opening.
This runs on close as well as open. Closing from the mobile toggle will move focus to dockedToggleRef after the dock collapses, leaving keyboard focus in hidden UI.
Suggested fix
const onMobileToggle = () => {
- setIsDockExpanded(!isDockExpanded);
-
- setTimeout(() => {
- dockedToggleRef.current?.focus();
- }, 200);
+ const nextIsDockExpanded = !isDockExpanded;
+ setIsDockExpanded(nextIsDockExpanded);
+
+ if (nextIsDockExpanded) {
+ setTimeout(() => {
+ dockedToggleRef.current?.focus();
+ }, 200);
+ }
};📝 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
| const onMobileToggle = () => { | |
| setIsDockExpanded(!isDockExpanded); | |
| setTimeout(() => { | |
| dockedToggleRef.current?.focus(); | |
| }, 200); | |
| }; | |
| const onMobileToggle = () => { | |
| const nextIsDockExpanded = !isDockExpanded; | |
| setIsDockExpanded(nextIsDockExpanded); | |
| if (nextIsDockExpanded) { | |
| setTimeout(() => { | |
| dockedToggleRef.current?.focus(); | |
| }, 200); | |
| } | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/react-core/src/demos/Compass/examples/CompassDockDemo.tsx` around
lines 65 - 71, onMobileToggle currently always moves focus to dockedToggleRef
after toggling; change it to only set focus when the mobile nav is opening
(i.e., when the dock is transitioning from collapsed to expanded). In the
onMobileToggle function, check the current isDockExpanded value (or compute the
new state) and only run the setTimeout(... dockedToggleRef.current?.focus())
when opening (when isDockExpanded is false before toggling or newExpandedState
=== true); leave the state flip via setIsDockExpanded unchanged.
mcoker
reviewed
Apr 27, 2026
Contributor
mcoker
left a comment
mcoker
left a comment
There was a problem hiding this comment.
LGTM, just one small about the docs
Comment on lines
+17
to
+18
| /** @beta Flag indicating the docked nav should display text on desktop. Only applies when dock content is passed, and will handle | ||
| * setting isTextExpanded on individual isDocked components. |
Contributor
There was a problem hiding this comment.
Since it just applies the text-expanded modifier to the dock element, do we need that last bit?
Suggested change
| /** @beta Flag indicating the docked nav should display text on desktop. Only applies when dock content is passed, and will handle | |
| * setting isTextExpanded on individual isDocked components. | |
| /** @beta Flag indicating the docked nav should display text on desktop. Only applies when dock content is passed. |
kmcfaul
requested changes
Apr 27, 2026
Contributor
kmcfaul
left a comment
kmcfaul
left a comment
There was a problem hiding this comment.
Small demo bug, otherwise lgtm.
| }; | ||
|
|
||
| const onToggleDock = () => { | ||
| if (isDockExpanded) { |
Contributor
There was a problem hiding this comment.
Noticed a bit of a bug in behavior due to this check: if you're on mobile view & expand the dock, and then widen the window to desktop, the first toggle click will not appear to work (because it is removing the pf-m-expanded modifier and not touching the text-expanded).
Not sure if there's a different way we can reset the mobile state or if we have to (pf-m-text-expanded currently persists between mobile and desktop view).
thatblindgeye
force-pushed
the
iss12196_compassDocked
branch
from
e52cff6 to
15b36c9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/react-core/src/demos/examples/Nav/NavDockedNav.tsx (1)
189-195:⚠️ Potential issue | 🟠 MajorDon't move focus to the dock toggle when closing the mobile nav.
onMobileToggleruns on both open and close. On close, focus is moved into the docked masthead, which is hidden on mobile, leaving keyboard focus in inaccessible UI. Only schedule focus todockedToggleRefwhen transitioning into the expanded state.🛡️ Suggested fix
const onMobileToggle = () => { - setIsDockExpanded(!isDockExpanded); - - setTimeout(() => { - dockedToggleRef.current?.focus(); - }, 200); + const nextIsDockExpanded = !isDockExpanded; + setIsDockExpanded(nextIsDockExpanded); + + if (nextIsDockExpanded) { + setTimeout(() => { + dockedToggleRef.current?.focus(); + }, 200); + } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-core/src/demos/examples/Nav/NavDockedNav.tsx` around lines 189 - 195, onMobileToggle currently flips isDockExpanded and always schedules focus to dockedToggleRef; change it so the focus scheduling only happens when opening (i.e., when the new state is expanded). Use the current isDockExpanded to compute the next state via setIsDockExpanded(prev => { const next = !prev; if (next) setTimeout(() => dockedToggleRef.current?.focus(), 200); return next; }) or equivalent, so focus is not moved when closing; keep references to onMobileToggle, isDockExpanded, setIsDockExpanded, and dockedToggleRef to locate the change.
♻️ Duplicate comments (2)
packages/react-core/src/demos/Compass/examples/CompassDockDemo.tsx (2)
80-86:⚠️ Potential issue | 🟠 MajorOnly move focus to the dock toggle when opening the mobile nav.
onMobileTogglealways schedules focus todockedToggleRef, including on close. Closing from the mobile toggle will move keyboard focus into the now-hidden docked masthead.🛡️ Suggested fix
const onMobileToggle = () => { - setIsDockExpanded(!isDockExpanded); - - setTimeout(() => { - dockedToggleRef.current?.focus(); - }, 200); + const nextIsDockExpanded = !isDockExpanded; + setIsDockExpanded(nextIsDockExpanded); + + if (nextIsDockExpanded) { + setTimeout(() => { + dockedToggleRef.current?.focus(); + }, 200); + } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-core/src/demos/Compass/examples/CompassDockDemo.tsx` around lines 80 - 86, onMobileToggle currently always schedules a focus on dockedToggleRef after toggling, which moves focus into the hidden dock when closing; change it to compute the next state (e.g. const nextExpanded = !isDockExpanded), call setIsDockExpanded(nextExpanded), and only schedule the setTimeout/focus when nextExpanded is true so focus is moved only when opening the mobile nav; reference onMobileToggle, setIsDockExpanded, and dockedToggleRef.
53-64:⚠️ Potential issue | 🟠 MajorReset
isDockExpandedwhen the viewport leaves the mobile breakpoint.Same responsive-state bug as in
NavDockedNav.tsx: open the dock on mobile, resize pastglobalBreakpointLg, andisDockExpandedstaystrue. The first desktop toggle then flipsisDockTextExpandedwhile the dock still carriespf-m-expanded, so the click visually does nothing until the second click. Sync the mobile-only state at the breakpoint transition.🛡️ Suggested fix
useEffect(() => { const mobileBreakpoint = Number.parseInt(globalBreakpointLg.value) * 16; const mediaQuery = window.matchMedia(`(max-width: ${mobileBreakpoint}px)`); const handleResize = (e: MediaQueryListEvent | MediaQueryList) => { - setIsMobile(e.matches); + setIsMobile(e.matches); + if (!e.matches) { + setIsDockExpanded(false); + } }; … }, []);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-core/src/demos/Compass/examples/CompassDockDemo.tsx` around lines 53 - 64, In CompassDockDemo's useEffect resize handler, update the handleResize (used with window.matchMedia) to also reset the dock mobile-only state when leaving the mobile breakpoint: keep calling setIsMobile(e.matches) and when e.matches is false (i.e., viewport is now desktop) call setIsDockExpanded(false) so the dock doesn't remain expanded after resizing; change the handler used in mediaQuery.addEventListener/removeEventListener (and the initial handleResize call) to this updated function and ensure you reference the CompassDockDemo state setters setIsMobile and setIsDockExpanded so the mobile-only expansion state is synced on breakpoint transitions.
🧹 Nitpick comments (2)
packages/react-core/src/components/Compass/Compass.tsx (1)
11-12:mastheadJSDoc overstates viewport gating.The Compass component itself does not viewport-gate the masthead; it only checks that
dockis provided. The "only at mobile viewports" behavior is the consumer's responsibility (e.g. viaMasthead display={{ default: 'inline' }}). Consider rewording so consumers don't expect Compass to enforce a mobile-only display.✏️ Suggested doc tightening
- /** The horizontal masthead content (e.g. <Masthead />). This masthead will only render when dock content is passed and only at mobile viewports. */ + /** Horizontal masthead content (e.g. <Masthead />) rendered alongside the dock. Only rendered when `dock` is provided. Typically used to show a mobile-only masthead via the consumer's `Masthead display` prop. */ masthead?: 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/Compass/Compass.tsx` around lines 11 - 12, Update the JSDoc for the masthead prop on the Compass component: remove the phrase that implies Compass enforces "only at mobile viewports" and instead state that masthead will render when dock is provided and that viewport-specific display is the consumer's responsibility (e.g., via Masthead display props). Target the masthead?: React.ReactNode comment in the Compass component to make this change.packages/react-core/src/demos/Compass/examples/CompassDockDemo.tsx (1)
102-145: Empty SVGlinearGradientid risks collisions.
id="linearGradient-"(with a dangling trailing dash) is also used by the same SVG inNavDockedNav.tsx. While each demo typically renders in its own iframe (isFullscreen), an empty/duplicated gradient id on the docs site can collide with other SVGs sharing that id and silently break the gradient fill (last-id-wins semantics in SVG). Consider giving it a unique suffix the waydockTextLogodoes (linearGradient-compass-docked-demo).- <linearGradient x1="68%" y1="2.25860997e-13%" x2="32%" y2="100%" id="linearGradient-"> + <linearGradient x1="68%" y1="2.25860997e-13%" x2="32%" y2="100%" id="linearGradient-compass-docked-demo-mobile">(And update both
fill="url(#linearGradient-)"references at the bottom of the same SVG.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-core/src/demos/Compass/examples/CompassDockDemo.tsx` around lines 102 - 145, The SVG in mobileTextLogo inside CompassDockDemo uses a non-unique id "linearGradient-" which can collide with other SVGs (e.g., NavDockedNav.tsx); change the id on the <linearGradient> (in the mobileTextLogo declaration) to a unique name (for example linearGradient-compass-docked-demo) and update the two corresponding fill="url(`#linearGradient-`)" attributes in the same SVG to match the new id; ensure the new id differs from dockTextLogo's id to avoid collisions.
🤖 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/demos/examples/Nav/NavDockedNav.tsx`:
- Around line 55-66: When detecting breakpoint changes in the useEffect's
handleResize (the MediaQueryListEvent | MediaQueryList handler used with
mediaQuery.addEventListener('change')), reset the mobile-specific dock state
when crossing back to desktop: if e.matches is false (i.e., not mobile) call
setIsDockExpanded(false) and also setIsDockTextExpanded(false) (or sync it
appropriately) in addition to setIsMobile(e.matches); this ensures the
pf-m-expanded mobile modifier is cleared when switching to desktop and prevents
the "first click does nothing" issue.
---
Outside diff comments:
In `@packages/react-core/src/demos/examples/Nav/NavDockedNav.tsx`:
- Around line 189-195: onMobileToggle currently flips isDockExpanded and always
schedules focus to dockedToggleRef; change it so the focus scheduling only
happens when opening (i.e., when the new state is expanded). Use the current
isDockExpanded to compute the next state via setIsDockExpanded(prev => { const
next = !prev; if (next) setTimeout(() => dockedToggleRef.current?.focus(), 200);
return next; }) or equivalent, so focus is not moved when closing; keep
references to onMobileToggle, isDockExpanded, setIsDockExpanded, and
dockedToggleRef to locate the change.
---
Duplicate comments:
In `@packages/react-core/src/demos/Compass/examples/CompassDockDemo.tsx`:
- Around line 80-86: onMobileToggle currently always schedules a focus on
dockedToggleRef after toggling, which moves focus into the hidden dock when
closing; change it to compute the next state (e.g. const nextExpanded =
!isDockExpanded), call setIsDockExpanded(nextExpanded), and only schedule the
setTimeout/focus when nextExpanded is true so focus is moved only when opening
the mobile nav; reference onMobileToggle, setIsDockExpanded, and
dockedToggleRef.
- Around line 53-64: In CompassDockDemo's useEffect resize handler, update the
handleResize (used with window.matchMedia) to also reset the dock mobile-only
state when leaving the mobile breakpoint: keep calling setIsMobile(e.matches)
and when e.matches is false (i.e., viewport is now desktop) call
setIsDockExpanded(false) so the dock doesn't remain expanded after resizing;
change the handler used in mediaQuery.addEventListener/removeEventListener (and
the initial handleResize call) to this updated function and ensure you reference
the CompassDockDemo state setters setIsMobile and setIsDockExpanded so the
mobile-only expansion state is synced on breakpoint transitions.
---
Nitpick comments:
In `@packages/react-core/src/components/Compass/Compass.tsx`:
- Around line 11-12: Update the JSDoc for the masthead prop on the Compass
component: remove the phrase that implies Compass enforces "only at mobile
viewports" and instead state that masthead will render when dock is provided and
that viewport-specific display is the consumer's responsibility (e.g., via
Masthead display props). Target the masthead?: React.ReactNode comment in the
Compass component to make this change.
In `@packages/react-core/src/demos/Compass/examples/CompassDockDemo.tsx`:
- Around line 102-145: The SVG in mobileTextLogo inside CompassDockDemo uses a
non-unique id "linearGradient-" which can collide with other SVGs (e.g.,
NavDockedNav.tsx); change the id on the <linearGradient> (in the mobileTextLogo
declaration) to a unique name (for example linearGradient-compass-docked-demo)
and update the two corresponding fill="url(`#linearGradient-`)" attributes in the
same SVG to match the new id; ensure the new id differs from dockTextLogo's id
to avoid collisions.
🪄 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: 340c5aaf-c29e-4222-8a97-5e939701f3bd
⛔ Files ignored due to path filters (2)
packages/react-core/src/components/Compass/__tests__/__snapshots__/CompassDockMain.test.tsx.snapis excluded by!**/*.snapyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (15)
packages/react-core/package.jsonpackages/react-core/src/components/Compass/Compass.tsxpackages/react-core/src/components/Compass/CompassDockMain.tsxpackages/react-core/src/components/Compass/__tests__/Compass.test.tsxpackages/react-core/src/components/Compass/__tests__/CompassDockMain.test.tsxpackages/react-core/src/components/Compass/index.tspackages/react-core/src/components/Page/Page.tsxpackages/react-core/src/demos/Compass/Compass.mdpackages/react-core/src/demos/Compass/examples/CompassDockDemo.tsxpackages/react-core/src/demos/Nav.mdpackages/react-core/src/demos/examples/Nav/NavDockedNav.tsxpackages/react-docs/package.jsonpackages/react-icons/package.jsonpackages/react-styles/package.jsonpackages/react-tokens/package.json
✅ Files skipped from review due to trivial changes (9)
- packages/react-core/src/components/Compass/index.ts
- packages/react-icons/package.json
- packages/react-tokens/package.json
- packages/react-docs/package.json
- packages/react-core/package.json
- packages/react-core/src/demos/Compass/Compass.md
- packages/react-core/src/components/Page/Page.tsx
- packages/react-core/src/components/Compass/CompassDockMain.tsx
- packages/react-styles/package.json
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/react-core/src/components/Compass/tests/CompassDockMain.test.tsx
- packages/react-core/src/components/Compass/tests/Compass.test.tsx
There was a problem hiding this comment.
♻️ Duplicate comments (3)
packages/react-core/src/demos/examples/Nav/NavDockedNav.tsx (1)
55-65:⚠️ Potential issue | 🟠 MajorClear the mobile-expanded state when the viewport returns to desktop.
If the dock is open on mobile and the window widens,
isDockExpandedstaystrue. The next desktop toggle only clears the stale mobile expanded modifier, so the first click appears to do nothing.Suggested fix
useEffect(() => { const mobileBreakpoint = Number.parseInt(globalBreakpointXl.value) * 16; const mediaQuery = window.matchMedia(`(max-width: ${mobileBreakpoint}px)`); const handleResize = (e: MediaQueryListEvent | MediaQueryList) => { setIsMobile(e.matches); + if (!e.matches) { + setIsDockExpanded(false); + } }; handleResize(mediaQuery); mediaQuery.addEventListener('change', handleResize);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-core/src/demos/examples/Nav/NavDockedNav.tsx` around lines 55 - 65, The effect that listens for viewport changes (useEffect with handleResize) must also clear the mobile-expanded dock state when switching to desktop: update handleResize (the function used with mediaQuery.addEventListener) so that when e.matches is false (i.e., not mobile) it calls the state updater that clears the mobile expansion (setIsDockExpanded(false) or the equivalent mobile-expand state setter), while preserving the existing setIsMobile(e.matches) behavior; ensure the same change happens on the initial handleResize(mediaQuery) call and in the cleanup remains unchanged.packages/react-core/src/demos/Compass/examples/CompassDockDemo.tsx (2)
53-63:⚠️ Potential issue | 🟠 MajorReset the mobile dock state when crossing back to desktop.
When the dock is expanded on mobile and the viewport widens,
isDockExpandedremains set. The first desktop toggle then just removes that stale mobile state instead of expanding the text dock.Suggested fix
useEffect(() => { const mobileBreakpoint = Number.parseInt(globalBreakpointLg.value) * 16; const mediaQuery = window.matchMedia(`(max-width: ${mobileBreakpoint}px)`); const handleResize = (e: MediaQueryListEvent | MediaQueryList) => { setIsMobile(e.matches); + if (!e.matches) { + setIsDockExpanded(false); + } }; handleResize(mediaQuery); mediaQuery.addEventListener('change', handleResize);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-core/src/demos/Compass/examples/CompassDockDemo.tsx` around lines 53 - 63, The useEffect's resize handler doesn't clear the mobile-only dock state, so when switching from mobile to desktop isDockExpanded stays true; update the handleResize function inside useEffect (the one passed to mediaQuery.addEventListener) to also reset the dock state when entering desktop by calling setIsDockExpanded(false) (or the appropriate collapsed value) whenever e.matches is false; ensure this applies both for the initial handleResize(mediaQuery) invocation and subsequent change events so the desktop toggle isn't working against stale mobile state (refer to handleResize, mediaQuery, setIsMobile, and setIsDockExpanded).
80-85:⚠️ Potential issue | 🟠 MajorOnly shift focus into the dock when the mobile nav is opening.
On close, this still moves focus to
dockedToggleRefafter the dock has collapsed, which leaves keyboard focus in hidden/inert UI.Suggested fix
const onMobileToggle = () => { - setIsDockExpanded(!isDockExpanded); - - setTimeout(() => { - dockedToggleRef.current?.focus(); - }, 200); + const nextIsDockExpanded = !isDockExpanded; + setIsDockExpanded(nextIsDockExpanded); + + if (nextIsDockExpanded) { + setTimeout(() => { + dockedToggleRef.current?.focus(); + }, 200); + } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-core/src/demos/Compass/examples/CompassDockDemo.tsx` around lines 80 - 85, The onMobileToggle handler currently always moves focus to dockedToggleRef after toggling, which shifts focus into hidden/inert UI when closing; change the logic in onMobileToggle to compute the next state (e.g., const next = !isDockExpanded), call setIsDockExpanded(next), and only run the setTimeout/focus call when next is true (dock is opening). Reference: onMobileToggle, setIsDockExpanded, and dockedToggleRef.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/react-core/src/demos/Compass/examples/CompassDockDemo.tsx`:
- Around line 53-63: The useEffect's resize handler doesn't clear the
mobile-only dock state, so when switching from mobile to desktop isDockExpanded
stays true; update the handleResize function inside useEffect (the one passed to
mediaQuery.addEventListener) to also reset the dock state when entering desktop
by calling setIsDockExpanded(false) (or the appropriate collapsed value)
whenever e.matches is false; ensure this applies both for the initial
handleResize(mediaQuery) invocation and subsequent change events so the desktop
toggle isn't working against stale mobile state (refer to handleResize,
mediaQuery, setIsMobile, and setIsDockExpanded).
- Around line 80-85: The onMobileToggle handler currently always moves focus to
dockedToggleRef after toggling, which shifts focus into hidden/inert UI when
closing; change the logic in onMobileToggle to compute the next state (e.g.,
const next = !isDockExpanded), call setIsDockExpanded(next), and only run the
setTimeout/focus call when next is true (dock is opening). Reference:
onMobileToggle, setIsDockExpanded, and dockedToggleRef.
In `@packages/react-core/src/demos/examples/Nav/NavDockedNav.tsx`:
- Around line 55-65: The effect that listens for viewport changes (useEffect
with handleResize) must also clear the mobile-expanded dock state when switching
to desktop: update handleResize (the function used with
mediaQuery.addEventListener) so that when e.matches is false (i.e., not mobile)
it calls the state updater that clears the mobile expansion
(setIsDockExpanded(false) or the equivalent mobile-expand state setter), while
preserving the existing setIsMobile(e.matches) behavior; ensure the same change
happens on the initial handleResize(mediaQuery) call and in the cleanup remains
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 32cd7c35-f6d5-4f57-8200-448d7844eea7
📒 Files selected for processing (2)
packages/react-core/src/demos/Compass/examples/CompassDockDemo.tsxpackages/react-core/src/demos/examples/Nav/NavDockedNav.tsx
kmcfaul
approved these changes
Apr 27, 2026
andrew-ronaldson
approved these changes
Apr 28, 2026
Collaborator
andrew-ronaldson
left a comment
andrew-ronaldson
left a comment
There was a problem hiding this comment.
Looks good. thanks
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
5 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 #12196
Additional issues:
Summary by CodeRabbit
New Features
Tests
Chores
Documentation