dialog icon indicating copy to clipboard operation
dialog copied to clipboard

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

Open va-stefanek opened this issue 2 years ago • 11 comments

PR Checklist

Please check if your PR fulfills the following requirements:

  • [ ] The commit message follows our guidelines: CONTRIBUTING.md#commit
  • [x] Tests for the changes have been added (for bug fixes / features)
  • [ ] Docs have been added / updated (for bug fixes / features)

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

va-stefanek avatar Jun 27 '23 12:06 va-stefanek

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

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

NetanelBasal avatar Jun 28 '23 05:06 NetanelBasal

@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.

milo526 avatar Jun 28 '23 09:06 milo526

@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)

va-stefanek avatar Jun 28 '23 09:06 va-stefanek

Interesting

NetanelBasal avatar Jun 28 '23 11:06 NetanelBasal

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

va-stefanek avatar Jun 28 '23 20:06 va-stefanek

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

NetanelBasal avatar Jun 29 '23 05:06 NetanelBasal

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).

milo526 avatar Jun 29 '23 09:06 milo526

@NetanelBasal any input?

va-stefanek avatar Jul 11 '23 13:07 va-stefanek

@milo526 what do u think about this approach?

NetanelBasal avatar Jul 11 '23 18:07 NetanelBasal

@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.

milo526 avatar Jul 11 '23 18:07 milo526