fix(ui): align sidebar hamburger in macOS fullscreen#9257
Conversation
The hamburger button is absolutely positioned with a 96px left inset to clear the traffic-light controls, which are hidden in macOS fullscreen. Without traffic lights to clear, that inset made the button appear detached from the sidebar navigation. Forward enter-full-screen / leave-full-screen events from the main window to the renderer, expose an initial-state query via IPC, and drop the traffic-light inset on the hamburger header when fullscreen is active. Fixes aaif-goose#8510 Signed-off-by: Cole McIntosh <colemcintosh6@gmail.com>
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 645c1447fa
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| aria-hidden={!isNavExpanded} | ||
| className={cn( | ||
| 'w-full h-full', | ||
| effectiveNavigationStyle === 'condensed' && !isHorizontalNav | ||
| ? 'overflow-visible' | ||
| : 'overflow-hidden' | ||
| 'w-full h-full overflow-hidden', | ||
| !isNavExpanded && 'pointer-events-none' | ||
| )} |
There was a problem hiding this comment.
When push-mode navigation is collapsed, this wrapper only fades the nav out, marks it aria-hidden, and disables pointer events, but the <Navigation /> subtree remains mounted with its buttons/search input still focusable by keyboard. In the collapsed state users can tab into invisible controls inside the zero-width/zero-height nav; pointer-events-none does not affect keyboard focus, and aria-hidden does not make descendants inert. The previous NavigationPanel behavior returned null when collapsed, so this is a regression for keyboard navigation unless the subtree is unmounted or made inert/tab-disabled while hidden.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
No longer applies — the commit this was reviewing (645c144) was reverted to scope the PR to the issue fix only. The collapsed-nav tab-order concern is moot since Navigation now unmounts when collapsed (the original behavior) again.
colesmcintosh
force-pushed
the
fix/sidebar-fullscreen-alignment
branch
from
645c144 to
0ce65b4
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0ce65b4025
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| useEffect(() => { | ||
| if (!safeIsMacOS) return; | ||
| window.electron | ||
| .getIsFullScreen() |
There was a problem hiding this comment.
When AppLayout mounts under the existing mocked macOS renderer environment, this unconditional call now requires window.electron.getIsFullScreen, but the current Electron mocks used by the App tests/global renderer setup only define platform, getSetting, setSetting, etc. That makes affected App/AppLayout tests throw TypeError: window.electron.getIsFullScreen is not a function as soon as the effect runs in CI unless the new preload API is added to those mocks or this call is guarded.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 4daffad — added getIsFullScreen / on / off to the renderer test mock in src/test/setup.ts. All 346 tests still pass locally.
AppLayout now calls getIsFullScreen / on / off on window.electron; keep the renderer test mock in sync so future tests that mount the layout don't trip a TypeError. Signed-off-by: Cole McIntosh <colemcintosh6@gmail.com>
2 participants
Summary
In macOS fullscreen, the sidebar hamburger button appeared detached from the rest of the sidebar navigation. The button is absolutely positioned with a 96px left inset to clear the traffic-light controls in windowed mode, but the traffic lights are hidden in fullscreen so the inset became wasted space and visually orphaned the button.
This change forwards Electron
enter-full-screen/leave-full-screenevents from the main window to the renderer, exposes an initial-state query over IPC, and conditionally drops the traffic-light inset on the hamburger header when fullscreen is active so it aligns with the rest of the sidebar items.Fixes #8510