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

fix(modal): fix bindToController bug #5569

Closed
wants to merge 3 commits into from
Closed

Conversation

kuitos
Copy link
Contributor

@kuitos kuitos commented Mar 2, 2016

when set bindToController true,props not be bound to controller instance correctly before it initialize,this pointer not be init with $scope props

what I added in modal unit test

var $scope = $rootScope.$new(true);
$scope.foo = 'bar';
open({
  template: '<div>{{test.fromCtrl}} {{test.closeDismissPresent()}} {{test.foo}}</div>',
  controller: function($uibModalInstance) {
  + expect(this.foo).toEqual($scope.foo);
    this.fromCtrl = 'Content from ctrl';
    this.closeDismissPresent = function() {

it will make the test fail

Expected undefined to equal 'bar'.

because the props not bound to the this instance of controller correctly before the controller constructor called.

Review on Reviewable

kuitos added 2 commits March 2, 2016 17:09
when set bindToController true,props not be bound to controller instance correctly before it initialize,this pointer not be init with $scope props

ctrlInstance = ctrlInstantiate();

if (modalOptions.bindToController && angular.isFunction(ctrlInstance.$onInit)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is bindToController being required here for $onInit usage?

Copy link
Contributor

Choose a reason for hiding this comment

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

Per https://docs.angularjs.org/guide/component, in the Intercomponent Communication section, $onInit does NOT require bindToController being present.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes u are right,I had checked the source code of angular.js,it does not require controllerAs too.Pls review the latest commit.🙂

$onInit does not require controllerAs too
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants