Dialog component has no default onRequestClose behavior

[aweary]

This isn't documented at all, so either it should be changed or documented. Currently _onRequestClose
calls this.props.onRequestClose if it exists, but does nothing otherwise.

https://github.com/callemall/material-ui/blob/master/src/dialog.jsx#L371

 _requestClose(buttonClicked) {

    if (!buttonClicked && this.props.modal) {
      return;
    }

    if (this.props.onRequestClose) {
      this.props.onRequestClose(!!buttonClicked);
    }
  }

It seems the component should just close the the Dialog by default with _onRequestClose if there is no this.props.onRequestClose. This way the expected behavior (closing when requested to be closed) is the default and if the user needs to implement more advance logic then they can.

I can get a PR for this if you guys agree.

[alitaheri]

We have 2 solutions:

1. Leave it as it is but provide documentation.
2. Make Dialog support both modes of Controlled and Uncontrolled, just like LeftNav.

However, there is a problem with solution 2. If the component is to be Uncontrolled there are no "User Actions" on the component that can trigger it to pop. That means we will have to undeprecate open(). Otherwise, we should provide two-way binding which is imo an anti-pattern. Besides, I don't think handling this single callback is so hard on the user. @aweary Do you think if the documentations clearly stated the usage, you would be totally fine with this? Or do you believe the Dialog should automatically handle it's closing?

[oliviertassinari]

there are no "User Actions" on the component that can trigger it to pop

@subjectix Thanks, now, I remember why it's gone.
Yeap, I think that this component should only be controlled, hence we should probably don't touch it and go for option 1.

[alitaheri]

Ok. Now do you think we should differ any improvements to the docs until we entirely revise how they are structured and generated? Or this takes priority?

[oliviertassinari]

until we entirely revise how they are structured and generated

I haven't started yet. No need to differ.

[alitaheri]

OK then.

[aweary]

@subjectix I don't think it's an unreasonable request for the user if documented. But it seems pretty easy to just close the dialog on the in _onRequestClose If no this.props.onRequestClose is provided.

That would mean that built in close requests work (like clicking on the overlay) and if the User Actions need to close the Dialog then the user can go ahead and implement that.

[aweary]

Though to be fair I'm not totally aware of the consequences of controlled vs uncontrolled in MUI so I could be wrong.

[oliviertassinari]

@aweary We are more or less following their API https://github.com/rackt/react-modal.

[alitaheri]

But then how would the value provided via the open prop be updated? The Dialog component cannot know where that data comes from (and it shouldn't). I mean this seems a little inconvenient, but trust me, it's worth the trade-off.
In my opinion: Inconvenient Declarative API > Convenient Imperative API

[aweary]

That's fair, I think documenting this behavior is best then 👍