Enterprise-ng: Modal: Dialogues not being properly destroyed if not closed beforehand

Created on 31 May 2019 · 12Comments · Source: infor-design/enterprise-ng

Describe the bug
Something seems to have changed in the Destroy/Tear-down mechanism of dialogues (modals, panels, etc.) from 4.12.1 to 5.3.0 that when navigating outside of the current app-route component that opened the dialog the <div class="modal-page-container"> and all its contents remains in the DOM if the dialog is not explicitly closed (e.g., by clicking "cancel", "close", etc.).

To Reproduce

Clone the Git branches for "enterprise-ng" versions 4.12.x and 5.3.x
Open the app.routes.ts file of both projects and change the last line to:
export const AppRoutingModule: ModuleWithProviders = RouterModule.forRoot(routes, { useHash: true });
Build/Serve the two projects either one at a time or both by changing one server port
Navigate to the "Modal Dialog" option and then open the "Example" dialog
Inspect the DOM elements to find the "modal-page-container" DIV.
Change the URL in the address bar to switch to another component page (e.g. /line)
Notice that in the earlier version the DIV is destroyed while in the newer it's not.

Expected behavior
All the dialog's mark-up must be removed when the component is destroyed.

Version

ids-enterprise-ng: 5.3.x

Screenshots
Please, refer to the attached videos inside the Zip file showcasing the issue as per the steps above:
modal.zip

Platform

OS Version: Windows
Browser Name: Chrome
Browser Version: Latest

[3] high type

Source

edgarinfor

Most helpful comment

Don't use hash navigation (the way you modify the last line). It actually seems a bit funny as it shows the project twice in the url http://localhost:4200/ids-enterprise-ng-demo/#/ids-enterprise-ng-demo/line

The project being shown twice in the url is actually due to the IDS project's own configuration:

package.json "start": "npx ng serve --serve-path=/ids-enterprise-ng-demo",
index.html <base href="/ids-enterprise-ng-demo/">

If you replace that with just '/' then the url becomes: http://localhost:4202/#/line

edgarinfor on 17 Jun 2019

👍2

All 12 comments

@bthharper @pwpatton have any suggestions on this one at all?

tmcconechy on 13 Jun 2019

FYI: @bthharper I think you encountered a similar issue last year: https://teams.microsoft.com/l/message/19 :[email protected]/1531322910103?tenantId=457d5685-0467-4d05-b23b-8f817adda47c&groupId=4f50ef7d-e88d-4ccb-98ca-65f26e57fe35&parentMessageId=1531322732645&teamName=IDS%20Enterprise%20Development%20(SoHo%20Xi)&channelName=Angular%20Components&createdTime=1531322910103

edgarinfor on 13 Jun 2019

@bthharper, I just tried on the latest master and I could reproduce it:
<html lang="en-US" data-sohoxi-version="4.20.0-dev" class="is-chrome light-theme" data-ids-enterprise-ng-version="5.5.0-dev">

edgarinfor on 17 Jun 2019

FYI: I was able to replicate the issue and look at it briefly as we see it. I used the following steps:

1. just added this as last line in src/app/app.routes.ts `export const AppRoutingModule: ModuleWithProviders = RouterModule.forRoot(routes, { useHash: true });`
2. Go to http://localhost:4200/ids-enterprise-ng-demo/#/ids-enterprise-ng-demo/modal-dialog
3. Type the url in and change it like http://localhost:4200/ids-enterprise-ng-demo/#/ids-enterprise-ng-demo/line 
4. Can see the modal on top still and modal destroy is not being called.

We suspect that the way the dialog is created (dynamically in code) is no longer hooked into the component life-cycle properly. It would need to some investigation to find out what has changed in angular. Seems like you have a couple workarounds for now.

Don't use hash navigation (the way you modify the last line). It actually seems a bit funny as it shows the project twice in the url http://localhost:4200/ids-enterprise-ng-demo/#/ids-enterprise-ng-demo/line
Implement destroy in the demo (parent page and call destroy on the modal manually as this is firing), tried to come up with an example but this isnt so straightforward.

export class ModalDialogDemoComponent implements OnDestroy {
 ...
  ngOnDestroy() {
    // call modal component destroy
  }

So Still investigating...

tmcconechy on 17 Jun 2019

@edgarinfor - managed to replicate it.

Making this change to the dialog fixes the issue for me. However, you will need to do this on all dialogs as adding this in generically will take longer to figure out.

export class ExampleModalDialogComponent implements OnDestroy {
   public model = {
    header: 'Default Header Text',
    comment: 'This task needs to be escalated to maximum priority and delivered by the end of this week.',
   };

  constructor(private dialogRef: SohoModalDialogRef<ExampleModalDialogComponent>) {
  }

  ngOnDestroy(): void {
    this.dialogRef.close();
  }
}

bthharper on 17 Jun 2019

👍1

See branch https://github.com/infor-design/enterprise-ng/compare/508-modal-destroy for a working example of this workaround.

tmcconechy on 17 Jun 2019

Don't use hash navigation (the way you modify the last line). It actually seems a bit funny as it shows the project twice in the url http://localhost:4200/ids-enterprise-ng-demo/#/ids-enterprise-ng-demo/line

The project being shown twice in the url is actually due to the IDS project's own configuration:

package.json "start": "npx ng serve --serve-path=/ids-enterprise-ng-demo",
index.html <base href="/ids-enterprise-ng-demo/">

If you replace that with just '/' then the url becomes: http://localhost:4202/#/line

edgarinfor on 17 Jun 2019

👍2

The ids-enterprise-ng-demo prefix is there to mimic real world usage.

I does seem somewhat odd that it appears twice, but I guess it's to ensure uniqueness on the internal routes. Would have to look to look into hash routing, but have not used that in any applications to date.

bthharper on 17 Jun 2019

@tmcconechy , @bthharper , thanks for the workaround!

We've been using it to try to fix our broken tests and continue with the upgrade to see what else we find...

So far, it seems to be working but, hopefully you can find a general fix within the IDS project.

edgarinfor on 19 Jun 2019

👍1

Re-opening until this is fixed

tmcconechy on 31 Mar 2020

Just a note im still confused about this issue. And exactly how to reproduce it running the demo app?
Would someone be willing to make a branch with a failing example?

tmcconechy on 7 Apr 2020

To reproduce it, it is very easy, look at your own comment above:
https://github.com/infor-design/enterprise-ng/issues/508#issuecomment-502730287

edgarinfor on 7 Apr 2020

😄1

Was this page helpful?

0 / 5 - 0 ratings