Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Setting dismissable=false and modal=false still triggers the component to close #164

Closed
xih opened this issue Nov 9, 2023 · 15 comments
Closed

Comments

@xih
Copy link

xih commented Nov 9, 2023

Here is the behavior that I am seeing
https://github.com/emilkowalski/vaul/assets/8076886/fb058903-fa8a-49b8-bc4b-739369be16eb

BEST CASE: what i want to happen is:

  1. click on two dots successfully in a row, causes the bottom sheet to stay open

MID CASE:

  1. if i click on two dots in a row, it closes and reopens for each dot

CURRENT CASE:

  1. if I click on two dots in a row. The first one opens, then the sheet doesn't open again.
<Drawer.Root dismissible={false} open={open} modal={false}>
   <Drawer.Portal>
      ...
   </Drawer.Portal>
</Drawer.Root>

How I"m calling the Bottomsheet:

<NonDismissBottomSheet
          open={!!selectedPointData}
          setOpen={() => {
            setSelectedIndex(null);
          }}
        />
@neallseth
Copy link

Hmm yeah, I've noticed the drawer doesn't necessarily adhere to the value passed via the open prop. My expectation was that with open={true}, the drawer would remain open indefinitely, regardless of clicks outside the drawer.

Is this intended behavior? It's possible I've overlooked something.

@cedarbaum
Copy link

I just ran into this issue as well - I think that the problem is that the underlying Radix dialog's onOpenChange handler doesn't take the open property into account. Below is a small patch that fixes this specific case, though it would be good to get input from a maintainer if this is the right fix that can be checked in.

diff --git a/src/index.tsx b/src/index.tsx
index 891a0a8..65389a2 100644
--- a/src/index.tsx
+++ b/src/index.tsx
@@ -613,6 +613,11 @@ function Root({
     <DialogPrimitive.Root
       modal={modal}
       onOpenChange={(o: boolean) => {
+        // Defer to open prop if explicitly set
+        if (openProp !== undefined) {
+          return;
+        }
+
         if (!o) {
           closeDrawer();
         } else {

@jzxhuang
Copy link

jzxhuang commented Nov 9, 2023

+1, I would expect the the open prop corresponds to the radix open prop, if set the state is forced to open/closed (externally controlled) 🤷

@emilkowalski
Copy link
Owner

Controlled state issue has been fixed in #166.

@emilkowalski
Copy link
Owner

Here is the behavior that I am seeing https://github.com/emilkowalski/vaul/assets/8076886/fb058903-fa8a-49b8-bc4b-739369be16eb

BEST CASE: what i want to happen is:

  1. click on two dots successfully in a row, causes the bottom sheet to stay open

MID CASE:

  1. if i click on two dots in a row, it closes and reopens for each dot

CURRENT CASE:

  1. if I click on two dots in a row. The first one opens, then the sheet doesn't open again.
<Drawer.Root dismissible={false} open={open} modal={false}>
   <Drawer.Portal>
      ...
   </Drawer.Portal>
</Drawer.Root>

How I"m calling the Bottomsheet:

<NonDismissBottomSheet
          open={!!selectedPointData}
          setOpen={() => {
            setSelectedIndex(null);
          }}
        />

With the setup you provided the drawer doesn't close for me upon interacting with the background. Can you provide a codesandbox demo of this issue?

@xih
Copy link
Author

xih commented Nov 9, 2023

@emilkowalski here is a codesandbox demo:
https://codesandbox.io/p/sandbox/drawer-with-scale-forked-jf899f

  1. click on Open Drawer
  2. Click on "click me" or "click me 2"

Desired behavior
3. drawer stays open with no animation

Current behavior
3. drawer closes

The actual implementation I want is similar to Google Maps bottom sheet on mobile. If i click on two points, I want it the sheet to stay open, but the content inside to switch.

Google Maps example:
https://github.com/emilkowalski/vaul/assets/8076886/33bbeff4-9138-4517-9bc8-2f20069a6e09

After you make the change can you add an example codepen with the correct props to cover this use case?

Cheers mate! Beautiful repo and thank you for open sourcing this

@xih
Copy link
Author

xih commented Nov 9, 2023

@emilkowalski one last thing, when will #166 get released on NPM?

@emilkowalski
Copy link
Owner

@xih Ok yea, I can confirm that this does work as expected after fixing #166. I'll release a new version in the next hour.

@IsaiahHarris
Copy link

@xih did this fix work for you guys?

@xih
Copy link
Author

xih commented Nov 21, 2023

@IsaiahHarris yep it works!

@emilkowalski thank you for being on top of it! 🥰 the beauty of open-source =))

@IsaiahHarris
Copy link

@xih what props did you use? It's not working for me in the latest version

@IsaiahHarris
Copy link

@xih I see, you don't have snap points. If you add snap points you're able to dismiss it

@xih
Copy link
Author

xih commented Nov 21, 2023

@IsaiahHarris

here are my props

 <Drawer.Root
      open={open}
      onClose={onClose}
      modal={false}
      snapPoints={[0.3, 0.6, 0.95]}
      activeSnapPoint={snap}
      setActiveSnapPoint={setSnap}
    >
   

let me know if you have any issues!

@IsaiahHarris
Copy link

IsaiahHarris commented Nov 21, 2023

@xih what version are you on? I can still close the drawer with those props and dismissible={false} on 0.7.9

@IsaiahHarris
Copy link

IsaiahHarris commented Nov 21, 2023

@xih your props that you posted does not have dismissible={false}.

What does your onClose function look like?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants