Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

va-stefanek
Copy link

PR Checklist

Please check if your PR fulfills the following requirements:

  • The commit message follows our guidelines: CONTRIBUTING.md#commit
  • 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

@stackblitz
Copy link

stackblitz bot commented Jun 27, 2023

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

@va-stefanek va-stefanek changed the title feat: allow to define config in scope of the dialog component class instance feat: allow to define config in scope of the dialog component class Jun 27, 2023
@va-stefanek va-stefanek force-pushed the feat-89-class-based-dialog-config branch from 9e65d7e to ace7359 Compare June 27, 2023 13:07
@NetanelBasal
Copy link
Member

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

@milo526
Copy link
Contributor

milo526 commented Jun 28, 2023

@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
Copy link
Author

@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
Copy link
Member

Interesting

@va-stefanek
Copy link
Author

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

@NetanelBasal
Copy link
Member

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

@milo526
Copy link
Contributor

milo526 commented Jun 29, 2023

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
Copy link
Author

@NetanelBasal any input?

@NetanelBasal
Copy link
Member

@milo526 what do u think about this approach?

@milo526
Copy link
Contributor

milo526 commented Jul 11, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants