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

Docs are not clear about where to set props when using a custom ModalOverlay with Modal #6277

Open
syedtaqi95 opened this issue Apr 26, 2024 · 5 comments
Labels
documentation Improvements or additions to documentation

Comments

@syedtaqi95
Copy link

syedtaqi95 commented Apr 26, 2024

🙋 Documentation Request

The Modal component can be optionally wrapped inside a custom ModalOverlay, for example to set a custom class on the ModalOverlay as described in the docs here.

When I used this approach, I initially set the props like isOpen, isDismissable etc. on the Modal instead of the ModalOverlay. This did not work as I intended as the Modal was not using the prop values I set (but instead may have received the prop values from the ModalContext, not 100% sure).

It took me some time to realise that I have to set the props on the ModalOverlay component instead of the Modal. It is not immediately clear to me from the docs that this is required - I think a note in the props section or custom overlay section would help future users with the same issue.

🧢 Your Company/Team

Cambustion

@yihuiliao
Copy link
Collaborator

Thanks for the feedback! We do briefly mention here that "by default, Modal includes a builtin ModalOverlay, which renders a backdrop over the page when a modal is open". Though you do have to scroll pretty far down to read it and it still might not be super obvious. I think we could probably add more to the custom overlay section so that it's less confusing.

@yihuiliao yihuiliao added the documentation Improvements or additions to documentation label May 2, 2024
@psirenny
Copy link

psirenny commented May 8, 2024

@syedtaqi95 were you able to get isDismissable to work when set on the ModalOverlay component? I've found that custom overlays don't receive click/press events.

@syedtaqi95
Copy link
Author

@psirenny yes, isDismissable works fine for me when I set it directly on the ModalOverlay.

Are you using a controlled Modal, i.e. passing isOpen to the ModalOverlay? If so, you need to make sure that isOpen is set to False when the onOpenChange event fires (as per the example in the docs here).

@psirenny
Copy link

psirenny commented May 9, 2024

It's just an ordinary uncontrolled Modal. Essentially this:

<DialogTrigger>
  <Button>Trigger</Button>
  <ModalOverlay isDismissable>
    <Modal>
      <Dialog>
        {/* … */}
      </Dialog>
    </Modal>
  </ModalOverlay>
</DialogTrigger>

Adapted from the Destructive Alert Dialog example. I noticed that the Modal component is also creating its own internal overlay (not the one specified), but it's screen hidden.

Edit: I've tried setting isDismissable on the other components as well 😛.

@syedtaqi95
Copy link
Author

@psirenny I'm not sure why it's not working for you, sorry.

FYI, I am developing my Modal in Storybook, so the component runs in an isolated iframe. This is what I am rendering in the iframe:

<div className="mx-4 flex max-w-4xl flex-col items-center gap-8">
  <DialogTrigger>
    <Button>Sign up...</Button>
    <ModalOverlay  {...props}>
      <Modal>

        <Dialog>
          {({ close }) => (
            <>
              <DialogHeading>Dialog with Button Group</DialogHeading>
              <div>
                Lorem ipsum dolor sit amet, qui minim labore adipisicing minim
                sint cillum sint consectetur cupidatat.
              </div>
              <ButtonGroup align="end" className="w-full">
                <Button variant="quiet" onPress={close}>
                  Cancel
                </Button>
                <Button colour="secondary" onPress={close} autoFocus>
                  Confirm
                </Button>
              </ButtonGroup>
            </>
          )}
        </Dialog>

      </Modal>
    </ModalOverlay>
  </DialogTrigger>

  <p className="text-justify">
    Lorem ipsum dolor sit amet, officia excepteur ex fugiat reprehenderit
    enim labore culpa sint ad nisi Lorem pariatur mollit ex esse
    exercitation amet. Nisi anim cupidatat excepteur officia.
    Reprehenderit nostrud nostrud ipsum Lorem est aliquip amet voluptate
    voluptate dolor minim nulla est proident. Nostrud officia pariatur ut
    officia. Sit irure elit esse ea nulla sunt ex occaecat reprehenderit
    commodo officia dolor Lorem duis laboris cupidatat officia voluptate.
    Culpa proident adipisicing id nulla nisi laboris ex in Lorem sunt duis
    officia eiusmod. Aliqua reprehenderit commodo ex non excepteur duis
    sunt velit enim. Voluptate laboris sint cupidatat ullamco ut ea
    consectetur et est culpa et culpa duis.
  </p>
</div>

So I am simply setting the props on the ModalOverlay which works ok for me.

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

No branches or pull requests

3 participants