feat(modal): allow interactions with components passed as content

[pkozlowski-opensource]

Closes #861

[jnizet]

I have some questions regarding this design:

• why is the name attribute in the demo decorated with @Input()? This seems unnecessary, and even couter-intuitive since it kind of implies that the framework will populate this attribute, whereas it must actually be set explicitly by the developer code. Moreover, the value of this input is not available in ngOnInit like for traditional inputs, changes do not trigger ngOnChanges calls, etc., so it's really not an input in the angular meaning, IMHO.
• if that's how we pass values to the modal inner component, as opposed to populating a data field of the injectable NgbActiveModal service, isn't there at least a way to make the open() method and the NgbModalRef class generic, so that the returned component instance is no any, but is an instance of the actual type of component passed to open()? That would make the code more type-safe, and would allow code completion, etc.

[deepu105]

LGTM

[pkozlowski-opensource]

why is the name attribute in the demo decorated with @input()? This seems unnecessary, and even couter-intuitive since it kind of implies that the framework will populate this attribute, whereas it must actually be set explicitly by the developer code. Moreover, the value of this input is not available in ngOnInit like for traditional inputs, changes do not trigger ngOnChanges calls, etc., so it's really not an input in the angular meaning, IMHO.

This is the biggest issue with dynamically created components, in general. IMO we can add or remove @Input and it won't matter much as a member marked with @Input is still a member. The idea was to show how you can use a pre-existing component in a modal but then again, I'm totally cool with removing it.

isn't there at least a way to make the open() method and the NgbModalRef class generic, so that the returned component instance is no any, but is an instance of the actual type of component passed to open()? That would make the code more type-safe, and would allow code completion, etc.

TBH it is beyond my current TS knowledge. Feel free to build on top of this PR to add type safety.

[jnizet]

The idea was to show how you can use a pre-existing component in a modal

That's my biggest fear with this design: I fear that people use components not intended to be used in a modal originally, and then complain to us because the component uses standard lifecycle callbacks on inputs and they don't work as usual. But you're in a better position than I am to choose the right solution.

Feel free to build on top of this PR to add type safety

I'll try doing that when I have some time. But I'm not sure it's even possible.

[pkozlowski-opensource]

I fear that people use components not intended to be used in a modal originally, and then complain to us because the component uses standard lifecycle callbacks on inputs and they don't work as usual.

That's the danger, I agree. But given that there are no great solutions I prefer to err on the flexibility side.

But you're in a better position than I am to choose the right solution.

Thnx for the trust, but I don't think we are in the position of choosing "right" solution. The way I feel about it is that we are just balancing different tradeoffs. I'm planning to take this issue with Misko and Material folks but at the same time don't want to block people from using modals.

Event if the solution proposed here isn't perfect it is simple, flexible and we are still in alpha so nothing is set in stone. Let's get some real-World feedback.

[jnizet]

This property doesn't appear in the generated doc in the demo.

[jnizet]

Regarding generic open() and NgbModalRef, here's an implementation: jnizet@75aa2b6

Advantages:

• the signature of open() is clearer (if you can understand it): you can pass a TemplateRef or a type
• NgbModalRef.componentInstance is of the right type, so setting naame instead of name, for example, is detected by the compiler.

[pkozlowski-opensource]

So, after long and painful deliberations I'm going to make an "executive decision" and merge this one as-is. Being painfully aware of all the imperfections of this PR I believe that this is a good compromise for now. Let's iterate on it.

[karptonite]

@pkozlowski-opensource You were looking for real-world feedback, so here is some.

As you say, this approach is quite flexible, but I've run into some of the issues raised above. I'm showing a form in a modal that I am building from data that has to be passed to the modal. I was passing the data in as an Input(), and, as predicted, was surprised when the OnChanges lifecycle hook wasn't triggered. I'm reevaluating my options, and may just make a buildForm function that I call directly on modalRef.componentInstance after opening the modal and setting the appropriate variables, but it feels like a pretty heavy handed solution.

But at least I'm able to use the modal for my use case, so that is a plus.

[BenjaminRichardson]

@karptonite I'm in a similar situation, and came to the same solution of writing a manual initialization function.

@pkozlowski-opensource I understand this is a limitation of Angular, so I don't think there's a better solution at the moment, but I think this would benefit from being spelled out explicitly in the documentation. Perhaps a brief warning that change detection in modal components is different from other components, similar to the warning about entryComponents? I had to read through a few Github issues before I realized what was going on.