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

use objects as bindToController option for modals #5039

Closed
matthieusieben opened this issue Dec 9, 2015 · 14 comments
Closed

use objects as bindToController option for modals #5039

matthieusieben opened this issue Dec 9, 2015 · 14 comments

Comments

@matthieusieben
Copy link

We should be able to set bindToController as an object, which will be bound to the controller instead of the scope variables.

@matthieusieben
Copy link
Author

The bindToController option is actually broken:

Using bindToController in modals requires to also use the scope option. But then, a new child scope is created:

var modalScope = (modalOptions.scope || $rootScope).$new();

Then, the variables are bound to the controller using:

angular.extend(ctrlInstance, modalScope);

Bunt since modalScope is a child scope, it does not contain any own keys, making this feature impossible to work.

Using the following would fix the issue.

angular.extend(ctrlInstance, modalOptions.bindToController);

@Foxandxss
Copy link
Contributor

We will support 1.5 when it gets out. Not a priority right now.

@matthieusieben
Copy link
Author

Please dont mind the poor title choice and read my second comment. bindToController is broken and does not work.

@matthieusieben matthieusieben changed the title bindToController in modals should be 1.5 compliant use objects as bindToController option for modals Dec 9, 2015
@wesleycho
Copy link
Contributor

Can you point to examples/documentation on this? Keep in mind, we are highly unlikely to replicate isolate $scope creation in the modal itself.

@matthieusieben
Copy link
Author

Juste try to use bindToController as defined in the official doc. It won't work.

Here is why.

During the modal creation, a new child scope is created. This child scope does not contain any own variables (even though parent scope variables are available in the partials, through the $parent chain). A few lines later, if the option bindToController is truthy, the ctrlInstance is extended with the (still empty) modalScope. This last operation has no effect whatsoever because the child scope has no own property and angular.extend does not copy properties from $parent scopes. Just read the code, it's all written in there.

@wesleycho
Copy link
Contributor

Link to/quote the official docs where it says it - I saw nothing on the use of an object, and as far as I am aware, the use of an object with bindToController is for isolate scope binding declarations which does not apply.

Also we have a unit test asserting that the usage as a boolean does work as intended, so more evidence-based proof is needed since you are asserting the opposite.

@wesleycho
Copy link
Contributor

You are not using a controller - bindToController is meaningless without a controller, as per Angular's own documentation here.

In addition, bindToController clearly only supports an object notation for isolate scope bindings in $compile, as can be seen here. It also clearly only supports bindToController when controller and controllerAs is defined.

This appears to be a non-issue.

@matthieusieben
Copy link
Author

There is no doc about using bindToController with an object (in bootstrap ui). There is only doc about setting bindToController to true. But this does nothing. Hence my proposal to use an object (which would make bootstrap-ui more angular 1.5 compliant) instead of a useless boolean.

@matthieusieben
Copy link
Author

Please read again the plunker, I forgot to add the controller.

@matthieusieben
Copy link
Author

I have spend too much time trying to help you improve your (poorly written, poorly supported) angular module. You will probably be glad to know that I will never use this module again. Sayonara.

@matthieusieben
Copy link
Author

Just read your test. How can you be content with this kind of code? It does not event test the feature it is supposed to. Only that adding a random option does not generate an error. No wonder why you have so little open issues.
https://github.com/angular-ui/bootstrap/blob/master/src/modal/test/modal.spec.js#L665

@wesleycho
Copy link
Contributor

I am willing to help, but you are not very clear with your explanations (some of them were incorrect or incomplete), and your attitude is frankly lacking. Try being professional and putting together well-researched reproductions and clear explanations up front before putting together a stream of hard to parse thoughts where I have to try to predict what you are intending to say and do research in source code to figure out the intention of meaning - it is a waste of everyone's time, and probably one of the largest causes of open source slowdown.

I do this in my spare time, and should not have to deal with this lack of professionalism & entitlement while providing this free service.

@wesleycho
Copy link
Contributor

@matthieusieben try the change in #5048 - this should fix the issue.

We are trying to get a release by the end of the year, so this fix should be live in our 1.0 release then.

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

Successfully merging a pull request may close this issue.

3 participants