feat(dialog): open dynamic component in dialog

ty-ler commented 4 months ago

PR Checklist

Please check if your PR fulfills the following requirements:

[x] The commit message follows our guidelines: https://github.com/goetzrobin/spartan/blob/main/CONTRIBUTING.md#-commit-message-guidelines
[x] Tests for the changes have been added (for bug fixes / features)
[x] 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:

Which package are you modifying?

[ ] accordion
[ ] alert
[x] alert-dialog
[ ] aspect-ratio
[ ] avatar
[ ] badge
[ ] button
[ ] calendar
[ ] card
[ ] checkbox
[ ] collapsible
[ ] combobox
[ ] command
[ ] context-menu
[ ] data-table
[ ] date-picker
[x] dialog
[ ] dropdown-menu
[ ] hover-card
[ ] input
[ ] label
[ ] menubar
[ ] navigation-menu
[ ] pagination
[x] popover
[ ] progress
[ ] radio-group
[ ] scroll-area
[ ] select
[ ] separator
[x] sheet
[ ] skeleton
[ ] slider
[ ] spinner
[ ] switch
[ ] table
[ ] tabs
[ ] textarea
[ ] toast
[ ] toggle
[ ] tooltip
[ ] typography

What is the current behavior?

Closes #35

Dialogs can currently only be created and opened from a component's template. Opening a dialog from a service is possible (using a subject in the service), but requires the dialog to exist in the template of an already existing component.

What is the new behavior?

Dialogs can now be opened from anywhere where the HlmDialogService can be injected.

Most dialog-related components relied on the BrnDialogComponent existing for the dialog's open/closed state. Since BrnDialogComponent did not exist in the context of dynamically created dialogs, those reliant components had to be slightly reworked. These were just internal changes - the public APIs are the same.

BrnDialogService is now provided in root.
HlmDialogService is a new service which has a method for opening a component in a styled dialog, from a component or service.
- Can pass through a context object and providers to the dialog.

Does this PR introduce a breaking change?

[x] Yes
[ ] No

The injectDialogCtx has been renamed to injectDialogContext.
provideBrnDialog() is no longer necessary (BrnDialogService is provided in root)

Other information

An example section in the dialog documentation has been added to demonstrate this new feature. A matching story in the storybook has also been added.

goetzrobin commented 4 months ago

I think this is an awesome improvement for the current Dialog! I do want to make sure we are still able to have multiple dialogs open at the same time. Can you add a test for that?

Also, let's continue to support the injectDialogCtx method and add the injectDialogContext. We can mark injectDialogCtx as deprecated and remove before we go stable. I do want to be considerate as I believe that the Dialog Component is one of the most used!

ty-ler commented 4 months ago

I think this is an awesome improvement for the current Dialog! I do want to make sure we are still able to have multiple dialogs open at the same time. Can you add a test for that?

Also, let's continue to support the injectDialogCtx method and add the injectDialogContext. We can mark injectDialogCtx as deprecated and remove before we go stable. I do want to be considerate as I believe that the Dialog Component is one of the most used!

Thanks for your review! Yes, continuing support for injectBrnDialogCtx is a good idea. I will add that function back into the PR. Will also add in the test for multiple dialogs.

On a similar note - should the injectDialogContext really be named injectBrnDialogContext? I realize now that I dropped the Brn part without thinking.

goetzrobin commented 4 months ago

I think this is an awesome improvement for the current Dialog! I do want to make sure we are still able to have multiple dialogs open at the same time. Can you add a test for that? Also, let's continue to support the injectDialogCtx method and add the injectDialogContext. We can mark injectDialogCtx as deprecated and remove before we go stable. I do want to be considerate as I believe that the Dialog Component is one of the most used!

Thanks for your review! Yes, continuing support for injectBrnDialogCtx is a good idea. I will add that function back into the PR. Will also add in the test for multiple dialogs.

On a similar note - should the injectDialogContext really be named injectBrnDialogContext? I realize now that I dropped the Brn part without thinking.

Let's do it! Thanks for working on this!

ty-ler commented 4 months ago

This looks really good, I think we just need to update the popover e2e tests along with some of the other feedback and I would say this is probably good.

Changed the popover component's ariaDescribedBy and ariaLabelledBy values to be empty strings by default so that the popover tests now work as intended. This way the values are not assigned in the dialog service, and the aria values do not appear in the DOM.

goetzrobin / spartan