Skip to content

ref(nav) separate mobile context#111416

Merged
JonasBa merged 20 commits intomasterfrom
jb/ref/nav-mobile-context
Mar 24, 2026
Merged

ref(nav) separate mobile context#111416
JonasBa merged 20 commits intomasterfrom
jb/ref/nav-mobile-context

Conversation

@JonasBa
Copy link
Copy Markdown
Member

@JonasBa JonasBa commented Mar 24, 2026

Mobile navigation changes should not affect user's desktop preferences. This PR mitigates this by wrapping the secondary navigation using in-memory expanded/collapsed state as opposed to reusing the desktops localStorage persisted state

JonasBa and others added 19 commits March 23, 2026 14:53
Replace all inline 'hasPageFrame && layout === mobile' compound checks with
a named isMobilePageFrame constant in each function scope. Also fix the
hamburger button aria-label in MobilePageFrameNavigation and the hasPageFrame
early-return path of MobileNavigation to toggle between 'Open main menu' and
'Close main menu' based on open state.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ogic

When PrimaryNavigationItems was extracted from Navigation, its local ref was
never attached to any DOM element. useActivateNavigationGroupOnHover received
a ref with current === null, causing useMouseMovement to skip event listener
setup and breaking the smart hover delay for the desktop sidebar.

Fix by passing the ref from Navigation (which is attached to PrimaryNavigation.List)
down to PrimaryNavigationItems as listRef. Mobile usages fall back to a local
null ref, preserving the previous no-op behavior since hover delay is desktop-only.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Remove unreachable if (hasPageFrame) block from MobileNavigation — index.tsx
only renders MobileNavigation when hasPageFrame is false, so this branch
could never execute. The dead block also rendered a non-functional menu
button whose overlay would never appear.

Fix MobilePageFrameNavigation useEffect calling setView('expanded') on
initial mount. Since isOpen initializes as false, the else branch fired
immediately and overwrote any persisted collapsed preference. Now checks
scrollLock.held() before calling setView — if the lock is not held the
panel was never open, so no reset is needed. Expose held() on useScrollLock
to support this check.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@JonasBa JonasBa requested a review from natemoo-re March 24, 2026 17:27
@JonasBa JonasBa requested a review from a team as a code owner March 24, 2026 17:27
@github-actions github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Mar 24, 2026
…xtProvider

The mobile context provider passed useState's setView directly as the context value, but the context interface expects (view: SecondaryNavState) => void. Wrap in useCallback to match the expected signature.

Co-Authored-By: Claude Sonnet 4 <noreply@example.com>
@natemoo-re
Copy link
Copy Markdown
Member

@JonasBa rebase to drop commits from #111387 ?

Copy link
Copy Markdown
Member

@natemoo-re natemoo-re left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess all the commits will get squashed anyway. LGTM!

@JonasBa JonasBa merged commit 9ffc883 into master Mar 24, 2026
70 checks passed
@JonasBa JonasBa deleted the jb/ref/nav-mobile-context branch March 24, 2026 20:05
Asynchronite pushed a commit to Asynchronite/sentry that referenced this pull request Mar 24, 2026
Mobile navigation changes should not affect user's desktop preferences.
This PR mitigates this by wrapping the secondary navigation using
in-memory expanded/collapsed state as opposed to reusing the desktops
localStorage persisted state

---------

Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-authored-by: Claude Sonnet 4 <noreply@example.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Scope: Frontend Automatically applied to PRs that change frontend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants