[bug]: Sidebar seems incompatable with a header navigation

youngclaude commented 1 week ago

Describe the bug

I am building a dashboard and desire to have a header nav bar as well as the sidebar.

Problem:

Ideal:

All examples show the popular use case of a sidebar under a header.

Please let me know if my understanding on this is correct. As i write it may be the case: 1. this pattern may be redundant as the side bar may need to be used for primary navigation. 2. I may need to incorporate the site logo in the head of the sidebar

Affected component/components

Sidebar

How to reproduce

Set up a new next.js application
clear out the default page
add full width header with a logo on the far left and a login button on the far right
implement the header as presented in the documentation

The sidebar overlaps the header covering the site logo vs being a in the div below on the left side with the content on the right side all under the header nav.

Codesandbox/StackBlitz link

https://stackblitz.com/edit/stackblitz-starters-vqqoj5

Logs

No response

System Info

Stackblitz on Firefox and Chrome

Before submitting

[X] I've made research efforts and searched the documentation
[X] I've searched for existing issues

shadcn commented 6 days ago

This is expected with the fixed sidebar (not a bug). Let me add an example for doing navbar and sidebar.

Jacksonmills commented 6 days ago

Hey @youngclaude and @shadcn,

I attempted the header and sidebar integration based on the discussion: https://shadcn-10-30-24.vercel.app/

I made several modifications to the Sidebar component to resolve the overlapping issue with the header, and the solution seems to work effectively for my setup. Here are the key adjustments I implemented:

Added a top offset to the Sidebar:
```
<Sidebar className="top-[--header-height]" variant="inset" {...props}>
```
This ensures the sidebar is positioned below the header by offsetting it based on the header's height, preventing any overlap.

Defined the header height as a CSS variable within the body tag:

<body
 className={`${geistSans.variable} ${geistMono.variable} antialiased overflow-hidden`}
 style={{
   "--header-height": HEADER_HEIGHT,
 } as React.CSSProperties}
>

This allows us to reference the header height dynamically across different components.

Modified SidebarInset to factor in the header height:
```
<SidebarInset className="h-full peer-data-[variant=inset]:min-h-[calc(100svh-theme(spacing.4)-var(--header-height))]">
```
The SidebarInset adjustment ensures that the sidebar height takes into account the space occupied by the header, maintaining proper positioning without overlap.
Modified ScrollArea to factor in the double header height:
```
<ScrollArea className="h-[calc(100svh-theme(spacing.4)-(var(--header-height)*2))]">
 {children}
</ScrollArea>
```
The ScrollArea uses header-height * 2 because of the inner header within the SidebarInset, ensuring that the scrollable content respects both the main header and the additional inset header.
Set a high z-index for the NavigationMenu within the root layout header:
```
<NavigationMenu className="z-20">
```
This ensures that the navigation menu is displayed above the sidebar when hovering over the top header, even if the sidebar is open.

⚠️ These modifications might vary slightly depending on the sidebar variant being used. For this example, I used the 'inset' variant, so behavior may differ with other variants.

You can check out the repository for further details: shadcn-10-30-24.

If any of these adjustments seem useful, feel free to incorporate them into the project or let me know if you need further details. I'd be happy to collaborate or refine any part of this solution.

youngclaude commented 5 days ago

@Jacksonmills Thanks for you're time on this. Fixed header + (fixed sidebar - header height) ≅ gives us the intended dashboard effect. I'll float another idea: there may be an opportunity to handle navigation entirely by taking in a nav bar as a prop/ child for the a parent 'Group'/ 'Provider' component.

Precedence for nesting UI elements already exists in ShadCN in elements like the RadioGroup, ResizablePanelGroup and TooltipProvider.

These elements show support for the pattern of not just deriving a stand-alone component but using a parent provider to ensure desired behavior of child components.

An example solution, using @Jacksonmills 's example, could be:

<ExperimentalNavigationProvider
   header={<MyHeader />}
   sidebar={ <MySidebar /> }
   body={ children }

   bodyVerticalScroll={true}
   gapSpacing={4}
/>

A solution like this would be great for dashboards, documentation pages and admin panel use-cases. I can add a feature request of @shadcn has interest.

shadcn-ui / ui