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

Add $resolve to uibModal scope #5808

Closed
radarfox opened this issue Apr 14, 2016 · 22 comments
Closed

Add $resolve to uibModal scope #5808

radarfox opened this issue Apr 14, 2016 · 22 comments

Comments

@radarfox
Copy link

radarfox commented Apr 14, 2016

Hello,

it would be super, if modal plugin would support new $resolve property same as angular-route does.

See example here (search for "$resolve") https://docs.angularjs.org/api/ngRoute/service/$route

It makes usage with components much easier, because there is no need for "modal controller", just the "component controller":

$uibModal.open({
    template: '<my-component data-foo="$resolve.bar"></my-component>',
    resolve: {
        bar: 'Hello world!'
    }
});

Then in component just add bindings, and everything is set:

var myComponent = {
    template: '<p data-ng-bind="$ctrl.foo"><p>',
    bindings: {foo: '<'}
};

All I had to do, was to add 2 new lines in your source:

var modalScope = providedScope.$new();
modalScope.$close = modalInstance.close;
modalScope.$dismiss = modalInstance.dismiss;
modalScope.$resolve = tplAndVars[1]; // here

and

if (modalOptions.bindToController) {
  ctrlInstance.$close = modalScope.$close;
  ctrlInstance.$dismiss = modalScope.$dismiss;
  ctrlInstance.$resolve = modalScope.$resolve; // here
  angular.extend(ctrlInstance, providedScope);
}

Plunker http://plnkr.co/edit/UtTBUZoztzVS8A5jcoQq

Example in angular-route https://docs.angularjs.org/guide/component#components-as-route-templates

@deeg
Copy link
Contributor

deeg commented Apr 14, 2016

@radarfox, are you planning to fill out the issue description or was this created by mistake?

@radarfox radarfox changed the title $uibModal Add $resolve to $uibModal Apr 14, 2016
@icfantv
Copy link
Contributor

icfantv commented Apr 14, 2016

closing due to lack of content.

@icfantv icfantv closed this as completed Apr 14, 2016
@radarfox
Copy link
Author

radarfox commented Apr 14, 2016

Sorry, i accidentaly pressed enter before entering description.

You are too fast for me :)

Now is all set, could you reopen?

@icfantv
Copy link
Contributor

icfantv commented Apr 14, 2016

@radarfox, i'm not seeing a $resolve property in the docs page you linked. regardless, if this is an angular 1.5.x thing, we are not yet ready to require angular 1.5+.

@deeg
Copy link
Contributor

deeg commented Apr 14, 2016

@radarfox, is this somehow different than the scope property you can pass into open?

scope (Type: $scope) - The parent scope instance to be used for the modal's content. Defaults to $rootScope.

@radarfox
Copy link
Author

@icfantv It is located near the end of page - Properties -> current

Well it is not 100% Angular 1.5. It is just adding resolved object to modal scope. It is not requiring any 1.5 functionality.

@radarfox
Copy link
Author

@deeg no, it should be bound to modal scope - child scope of scope passed in options. It is the same scope where $close and $dismiss is bound to.

@icfantv
Copy link
Contributor

icfantv commented Apr 14, 2016

@radarfox do you mean resolveAs? Also, I don't see this option in 1.4.x so it appears to be a 1.5+ thing.

@deeg
Copy link
Contributor

deeg commented Apr 14, 2016

@deeg no, it should be bound to modal scope - child scope of scope passed in options. It is the same scope where $close and $dismiss is bound to.

I guess I'm still confused on the difference between scope and resolve options your propose. Could you maybe set up a plunker with how you want resolve to work so we can better understand?

@radarfox
Copy link
Author

@icfantv well resolveAs is just another enhancement for this use-case. It lets you choose $resolve property name. So if you dont like modalScope.$resolve you can change it to modalScope.myResolve by passing resolveAs: "myResolve" in options.

@radarfox
Copy link
Author

@deeg here it is http://plnkr.co/edit/UtTBUZoztzVS8A5jcoQq

If support for $resolve would be added, then there would be no need for $uibModal.open controller and you can pass data to component very fast without extra code for each modal.

@radarfox radarfox changed the title Add $resolve to $uibModal Add $resolve to uibModal scope Apr 14, 2016
@radarfox
Copy link
Author

Also ui-router implemented support for this - https://github.com/angular-ui/ui-router/releases/tag/1.0.0-alpha.1 (see the first paragraph of "Route to component")

And i found more detailed info on angular-route https://docs.angularjs.org/guide/component#components-as-route-templates

@deeg is this enought info for you?

@deeg
Copy link
Contributor

deeg commented Apr 14, 2016

@radarfox, is there a reason something like this wouldn't work?

That uses the already in place scope option instead of the newly created resolve option.

Sorry if I'm missing something completely.

@radarfox
Copy link
Author

@deeg no, because that would not work with promises, like resolving does.

@wesleycho
Copy link
Contributor

I can't say I love this suggestion, but I know ngRoute and UI Router have both went this route.

For parity, it may be better for us to just support this.

@deeg
Copy link
Contributor

deeg commented Apr 14, 2016

@radarfox, I see. None of the examples you provided showed promises that is why I was confused.

Agree with you now though!

@radarfox
Copy link
Author

I hope it is clear that this is 100% backwards compatible enhancement, which takes only 2 lines of code to implement.

I see no performance or compatibility problems.

As a result you save a lot of code, when using component aproach with multiple modals.

@deeg
Copy link
Contributor

deeg commented Apr 14, 2016

@radarfox, yes that is clear. Feel free to open a PR and we can review it and merge it in.

@wesleycho
Copy link
Contributor

It actually has a bit of negative perf implications, since the dirty checking loop becomes longer, but it seems that those concerns are fairly weak, or weak enough that this was not a strong point of consideration for both the Angular and UI Router teams.

@wesleycho
Copy link
Contributor

@pcalouche there were multiple things wrong with your Plunker - see here. In the future, please do not open issues/pollute existing issues for what are support issues.

@radarfox
Copy link
Author

radarfox commented Jun 6, 2016

@wesleycho Thanks for implementation and release.

The only disadvantage is that you also need controller, so I end up with dummy controller: angular.noop.

Why did you implement it differently from how $close and $dismiss already work?

@radarfox
Copy link
Author

radarfox commented Jun 6, 2016

@wesleycho also angular-route does not require controller to be present.

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

No branches or pull requests

4 participants