Convert unnecessary drawer navigation to flex Box#2021
Conversation
⚪ Code HealthNo change to the dead-code surface. 60 Unused files
157 Unused exports
103 Unused exported types
14 Unused exported enum members
5 Unused dependencies
3 Unused devDependencies
|
There was a problem hiding this comment.
Pull request overview
This PR removes the leftover MUI Drawer used for a now-permanent navigation rail and replaces it with a simpler Box-based flex layout, while preserving the expand/collapse behavior via persisted local storage state.
Changes:
- Replace
MuiDrawerwith a flexBoxin the navigation component. - Move navigation open/closed persistence into
Navigation(viauseLocalStorage) and simplifyLayoutto a pure flex layout. - Replace
NavWidthsenum with a constant widths object.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/context/Theme.tsx | Converts NavWidths from an enum to a const object for navigation sizing. |
| src/components/navigation/Navigation.tsx | Replaces Drawer with Box, adds persisted open/close state, and updates toggle behavior. |
| src/app/Layout.tsx | Simplifies layout to a flex row with Navigation as a sibling (removes margin-based offsetting). |
| .gitignore | Adds AGENTS.md to ignored files. |
Comments suppressed due to low confidence (1)
src/components/navigation/Navigation.tsx:62
Stackis set toheight: '100%'while also rendering a sibling<Toolbar />above it. With the outer nav container atheight: '100%', this makes the content always overflow by the toolbar height, causing a persistent scrollbar. Prefer flex sizing for the remaining space.
<Toolbar />
<Stack
sx={{
height: '100%',
justifyContent: 'space-between',
overflowX: 'hidden',
}}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const [nav, setNav] = useLocalStorage<{ open: boolean }>( | ||
| Keys.NAVIGATION_SETTINGS | ||
| ); | ||
| const open = nav?.open ?? true; |
There was a problem hiding this comment.
IMO it's a wash either way and i have a slight preference for not writing to localstorage (which the default would do) unless the user actually interacts.
| <ListItemButton | ||
| component="a" | ||
| onClick={openNavigation} | ||
| onClick={() => setNav({ open: !open })} | ||
| sx={{ |
There was a problem hiding this comment.
component="a": ✅ good catch
functional setter: passing for now - YAGNI?
jshearer
left a comment
jshearer
left a comment
There was a problem hiding this comment.
LGTM % a couple of optional nits
| width, | ||
| height: '100%', | ||
| width: open ? NavWidths.FULL : NavWidths.RAIL, | ||
| flexShrink: 0, |
There was a problem hiding this comment.
The old permanent-Drawer paper was position: fixed at zIndex.drawer (1200), so it stacked above other fixed chrome. This plain Box has no z-index, so the AgentSkills Toast (position: fixed; right: 24; bottom: 24; width: 380; zIndex: 175, shown to every user until dismissed) now paints over the bottom of the nav on windows narrower than ~604px — covering the collapse toggle and ModeSwitch and stealing their clicks (the toast root has its own onClick).
Low severity since it only bites below the sm breakpoint, but it is a regression vs. the Drawer. A zIndex on this Box (or repositioning the toast) would restore the old stacking.
| transition: (boxTheme) => | ||
| `${boxTheme.transitions.duration.shortest}ms`, |
There was a problem hiding this comment.
Since this line is being rewritten anyway, worth cleaning up the carried-over Drawer idiom: a bare `${...}ms` shorthand means transition-property: all, so the background two lines up also animates on theme-mode switch, and the boxTheme callback duplicates the theme from useTheme() already in scope.
| transition: (boxTheme) => | |
| `${boxTheme.transitions.duration.shortest}ms`, | |
| transition: theme.transitions.create('width', { | |
| duration: theme.transitions.duration.shortest, | |
| }), |
matches the pattern used elsewhere (e.g. WizardContent.tsx).
GregorShear
force-pushed
the
claude/code-health-checks
branch
4 times, most recently
from
a4152a1 to
604416d
Compare
GregorShear
force-pushed
the
claude/nav-drawer-to-box
branch
from
cb9661b to
521e1bd
Compare
The old drawer-style navigation was abandoned (by setting a drawer prop
variant="permanent") but the MUI drawer component was left in place.This PR simplifies navigation and layout by replacing Drawer with Box (keeping the existing expand/collapse behavior that was bolted onto the old Drawer).