feat: enhance dialog service to manage open dialog refs #46
Conversation
|
|
ScottTylerTech
changed the title
|
@ScottTylerTech Thanks for putting this together! It looks good so far. Could you add an example usage of it in the dialog demo with multiple dialogs? That will help us validate the functionality given we don't have unit tests set up on this repo at the moment. |
Sure thing. |
|
Any advice on how to shift a dialog, so they are not stacked>? Currently I just set it to movable to show the nested dialogs
|
| } | ||
|
|
||
| private _closeAllDialogs(result: boolean, recursiveExecutionCount = 0): void { | ||
| if (recursiveExecutionCount > 2) { |
There was a problem hiding this comment.
@DRiFTy17 This is needed for the nested dialogs. I have it set to 2. is there a reasonable number that it should be set to? I know within our app we do not allow dialogs to open dialogs just as UX functionality, but I know our app does not represent the full usage of forge.
There was a problem hiding this comment.
So this is just a safe guard against it recursing infinitely basically in the case where closing a dialog also opens a dialog? This is probably an edge case, but I understand the need for it. I would say 2 sounds fine to me, but let's put that into a constant like MAX_NESTED_DIALOGS to avoid the magic number. If we ever run into a case where this needs to be adjusted then we could make the value configurable or something, but this looks fine to me for now.
| } | ||
|
|
||
| private _closeAllDialogs(result: boolean, recursiveExecutionCount = 0): void { | ||
| if (recursiveExecutionCount > 2) { |
There was a problem hiding this comment.
So this is just a safe guard against it recursing infinitely basically in the case where closing a dialog also opens a dialog? This is probably an edge case, but I understand the need for it. I would say 2 sounds fine to me, but let's put that into a constant like MAX_NESTED_DIALOGS to avoid the magic number. If we ever run into a case where this needs to be adjusted then we could make the value configurable or something, but this looks fine to me for now.
There was a problem hiding this comment.
Overall looks good, and I really like the extra documentation, just need to remove the console.table(). We had similar functionality in place in Public Safety around Material Dialogs, so we'll definitely also benefit from this feature as we migrate to Forge dialogs.
ScottTylerTech
dismissed stale reviews from samrichardsontylertech and DRiFTy17
via
320130b
ScottTylerTech
requested review from
samrichardsontylertech,
DRiFTy17 and
MikeMatusz
|
🚀 PR was released in |
PR Checklist
Please check if your PR fulfills the following requirements:
This would close #45
Describe the new behavior?
Enhances the dialog service to manage open dialogs and give the ability to close all open dialogs. Similar to the
MatDialogModulein Angular Materialservice.show()is called, the dialog ref is added to an arrayDescribe any alternatives you've considered
A viable alternative to is to create an internal service with the suggested implementation that extends on the current dialog service and use that instead.
Additional information
A specific use case for this feature: This functionality is derived from a user session termination with the tid-session manager. In an app, when the session is terminated, if there is an open dialog, the dialog persists on top of the session timeout page and the user still has the ability to navigate the dialog given its a single or stepper. The purposed enhanced functionality would allow the session manager to