dougludlow / ng2-bs3-modal

Angular Bootstrap 3 Modal Component
http://dougludlow.github.io/ng2-bs3-modal/
ISC License
261 stars 133 forks source link

Add/Remove modal DOM elements to body on open/close #3

Closed ellern closed 8 years ago

ellern commented 8 years ago

My app becomes very sluggish after using it for a while. This is because it keeps modal elements in the DOM.

dougludlow commented 8 years ago

Interesting. I'll take a look into that.

ellern commented 8 years ago

It most noticeable in Edge/IE and Firefox. Chrome and Safari seems to be working just fine.

dougludlow commented 8 years ago

@ellern I took some time today trying to repro the issue. I even wrote some tests. I suspected the issue was around the fact that I'm appending the modal to the body and that it wasn't being cleaned up, but that didn't appear to be the case. Could you possibly link to a sample project or plunker that exposes the issue?

ellern commented 8 years ago

My use case: I show a list of 20 users per page, each user has 8 different modals. Some of the modals are complex others are just simple "confirm" modals.

I did some more testing, and I was too quick on the trigger regarding Firefox, it does slow down but not as much as IE and Edge, Edge being the worst. All of the browsers will eventually render it as it should, but Edge (and IE) will get into a non-responsive state before the page is fully rendered.

However I still think it's a issue that modals are left in the DOM, they should be cleaned up. And if they were added on "open" then my problem would be gone as well.

rsnider19 commented 8 years ago

I am noticing that the modals don't get cleaned up when routing.

  1. On my home page, I have 4 modals and am seeing #modal_1 through #modal_4
  2. Then when routing to another page with 2 modals, I am seeing #modal_1 through #modal_6.
  3. When navigating back home (with 2 modals), I am now seeing #modal_1 through #modal_10

From what I can tell, the old modals have just the modal class whereas the new ones have modal fade. Maybe you can use that?

btrauma8 commented 8 years ago

I think you need this:

ngOnDestroy() { this.$modal.remove(); } (on ModalComponent.ts)

edit: i just tested. you definitely need this. see below.

ngOnDestroy() {
        var domId = this.id;
        this.$modal.remove(); // Try commenting this out. You'll see  "it is still there"  when <modal> leaves the dom. the modal hasn't left!
        setTimeout(function () {
            if (document.getElementById(domId)) {
                alert('it is still there');
            } else {
                alert('it is GONE');
            }
        }, 100);
}
dougludlow commented 8 years ago

Thanks @rsnider19, @btrauma8. I've released it as v0.1.6. Give it a go and let me know if that resolves the issue.

dougludlow commented 8 years ago

Published it to npm.

rsnider19 commented 8 years ago

works perfectly @dougludlow

dougludlow commented 8 years ago

:thumbsup:

dougludlow commented 8 years ago

@ellern, @btrauma8 any feedback?

ellern commented 8 years ago

It's a lot better. But in my use case it's still slowing it down because the modal is added on ngAfterViewInit() and not on modal open. I don't know about this, but is it possible to move the code from ngAfterViewInit() into open(size?: string)?

dougludlow commented 8 years ago

@ellern still trying to repro this. Here's my latest attempt: https://run.plnkr.co/TYeyXvPG5qjVsQFi/

You can edit the plunker here: https://plnkr.co/edit/wg2d3A?p=preview.

Any way you can make this match up to your environment?

dougludlow commented 8 years ago

I like the idea of lazyloading the init on open. I'll create a release that does that, have you try it out.

dougludlow commented 8 years ago

@ellern give ng2-bs3-modal@0.1.7 a try.

ellern commented 8 years ago

The only difference for my use case is that my modals are a lot more complex.

I'll give it a try

dougludlow commented 8 years ago

@ellern have you had a chance to try that out?

dougludlow commented 8 years ago

Closing. Reopen if you're still having issues.

ellern commented 8 years ago

I've been busy, so have not been able to test it properly. My initial testings works a lot better than before, though I still have some performance issues, but they are probably not related to the modal.