Pin sidebar footer always visible (fixes #623)#636
Conversation
Add height: 100vh/100dvh to sidebar so footer stays visible regardless of nav content length. Nav section scrolls independently with overflow-y: auto; footer uses flex-shrink: 0. Closes #623
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Self-reviewDiff reviewed: 3 CSS property additions in What the change does
Compatibility check
Potential concerns reviewed
VerdictChange is minimal, correct, and safe. No issues found. |
There was a problem hiding this comment.
Code Review
This pull request updates the sidebar layout by defining a fixed viewport height, enabling vertical scrolling for the navigation area, and ensuring the footer does not shrink. The reviewer provided feedback regarding potential layout issues when the main content scrolls, suggesting the use of sticky positioning. Additionally, there are recommendations to verify mobile responsiveness and adjust padding in the collapsed state to prevent scrollbars from clipping icons.
| height: 100vh; | ||
| height: 100dvh; |
There was a problem hiding this comment.
The sidebar is set to a fixed height of 100vh, but the parent container (.td-shell in AppShell.vue) uses min-height: 100vh. If the main content is long enough to make the page scroll, the sidebar will scroll away with the page, and its background will not extend to the bottom of the document. To ensure the sidebar and its footer remain truly "pinned" to the viewport while the content scrolls, consider using position: sticky with top: 0.
position: sticky;
top: 0;
height: 100vh;
height: 100dvh;
| .td-sidebar { | ||
| width: var(--td-sidebar-width); | ||
| height: 100vh; | ||
| height: 100dvh; |
There was a problem hiding this comment.
On mobile devices (max-width: 640px), the sidebar uses position: fixed with top: 0 and bottom: 0. Adding an explicit height to the base class will override the bottom: 0 property in most browsers. While 100dvh is generally accurate, bottom: 0 is often more resilient for handling browser UI changes and virtual keyboards. Since the mobile media query is not part of this diff, please ensure that height is set to auto or initial in the mobile styles to preserve the existing bottom: 0 behavior.
|
|
||
| .td-sidebar__nav { | ||
| flex: 1; | ||
| overflow-y: auto; |
There was a problem hiding this comment.
Adding overflow-y: auto to the navigation container will cause a scrollbar to appear when items overflow. In the collapsed state (56px width), the custom 8px scrollbar (defined in style.css) will reduce the available horizontal space. Given the current padding (var(--td-space-5)) and icon width (20px), the icons will likely be pushed off-center or clipped when the scrollbar is visible. Consider reducing the horizontal padding for .td-nav-item specifically for the collapsed state to maintain visual balance.
Address Gemini review: sticky positioning keeps sidebar pinned while content scrolls. Mobile resets height to auto so the existing position: fixed + bottom: 0 behavior is preserved.
Summary
height: 100vh(with100dvhfallback) to.td-sidebarso the sidebar fills the viewport height on desktopoverflow-y: autoto.td-sidebar__navso navigation scrolls independently when items overflowflex-shrink: 0to.td-sidebar__footerto prevent the footer from being compressedThe footer (keyboard shortcuts help + logout) now stays pinned at the bottom of the sidebar regardless of how many nav items are present.
Closes #623
Test plan
position: fixedwithtop/bottom: 0)