shadcn-ui / ui

Beautifully designed components that you can copy and paste into your apps. Accessible. Customizable. Open Source.
https://ui.shadcn.com
MIT License
75.09k stars 4.69k forks source link

[bug]: Sidebar has the wrong background color in mobile dark mode (specificity issue) #5752

Open Gascon1 opened 2 weeks ago

Gascon1 commented 2 weeks ago

Describe the bug

The sidebar in mobile dark mode uses the default sheet background color rather than the sidebar's background color because of a specificity issue:

image

This is the line causing the issue:

    if (isMobile) {
      return (
        <Sheet open={openMobile} onOpenChange={setOpenMobile} {...props}>
          <SheetContent
            data-sidebar="sidebar"
            data-mobile="true"
            className="w-[--sidebar-width] bg-sidebar p-0 text-sidebar-foreground [&>button]:hidden" // <--- here
            style={
              {
                "--sidebar-width": SIDEBAR_WIDTH_MOBILE,
              } as React.CSSProperties
            }
            side={side}
          >
            <div className="flex h-full w-full flex-col">{children}</div>
          </SheetContent>
        </Sheet>
      )
    }

and the fix is here:

className="w-[--sidebar-width] bg-sidebar p-0 text-sidebar-foreground dark:bg-sidebar [&>button]:hidden"

I intend on opening a PR for this issue

Link to the PR: #5753

Affected component/components

Sidebar

How to reproduce

  1. Look at the sidebar in desktop mode, bg-sidebar is applied
  2. Look at the sidebar in mobile mode, bg-sidebar is overriden by the default bg color in dark mode

Codesandbox/StackBlitz link

No response

Logs

No response

System Info

No relevant info here

Before submitting

Jacksonmills commented 2 weeks ago

hm I cannot seem to reproduce... @Gascon1 can you provide more details on your setup? How is your dark theme done?

Example showing --sidebar-background working correctly:

image

Jacksonmills commented 2 weeks ago

okay I was able to reproduce your issue, however it looks as though it might be something you must have added to SheetContent.

Can you confirm that you have dark:bg-zinc-950 either in SheetContent or sheetVariants?

image

Gascon1 commented 2 weeks ago

Can confirm that I have dark:bg-zinc-950 on the sheetVariants:

image

I think the root cause of the issue is due to the fact that I don't use cssVariables in components.json so there's a clash in specificity since the sidebar uses thoses

To be clear it's not something I added manually, it's what gets automatically generated when you don't use cssVariables from the components.json.

My components.json:

{
  "$schema": "https://ui.shadcn.com/schema.json",
  "style": "default",
  "rsc": true,
  "tsx": true,
  "tailwind": {
    "config": "tailwind.config.ts",
    "css": "app/globals.css",
    "baseColor": "zinc",
    "cssVariables": false,
    "prefix": ""
  },
  "aliases": {
    "components": "@/components",
    "utils": "@/lib/utils",
    "ui": "@/components/ui",
    "lib": "@/lib",
    "hooks": "@/hooks"
  }
}
Jacksonmills commented 2 weeks ago

It looks like the issue might stem more from your specific setup than from shadcn/ui's default behavior. In shadcn/ui, theming is handled with a single dark class on the root layout, which dynamically adjusts CSS variables like --background across the entire app through bg-background. This means you don’t need to use dark: classes for individual components—shadcn/ui's global CSS variables take care of that.

In your setup, because you're aiming to apply a specific color using dark:, you’ll need to make that adjustment directly on the sidebar component. This is more of a tailored solution specific to your setup, rather than a default requirement in Shadcn.

However, if you’re looking for a more streamlined approach, the next section on shadcn/ui’s theming docs might offer a way to manage this through the theme settings. For example, setting "cssVariables": false could help simplify the adaptation to dark mode without needing individual overrides.

Gascon1 commented 2 weeks ago

It's a clash between the usage of cssVariables: false and how the styling of the sidebar is handled (using its own variables and not being dependent on the value of cssVariables to handle theming)

I'm fine handling this issue myself, but this impacts anyone who uses cssVariables: false

Jacksonmills commented 2 weeks ago

It's a clash between the usage of cssVariables: false and how the styling of the sidebar is handled (using its own variables and not being dependent on the value of cssVariables to handle theming)

I'm fine handling this issue myself, but this impacts anyone who uses cssVariables: false

Let me test with cssVariables: false, maybe this is just an issue with sidebar cli itself needing to either setup css variables or not.

However, looking at the docs it seems sidebar might only support css variables for now (https://ui.shadcn.com/docs/components/sidebar#theming). @shadcn can you confirm?