feat(modal): stacked modals #2640

ghost · 2018-08-24T15:50:39Z

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.

P.S. if you think the project is not ready to have stacked modals in the demo, at least consider merging changes related to the modal-open class.

pkozlowski-opensource · 2018-08-25T06:40:27Z

src/modal/modal-backdrop.ts

@@ -3,7 +3,8 @@ import {Component, Input} from '@angular/core';
 @Component({
  selector: 'ngb-modal-backdrop',
  template: '',
-  host: {'[class]': '"modal-backdrop fade show" + (backdropClass ? " " + backdropClass : "")'}
+  host:
+      {'[class]': '"modal-backdrop fade show" + (backdropClass ? " " + backdropClass : "")', 'style': 'z-index: 1050'}


I'm surprised that we don't need to adjust z-index on windows themselves...

@pkozlowski-opensource it's ok to not adjust manually z-index.
According to W3C:

Each box belongs to one stacking context. Each positioned box in a given stacking context has an integer stack level, which is its position on the z-axis relative other stack levels within the same stacking context. Boxes with greater stack levels are always formatted in front of boxes with lower stack levels. Boxes may have negative stack levels. Boxes with the same stack level in a stacking context are stacked back-to-front according to document tree order.

So it should be fine for us.

pkozlowski-opensource · 2018-08-25T06:42:27Z

src/modal/modal-stack.ts

    activeModal.close = (result: any) => { ngbModalRef.close(result); };
    activeModal.dismiss = (reason: any) => { ngbModalRef.dismiss(reason); };

    this._applyWindowOptions(windowCmptRef.instance, options);
+    renderer.addClass(this._document.body, 'modal-open');


Could we check if there are no open modals open already and skip DOM manipulation if there is already an open modal?

Done. addClass will be called only when _modalRefs has 1 modal.

pkozlowski-opensource · 2018-08-25T06:43:58Z

@Maxkorz thnx for the PR - I did a first pass on it and it looks really good! Just left minor nit comment.

I would like to spend more time testing it but if all goes well we will land it in 3.2.0. Thnx again!

benouat

Could you please have a look ?

benouat · 2018-08-28T09:00:09Z

src/modal/modal-stack.ts


  open(moduleCFR: ComponentFactoryResolver, contentInjector: Injector, content: any, options): NgbModalRef {
    const containerEl =
        isDefined(options.container) ? this._document.querySelector(options.container) : this._document.body;
+    const renderer = this._rendererFactory.createRenderer(null, null);


Is there any specific reason you are using RendererFactory2 here to create a new renderer instance instead of injecting directly private _renderer: Renderer2 in constructor ?

@benouat this is a service so I don't think you can inject Renderer2 (it is only injectable in components)

Oh right ! my bad. I tend to always forget modal things are occurring from services

benouat · 2018-08-28T09:39:10Z

The overall code LGTM! Nonetheless I have a few comments:

Keyboard: it seems broken (or simply missing) 😄. I was just playing with the new demo, open both modals, hit Esc -> it is closing the first one! A test should be added.
Backdrops: Each new modal created has its own NgbModalBackdrop. Do we really want that behaviour ? Can't we have only one backdrop, and move it dynamically in the DOM to be in between opened modals ? Maybe it sounds kind of unnecessary optimisation ?

ghost · 2018-08-28T09:59:37Z

@benouat Thanks for the review!

Keyboard: I'll look into it. Sound like a bug to me. Nice catch!
Backdrops: With only one backdrop the appearance of stacked modals is different:
- 2 modals with two backdrops: https://i.imgur.com/4iv4HDu.png
- 2 modals with one backdrop: https://i.imgur.com/LLZ27uA.png
- 3 modals with three backgdrops: https://i.imgur.com/OGPAtX8.png
- 3 modals with one backdrop: https://i.imgur.com/XURmXrb.png
I personally prefer the darker(stacked) backdrop for multiple modals, so I really don't know what's the best solution here. Some users might not like the stacked backdrop. Should it be configurable?

ghost · 2018-08-28T12:43:31Z

@benouat Keyboard issue has been fixed.

ghost · 2018-08-28T13:28:04Z

src/modal/modal-stack.ts

+
+    ngbWindowCmpt.onDestroy(() => {
+      const index = this._windowCmpts.indexOf(ngbWindowCmpt);
+      if (index > -1) {


I copied this check from _registerModalRef. Is it really necessary to check the existence of an element before removing it from an array?

With splice()yes it is needed, because splice( negative-index-here, ...) will remove from the end, e.g. : [1, 2 , 3].splice(-1, 1) -> [1, 2]

As a sidenote, I think we could only fire this._activeWindowCmptHasChanged.next(); if we remove a componentRef. If nothing is removed it is not needed.

As a sidenote, I think we could only fire this._activeWindowCmptHasChanged.next(); if we remove a componentRef. If nothing is removed it is not needed.

I subscribe to _activeWindowCmptHasChanged in constructor in order to call ngbFocusTrap on the top/activated/latest (you get the idea) WindowCmpt. So it should fire on push, and on splice (to trap the focus again on the previously opened modal window)

I was just talking about moving the call inside the if :

ngbWindowCmpt.onDestroy(() => { const index = this._windowCmpts.indexOf(ngbWindowCmpt); if (index > -1) { this._windowCmpts.splice(index, 1); this._activeWindowCmptHasChanged.next(); } // this._activeWindowCmptHasChanged.next(); move it inside the upper if });

oh, sure, I misunderstood you.
Done

ghost · 2018-08-28T13:49:15Z

src/modal/modal.spec.ts

+           });
+         }));
+
+      it('should dismiss modals on ESC in correct order', () => {


ESC didn't work because ngbFocusTrap was preventing focus to go out of the first modal window.

I fixed it by moving ngbFocusTrap to the NgbModalStack service, but I'm not sure how to test it. The test seems right, but no matter where I call ngbFocusTrap it still passes. And I couldn't find any tests for Focus Trap, so I assume it's not testable?

Your changes look good to me. No matter where we call the ngbFocusTrap, test does not need to be changed I think.

benouat · 2018-08-28T13:58:16Z

@Maxkorz for the backdrop I don't think we need to change anything right now. Let's keep it as it, with one new backdrop per modal. We can always refactor and change it later.

benouat

One more time a very big thank you for this contribution 👍
Let's merge it.

pkozlowski-opensource added component: modal needs: review labels Aug 25, 2018

pkozlowski-opensource reviewed Aug 25, 2018

View reviewed changes

pkozlowski-opensource added this to the 3.2.0 milestone Aug 25, 2018

feat(modal): stacked modals

f7340ec

benouat reviewed Aug 28, 2018

View reviewed changes

benouat added needs: work and removed needs: review labels Aug 28, 2018

ghost commented Aug 28, 2018

View reviewed changes

fix(modal): correctly trap focus for multiple modal windows

e79e4be

benouat approved these changes Aug 29, 2018

View reviewed changes

benouat merged commit 2409572 into ng-bootstrap:master Aug 29, 2018

benouat added the type: feature label Aug 29, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(modal): stacked modals #2640

feat(modal): stacked modals #2640

ghost commented Aug 24, 2018

pkozlowski-opensource Aug 25, 2018

benouat Aug 28, 2018

pkozlowski-opensource Aug 25, 2018

ghost Aug 25, 2018

pkozlowski-opensource commented Aug 25, 2018

benouat left a comment

benouat Aug 28, 2018

pkozlowski-opensource Aug 28, 2018

benouat Aug 28, 2018

benouat commented Aug 28, 2018 •

edited

Loading

ghost commented Aug 28, 2018

ghost commented Aug 28, 2018

ghost Aug 28, 2018

benouat Aug 28, 2018

ghost Aug 28, 2018 •

edited by ghost

Loading

benouat Aug 28, 2018

ghost Aug 28, 2018 •

edited by ghost

Loading

ghost Aug 28, 2018 •

edited by ghost

Loading

benouat Aug 28, 2018

benouat commented Aug 28, 2018

benouat left a comment

feat(modal): stacked modals #2640

feat(modal): stacked modals #2640

Conversation

ghost commented Aug 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pkozlowski-opensource commented Aug 25, 2018

benouat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benouat commented Aug 28, 2018 • edited Loading

ghost commented Aug 28, 2018

ghost commented Aug 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Aug 28, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Aug 28, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

ghost Aug 28, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benouat commented Aug 28, 2018

benouat left a comment

Choose a reason for hiding this comment

benouat commented Aug 28, 2018 •

edited

Loading

ghost Aug 28, 2018 •

edited by ghost

Loading

ghost Aug 28, 2018 •

edited by ghost

Loading

ghost Aug 28, 2018 •

edited by ghost

Loading