Skip to content
This repository has been archived by the owner on May 29, 2019. It is now read-only.

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

Closed
ProLoser opened this issue Sep 8, 2013 · 8 comments
Closed

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

ProLoser opened this issue Sep 8, 2013 · 8 comments

Comments

@ProLoser
Copy link
Member

ProLoser commented Sep 8, 2013

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
Copy link
Member Author

ProLoser commented Sep 8, 2013

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
Copy link
Member

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
Copy link
Member Author

ProLoser commented Sep 8, 2013

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

Here are my confusions:

  • How do I close a modal properly? Does the controller need a handle to the generated modal object? Does this mean I have to explicitly pass it through resolve or make sure I declare the controller inside the scope of where I created the modal? Why isn't there some simple context for closing a modal from within a modal controller (or view)?
    If we put $close on the scope, then I can call it either from the view OR from within the controller: $scope.$close() etc.
  • What happened to the backdropClass option? For our login modal, we want need to style an opaque background.
  • Why are we using confusing methods like close and dismiss when we could simply use common methods like resolve and reject?
  • Can a modal object be re-used (like to re-open a modal)? Can a modal object be created in the closed state (to store only 1 object in total, and simply opening it when needed)?
  • How do I listen to a modal object being closed so I can cleanup after it? Lets say I call a method 4 times that open the same modal, I want to only open it once, so I must cache the modal, and check it's existence subsequently, and then must remove the cache when the modal closes.
  • Why do we have result and opened properties as promises? How are they different?
  • Why support only windowClass as an option? Perhaps give better access to the template used (in specific cases) so I can modify whatever DOM element I want.

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
Copy link
Member Author

ProLoser commented Sep 8, 2013

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:

  • The promise is a confusing API to leverage for this purpose
  • The promise will never be rejected
  • The controller serves as a callback for business logic
  • People should not be traversing the DOM from inside a controller or inside a promise callback
  • Instead, an event should be broadcast inside the modal to it's children (I've demoed this technique before) like how vanilla bootstrap does it
  • Directives or ui-event should then be leveraged to trigger any DOM or rendering-related behaviors.
  • Again, listening to a promise to trigger business logic instead of placing it into the controller, or listening to a promise to trigger dom logic instead of doing it in a directive, are both bad approaches

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
Copy link
Member

@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
Copy link
Member

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
Copy link

ghost commented Jul 2, 2014

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

@SKoschnicke
Copy link

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.

@angular-ui angular-ui locked and limited conversation to collaborators Sep 18, 2014
@ProLoser
Copy link
Member Author

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants