emilkowalski / vaul

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

fix: restore position settings on drawer unmount #462

Closed maiconcarraro closed 1 month ago

maiconcarraro commented 1 month ago

In previous versions (~0.9.2) the restore position settings was executed in the useEffect unmount trigger: https://github.com/emilkowalski/vaul/blob/3510ebdfc35a67a6c7ec8ea66386dbaf0e1b941e/src/index.tsx#L347-L352

The new version is going to only restore based on the onChange, which is fine and works better for most of the situations, but it fails to restore during a redirect/click in a link inside of the drawer. https://github.com/emilkowalski/vaul/blob/5cd388cfb5d9bf1664d27fc280839562dd79297c/src/index.tsx#L172-L176

Added a new playright example: image

It only affects iphone, after redirect the body is going to still have the position fixed, preventing any scroll: image

image

I added a new solution trying to improve the unmount trigger, no changes to the onChange flow, but during the unmount if there is no drawer in the dom, then it restores covering the no-trigger of onChange , and it doesn't mess up nested drawers because of the dom check. I'm still thinking if it would be better/safer to add a setTimeout to wait any animation... during my tests the current logic was enough.

Playright final result: image

shealan commented 1 month ago

Can this be merged now? The library (and Shadcn which uses it) are broken on iPhone until it's fixed.

maiconcarraro commented 1 month ago

@shealan I'm also using shadcn, for now I'm patching it inside of the drawer root component, in case you want to use it rn:

[!WARNING]
Important this is not a final solution, only a patch if you need a fix asap in your own code

const Drawer = (props: React.ComponentProps<typeof DrawerPrimitive.Root>) => {
  React.useEffect(() => {
    return () => {
      const hasDrawerOpened = !!document.querySelector('[data-vaul-drawer]');
      if (hasDrawerOpened) return;

      Object.assign(document.body.style, {
        position: "",
        top: "",
        left: "",
        height: "",
        right: "unset",
      });
    };
  }, []);

  return (
    <DrawerPrimitive.Root
      repositionInputs={!isIphone()}
      {...props}
    />
  );
};

The object assign for document.body is based on my context, so it may require some additional work for your application if you are using different attributes, and I'm also disabiling repositionInputs for iphone only.

shealan commented 1 month ago

@maiconcarraro thank you that's super helpful. Much appreciated.