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

feat(modal): add support for bindToController #3965

Closed
wants to merge 1 commit into from
Closed

feat(modal): add support for bindToController #3965

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/modal/docs/readme.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ The `$modal` service has only one method: `open(options)` where available option
* `scope` - a scope instance to be used for the modal's content (actually the `$modal` service is going to create a child scope of a provided scope). Defaults to `$rootScope`
* `controller` - a controller for a modal instance - it can initialize scope used by modal. Accepts the "controller-as" syntax in the form 'SomeCtrl as myctrl'; can be injected with `$modalInstance`
* `controllerAs` - an alternative to the controller-as syntax, matching the API of directive definitions. Requires the `controller` option to be provided as well
* `bindToController` - when used with `controllerAs` & set to `true`, it will bind the controller properties onto the `$scope` directly
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

onto the $scope or onto this. ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As it says, $scope. If you see the implementation, it becomes instantly clear. This is similar to how bindToController works in Angular for directives with isolate scope.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, you're right.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that you understood bindToController in the other way around:

https://docs.angularjs.org/api/ng/service/$compile

bindToController: When an isolate scope is used for a component (see above), and controllerAs is used, bindToController: true will allow a component to have its properties bound to the controller, rather than to scope.

Bind to controller enforces the use of controllerAs, and it publishes the controller into the scope as it happens since 1.2. The new point is that all bindings to the directive are made towards the controller instead of towards the scope, but the scope does not dissappear, and you always use in template: ctrl.property.

The problem in angular 1.X is that you need to use the dot notation because you have a high risk (specilly with you work with non expert programmers) to mess it up with double bindings and nested scopes, for instance:

Value: {{vm.value}}
<div ng-if="vm.isInputActive()">
  <input ng-model="vm.value">
</div>

vs.

Value: {{value}}
<div ng-if="vm.isInputActive()">
  <input ng-model="value">
</div>

Which will not work as expected according the current implementation proposed in this thread: it will save the value in the scope created by ngIf but not in the directive context.

So, one of two:

  1. bindToController does not make sense at all here, there are no bindings to the modal, so better to remove it (my recommendation)

  2. ok, we want to bind controller properties to the scope, although it is dangerous, but we should name another name like: controllerPropertiesInScope, to avoid confusions with angular meaning

And according the presented implementation, it does not uses the controller instance at all, it is just a temporary object which instance reference is lost in favor to scope:

8adfc83

angular.extend(modalScope, ctrlInstance);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check #4054 :)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

It happened to me looking for ways to make "mastermodals" that I can accomodate for many uses. Ex:

  • A generic PurchaseModal which is extended by MacaroonsPurchaseModal in which you for example need to request remote macaroons colors and prices and pass it to the directive or whatever to choose macaroon, select a price and go ahead with purchase.

The main problem is how to extend specific existing controller with some nice locals. In such case something like but different to bindToController would be useful. For example binding resolve results to controller.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please continue this conversation on #4054.

* `resolve` - members that will be resolved and passed to the controller as locals; it is equivalent of the `resolve` property for AngularJS routes
* `animation` - set to false to disable animations on new modal/backdrop. Does not toggle animations for modals/backdrops that are already displayed.
* `backdrop` - controls presence of a backdrop. Allowed values: true (default), false (no backdrop), `'static'` - backdrop is present but modal window is not closed when clicking outside of the modal window.
Expand Down
6 changes: 5 additions & 1 deletion src/modal/modal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -464,7 +464,11 @@ angular.module('ui.bootstrap.modal', [])

ctrlInstance = $controller(modalOptions.controller, ctrlLocals);
if (modalOptions.controllerAs) {
modalScope[modalOptions.controllerAs] = ctrlInstance;
if (modalOptions.bindToController) {
angular.extend(modalScope, ctrlInstance);
} else {
modalScope[modalOptions.controllerAs] = ctrlInstance;
}
}
}

Expand Down
8 changes: 8 additions & 0 deletions src/modal/test/modal.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -440,6 +440,14 @@ describe('$modal', function () {
}, controllerAs: 'test'});
expect($document).toHaveModalOpenWithContent('Content from ctrl true', 'div');
});

it('should allow usage of bindToController', function () {
open({template: '<div>{{fromCtrl}} {{isModalInstance}}</div>', controller: function($modalInstance) {
this.fromCtrl = 'Content from ctrl';
this.isModalInstance = angular.isObject($modalInstance) && angular.isFunction($modalInstance.close);
}, controllerAs: 'test', bindToController: true});
expect($document).toHaveModalOpenWithContent('Content from ctrl true', 'div');
});
});

describe('resolve', function () {
Expand Down