ngneat / dialog

👻 A simple to use, highly customizable, and powerful modal for Angular Applications
https://ngneat.github.io/dialog/
MIT License
394 stars 38 forks source link

feat: allow to define config in scope of the dialog component class #106

Open va-stefanek opened 1 year ago

va-stefanek commented 1 year ago

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

Issue Number: #89

What is the new behavior?

Added possibility to define config in scope of component class

Does this PR introduce a breaking change?

[ ] Yes
[x] No
stackblitz[bot] commented 1 year ago

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

NetanelBasal commented 1 year ago

@milo526 @va-stefanek can we think on another way to do it without using inheritance and still keep it typed?

milo526 commented 1 year ago

@milo526 @va-stefanek can we think on another way to do it without using inheritance and still keep it typed?

I've looked into this and I cannot find a cleaner solution for now.

va-stefanek commented 1 year ago

@NetanelBasal @milo526 we can think of about dedicated function to create dialog definition e.g createDialogDef which will point into the dialog component class and accept config. Than that definition could be passed to open the dialog. We could define it in scope of same file as our component or in separate one. We can also lazy load that component in scope of that config definition which will reduce bundle size till the time we decide to open it.

e.g

export const dialogDef = createDialogDef({
  load: async () =>
    (await import(`./test-component`)).TestComponent,
  defaultConfig: {
    enableClose: true,
    minWidth: '626px',
    width: '626px',
  },
})

and

dialogService.open(dialogDef)

NetanelBasal commented 1 year ago

Interesting

va-stefanek commented 1 year ago

@NetanelBasal @milo526 let me know if this is what you are interested in.

NetanelBasal commented 1 year ago

I'd love to hear from @milo526, who'll probably be the first consumer.

milo526 commented 1 year ago

I'd love to hear from @milo526, who'll probably be the first consumer.

To be fair; this is not what I envisioned when creating the issue.

The configuration is now still outside of the class and a developer could still accidentally or intentionally open the class itself instead of this new definition.

This is not much different from adding a static method on the class and providing that as the argument to the dialogService.open call (expect from some stricter enforcing of types at the config definition location).

va-stefanek commented 1 year ago

@NetanelBasal any input?

NetanelBasal commented 1 year ago

@milo526 what do u think about this approach?

milo526 commented 1 year ago

@milo526 what do u think about this approach?

My first question would be, how does this interact with DI? Not able to test this myself right now, but being able to use DI in modals is important to use; in our experience requiring constructor parameters often messes with DI.