ref(nav) update page frame mobile navigation#111387
Conversation
github-actions
bot
added
the
Scope: Frontend
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>
| @@ -1,230 +0,0 @@ | |||
| import {Fragment, useRef} from 'react'; | |||
There was a problem hiding this comment.
Since we need to use the buttonbar from the sidebar in the top menu, it's been split into a separate component
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
…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>
natemoo-re
left a comment
natemoo-re
left a comment
There was a problem hiding this comment.
I know you acknowledged that the logic is a bit hard to follow, but it seems like we can reduce the indirection in most of these cases.
Markup + styling all LGTM tho! Nice cleanup 🙌
| export function Page(props: FlexProps<'main'> & {withPadding?: boolean}) { | ||
| const hasPageFrame = useHasPageFrameFeature(); | ||
| const primaryNavigation = usePrimaryNavigation(); | ||
| const secondaryNavigation = useContext(SecondaryNavigationContext); |
There was a problem hiding this comment.
Very minor, but any reason this doesn't use useSecondaryNavigation()?
There was a problem hiding this comment.
The secondary layout is actually managed by the primary nav in our current model because of the hover/peek interaction. I had actually been thinking that we might be able to simplify this and entirely remove the hook if we use a data-attr to signal navigation state and use CSS boolean operations
| height={layout === 'mobile' && !isMobilePageFrame ? 'auto' : undefined} | ||
| width={layout === 'mobile' && !isMobilePageFrame ? '100%' : undefined} | ||
| padding={layout === 'mobile' && !isMobilePageFrame ? 'md lg' : 'xs'} | ||
| justify={layout === 'mobile' && !isMobilePageFrame ? 'start' : 'center'} |
There was a problem hiding this comment.
Double checking the logic here... we've got two layout === 'mobile' checks, so this could be simplified to:
layout === 'mobile' && !hasPageFrame
| width={isMobilePageFrame ? '100%' : `${size}px`} | ||
| ref={isMobilePageFrame ? undefined : mergeRefs(resizableContainerRef, ref)} |
| minMenuWidth={200} | ||
| trigger={triggerProps => | ||
| layout === 'mobile' ? ( | ||
| layout === 'mobile' && !isMobilePageFrame ? ( |
There was a problem hiding this comment.
Can simplify to layout === 'mobile' && !hasPageFrame to remove the duplicate check?
| hasPageFrame && secondaryNavigation.view === 'expanded' ? undefined : 'primary' | ||
| hasPageFrame && secondaryNavigation.view === 'expanded' && !isMobilePageFrame | ||
| ? undefined | ||
| : 'primary' |
There was a problem hiding this comment.
Duplicate hasPageFrame means we can simplify to
hasPageFrame && secondaryNavigation.view === 'expanded' && layout !== 'mobile'
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>
I'll cleanup the expressions. Both layout === 'mobile' and hasPageFrame will be dead code once the new nav is released, so it'll simplify this a lot! |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
2 participants
Updates page frame mobile navigation to show the primary navigation we use on desktop and moves the primary footer items onto the mobile top navigation.
This got a bit hairy with the styling overrides, but is a consequence of us toggling the styling between mobile and desktop styling at runtime. That design hits its limitations once we start to introuduce more than two variants, and I'm not happy. That said, I am ok with this idea because the new nav won't require us to keep that distinction (see screenshots). Since the new primary and secondary navigation styles match in desktop and mobile, it means that once this flag is released, we will be able to simplify this to just the desktop navigation and the top menu bar