feat(modal): get all the open modal instances using the NgbModal (#3627)

[gpolychronis]

Before submitting a pull request, please make sure you have at least performed the following:

• read and followed the CONTRIBUTING.md guide.
• built and tested the changes locally.
• added/updated any applicable tests.
• added/updated any applicable API documentation.
• added/updated any applicable demos.

[benouat]

I am not sure I understand why we would need a forEach public API for that purpose...
I see 2 options in here:

• Can't we create a dynamic getter returning the open modals ? then users decide what they wan't to do.

or

• Can't we create an observable, we emit each time a modal is opened/closed. Users subscribe and do what they want with it. (I would be in favour of that one)

@maxokorokov @fbasso WDYT ?

[pkozlowski-opensource]

Agree with @benouat concerns - can we discuss more the the public API we want to expose?

[fbasso]

On my side, I agree with Benoit regarding the forEach API : it forces the developer to use a loop, so it's not flexible enough (what if we just want the number of opened modals, for example ?).

Regarding the solution, I would be be in favor of the first one, for practical reason (and considering the use case explained in #3627. With a subscribe, I guess that there would be a need to subscribe on each modal (to get the modal instance in the subscribe ?), and my feeling is it' can be painful.

On the other hand, the subscribe will allow to select modals where specific actions are required, letting the others apart.

So I have no clear idea of what to do here :)

[benouat]

With a subscribe, I guess that there would be a need to subscribe on each modal

No, I was more thinking about the equivalent of a QueryList thing. You subscribe, you always get a refreshed copy of the activeModals in one shot, an array of modals

[gpolychronis]

With a subscribe, I guess that there would be a need to subscribe on each modal

No, I was more thinking about the equivalent of a QueryList thing. You subscribe, you always get a refreshed copy of the activeModals in one shot, an array of modals

This seems a little counter-intuitive. The first time I read I though the observable would emit the ref any time a new child modal was opened.

Between the two options, I prefer the first and let the user iterate over the instances.

[benouat]

This seems a little counter-intuitive.

It might, arguably, feel counter-intuitive, but it's exactly how QueryList are working when using ViewChidlren and ContentChildren.
@gpolychronis maybe you're not familiar enough as well with Content Queries.

[benouat]

I would like that we move on with this one.
I suggest we stick to the #‍2 solution with an Observable returning a live list of active modals.

@pkozlowski-opensource Could we use directly QueryList from Angular ? Sounds like a perfect fit actually, and it seems to be part of their @‍PublicAPI

An unmodifiable list of items that Angular keeps up to date when the state of the application changes.

https://github.com/angular/angular/blob/master/packages/core/src/linker/query_list.ts

[codecov-commenter]

Codecov Report

Merging #3650 into master will increase coverage by 0.08%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #3650      +/-   ##
==========================================
+ Coverage   91.23%   91.31%   +0.08%     
==========================================
  Files         100      100              
  Lines        2988     2993       +5     
  Branches      555      555              
==========================================
+ Hits         2726     2733       +7     
+ Misses        192      191       -1     
+ Partials       70       69       -1     

Flag	Coverage Δ
#e2e	56.23% <60.00%> (+<0.01%)	⬆️
#unit	87.83% <100.00%> (+0.08%)	⬆️

Impacted Files	Coverage Δ
src/modal/modal-stack.ts	95.53% <100.00%> (+0.16%)	⬆️
src/modal/modal.ts	100.00% <100.00%> (ø)
src/modal/modal-window.ts	97.87% <0.00%> (+4.25%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d4df6f6...0d41b51. Read the comment docs.

[pkozlowski-opensource]

Could we use directly QueryList from Angular ? Sounds like a perfect fit actually,

@benouat hmmm, not sure which part of QueryList is a perfect fit here? Are you talking about notifications?

I guess I need more context as for now I'm not sure how we could take advantage of QueryList here...

[benouat]

@pkozlowski-opensource We need to maintain a "live" list of items in a way that would be a Iterable, with helpers sur as first and last and the length and a way to be notified each time the list of items is changing via observables.

Looking at it, my description is 100% matching what the class QueryList is providing. am I wrong ?

Regarding our feature, we would then maintain a QueryList<ActiveModal>
People could subscribe to the changes observable to be notified, access first and last if they do want to implement a FIFO/LIFO stack of modal...

[pkozlowski-opensource]

@benouat I see. I would rather stay away from using QueryList given my understanding where things are heading.

The main problem is that QueryList class is super-messy and mixes public and "private" APIs. To use it you would have to call methods that I consider "private" (ex. reset(...)) and those methods are likely to go away in the future. Even worst - we (Angular) are considering turning QueryList into an interface and make the implementation private - see angular/angular#29469
The other problem with the QueryList is that it emits itself on changes which might not be what you need here.

All in all I think it is safer to craft a minimal interface + impl and avoid dependency on sth that is likelly to change in the future.

[benouat]

That's exactly for this kind of information that I pinged you! Thanks 🙏

[gpolychronis]

UPDATE:

NgbModal has a new property: activeInstances
It's an observable that emits anytime a modal opens/closes. The emitted is an array of NgbModalRef.

@benouat Please review.

[benouat]

LGTM! Thanks