angular-ui / bootstrap

PLEASE READ THE PROJECT STATUS BELOW. Native AngularJS (Angular) directives for Bootstrap. Smaller footprint (20kB gzipped), no 3rd party JS dependencies (jQuery, bootstrap JS) required. Please read the README.md file before submitting an issue!
http://angular-ui.github.io/bootstrap/
MIT License
14.28k stars 6.73k forks source link

Add $close() and $dismiss() to the modal scope #966

Closed ProLoser closed 11 years ago

ProLoser commented 11 years ago

This way I do not have to explicitly code them myself in the controller, I can simply call them from the template. They would also be redundancies.

ProLoser commented 11 years ago

Actually, why not treat this exactly like a promise API and convert it to reject() and resolve() and provide a promise property? This way I can pass it to other code that is promise-friendly.

pkozlowski-opensource commented 11 years ago

I quite like the idea of exposing $close() and $dismiss(), it would allow people to drop creation of modal controllers for very simple cases.

At the same time I'm not sure I quite get the thing about promises. There is already a result promise exposed (as well as a promise that gets resolved when a window gets opened). Could you post a code snippet better illustrating the idea?

ProLoser commented 11 years ago

God I've been diving through the modal code and it seems so unnecessarily complicated.

Here are my confusions:

My proposal:

modalDeferred = $modal({
    // Easy access to close from within view
  template: '<div class="modal-footer"><a ng-click="$reject()">Cancel</a><a ng-click="$resolve()">Okay</a></div>',
  controller: function($scope) {
    $scope.$save = function(){
      // Easy access to close from within controller
      $http(...).then($scope.$resolve, $scope.$reject);
    }
  }
});
// Recognizeable API, Easy to close externally
modalDeferred.resolve() // close
modalDeferred.reject() // dismiss
// Reusable promise object
modalDeferred.promise.then(function(){ /* closed */ }, function(){ /* dismissed */ })

// Useful for chaining:
$http(...).then(function(response){
  if (response.property === 'some weird condition')
    return $modal({ template: 'This looks bad, want to continue? ... '}).promise;
});

I didn't bother with making it re-openable because I find this overall just makes things simpler. I can now listen to the promise and cleanup on the outer level, and there is no API confusion or learning curve.

ProLoser commented 11 years ago

I also don't get why opened was a promise. I was thinking about it and realized you were probably trying to address the issue of people executing code after the window has loaded or more accurately, after the modal has faded (animated) in. The #1 request I would then imagine to be focusing form inputs. Overall, I don't get using promises for this:

We could even develop a new ui-event directive specifically for $on (angular-centric) events, if they aren't already caught. Lets say we want to focus an input:

$modal({
  template: '<input ui-event="{opened:focus()}">' // or `ui-jq="focus" ui-watch="opened" ui-event="{opened:opened=true}"
})

People could also develop directives and place them into their modal that specifically listen to the opened event on the DOM.

pkozlowski-opensource commented 11 years ago

@ProLoser I can see that there is a lot of misunderstanding here as most of the things that you are proposing are already in place but just named differently. While we can discuss naming but you will always have situations where some people will find confusing what other people feel natural. As an example I far prefer working with promises as compared to events / callbacks. A matter of personal taste, I'm afraid.

But anyway, I once again think that there are a lot of misunderstandings: you don't have to do any cleanup, there is no need for DOM traversal etc.

I would be happy to address your concerns one by one but I think it will be easier to do so on chat, so ping me when you are available.

pkozlowski-opensource commented 11 years ago

I've discussed most of the items with @ProLoser on chat and I think we've clarified most of the issues. I've exposed $close and $dismiss methods on a scope as suggested initially in this issue.

@ProLoser sorry, didn't have time to work on the backdrop-per-instance thing, let's open a separate issue for this.

ghost commented 10 years ago

Private chat? :( was looking here for answers to that same issue.

SKoschnicke commented 10 years ago

Could you make a transcript of the chat available? I was hoping to get some information here about using the modal directive without a dedicated controller but the answers to @ProLoser s questions seem to be hidden in the chat.

ProLoser commented 10 years ago

You're referring to a private chat I had with @pkozlowski-opensource a year ago. His last comment is pretty much the summation of what we discussed.