RFC: Could/should the modal rely more heavily on React.Portal?

[jeffcarbs]

In react-portal there's a prop called openByClickOn (which stardust doesn't support currently) that lets you specify a trigger without requiring an empty div wrapper. So instead of:

<div>
  <Button onClick={this.show}>Basic Modal</Button>
  <Modal basic size='small' active={active} onHide={this.hide} />
</div>

You could do:

<Modal basic size='small' active={active} onHide={this.hide} openByClickOn={<Button>Basic Modal</Button>} />

It got me thinking, it seems there's quite a bit of functionality baked into React.Portal (e.g. tracking visible state, close on click outside, close on esc) that we've duplicated in the Modal component. Could we update the modal to pass portal-related props straight to the portal and remove duplicate functionality from the modal?

I recall reading a comment here recently about the desire to decouple the modal from the portal, so maybe this would take us in the wrong direction. However, this would give the end-user more control over the portal functionality and allow us to remove code that's already been written and tested elsewhere.

---

Edit I'm happy to take a pass at doing this if there's interest.

[levithomason]

IIRC at first I tried to use the portal props for open/close and close on key/click but had to implement the then within Stardust for some reason. I think it had something to do with the testing, or special SUI cases that didn't fit the portal API well? Can't recall exactly.

Anyhow, I'm game for removing code and delegating this to the portal if possible. I just think we'd rename the props to fit SUI conventions better (ie open vs isOpen).

At the very minimum, mapping trigger to openByClickOn makes perfect sense to me. I could even see using Button.create() shorthand there. Odds are, most modals triggers will be buttons:

<Modal trigger='Open modal!' />

[levithomason]

Regarding decoupling, it's a nice idea to have the Modal as a stateless component that is then wrapped up in the Portal. However, I'm doubting there are enough use cases to warrant it. We'll see how the Dimmer #447 component plays out. Its API is going to affect how the Modal/Portal relation plays out since the Portal is required to be the Modal Dimmer for SUI css to work. This could end up being a <Dimmer as={Portal} /> or something.

For now, let's keep Modal/Portal baked together. Time will tell the rest.

[jeffcarbs]

Fixed via #571