adobe / react-spectrum

A collection of libraries and tools that help you build adaptive, accessible, and robust user experiences.
https://react-spectrum.adobe.com
Apache License 2.0
13.02k stars 1.13k forks source link

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

Open syedtaqi95 opened 6 months ago

syedtaqi95 commented 6 months ago

๐Ÿ™‹ 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 commented 6 months ago

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.

psirenny commented 6 months ago

@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 commented 6 months ago

@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 commented 6 months ago

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 commented 6 months ago

@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.