emilkowalski / vaul

A drawer component for React.
https://vaul.emilkowal.ski
MIT License
6.42k stars 215 forks source link

Nested drawer doesn't fire onOpenChange callback #457

Open the-dream-machine opened 1 month ago

the-dream-machine commented 1 month ago

The <Drawer.NestedRoot/> component doesn't fire the onOpenChange callback.

Example nested drawer

const NestedDrawer = () => {
  return (
    <DrawerPrimitive.NestedRoot
      onOpenChange={() => console.log("Nested drawer's open state changed")}
    >
      <DrawerTrigger asChild>
        <Button>Open nested drawer</Button>
      </DrawerTrigger>

      <DrawerContent>
        <DrawerHeader>
          <DrawerTitle>Nested title</DrawerTitle>
          <DrawerDescription>Nested description</DrawerDescription>
        </DrawerHeader>
      </DrawerContent>
    </DrawerPrimitive.NestedRoot>
  )
}

Example Parent drawer

const ParentDrawer = () => {
  return (
    <DrawerPrimitive.Root
      onOpenChange={() => console.log("Parent drawer's open state changed")}
    >
      <DrawerTrigger asChild>
        <Button>Open parent drawer</Button>
      </DrawerTrigger>

      <DrawerContent>
        <DrawerHeader>
          <DrawerTitle>Parent title</DrawerTitle>
          <DrawerDescription>Parent description</DrawerDescription>
        </DrawerHeader>

       <NestedDrawer/>

      </DrawerContent>
    </DrawerPrimitive.Root>
  )
}

I'm on "@remix-run/dev": "2.10.2" and "vaul": "0.9.4"

lgraubner commented 1 month ago

I noticed this as well for controlled nested drawers. On v0.9.2 it works, but the parent drawer does not reset scaling on close. Since v0.9.4 it's not possible to use a nested drawer in controlled mode at all (as onOpenChange is not triggered).

the-dream-machine commented 1 month ago

I can confirm this is still an issue in v.1.0.0. As @lgraubner mentioned, scaling is also not reset on close. Perhaps you should consider re-implementing this component with state machines. The functionality is too complex to read/track all the possible states. A well modeled state machine will eliminate a whole class of state related bugs like this one.

nahasco commented 3 weeks ago

I can also confirm this is an issue. Waiting for a big fix...