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.

Sign up for GitHub

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

Jump to bottom

feat(dialog): open dynamic component in dialog #180

Merged
merged 1 commit into from
Mar 6, 2024
Merged

feat(dialog): open dynamic component in dialog #180

merged 1 commit into from
Mar 6, 2024

Conversation

ty-ler
Copy link
Contributor

@ty-ler ty-ler commented Mar 1, 2024

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • 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
  • alert-dialog
  • aspect-ratio
  • avatar
  • badge
  • button
  • calendar
  • card
  • checkbox
  • collapsible
  • combobox
  • command
  • context-menu
  • data-table
  • date-picker
  • dialog
  • dropdown-menu
  • hover-card
  • input
  • label
  • menubar
  • navigation-menu
  • pagination
  • popover
  • progress
  • radio-group
  • scroll-area
  • select
  • separator
  • 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?

  • 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
Copy link
Owner

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

ty-ler commented Mar 4, 2024 •
edited

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

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!

elite-benni
elite-benni approved these changes Mar 4, 2024
View reviewed changes
Copy link
Collaborator

@elite-benni elite-benni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This really improves the way dialogs and popovers can be used.
added just some small notes.

libs/ui/dialog/brain/src/lib/brn-dialog-trigger.directive.ts Show resolved Hide resolved
libs/ui/dialog/brain/src/lib/brn-dialog.component.ts Outdated Show resolved Hide resolved
thatsamsonkid
thatsamsonkid reviewed Mar 5, 2024
View reviewed changes
Copy link
Collaborator

@thatsamsonkid thatsamsonkid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@ty-ler ty-ler force-pushed the ty-ler/dynamic-dialog branch 2 times, most recently from 0dcbfe4 to 6d1ffb4 Compare March 5, 2024 22:58
@ty-ler
feat(dialog): open dynamic component in dialog
a515543 
refactor(dialog): working on new dialog approach

refactor(dialog): refactor dialog component to use global dialog service

refactor(dialog): remove provider function for BrnDialog

feat(dialog): add HlmDialogService for opening components in a dialog dynamically

chore: add dynamic dialog component section to docs

chore: add note about injectDialogContext

refactor(dialog): dialog overlay class names and dialog options as reusable defaults

chore: add story for dialog dynamic components

refactor(dialog): make HlmDialogService options optional (defaults are provided)

refactor(dialog): continue support for injectBrnDialogCtx (deprecated)

refactor(dialog): move (click) handlers back into metadata host

refactor(dialog): prefer early returns

refactor(dialog): injectDialogContext -> injectBrnDialogContext

chore: add nested dialog stories and e2e tests

refactor(dialog): use input function in brnDialogTrigger

refactor(dialog): use optional chaining in dialog component

refactor(dialog): _id() renamed to id(), use in inheriting classes

refactor(popover): change ariaDescribedBy, ariaLabelledBy to empty string so not assigned defaults

refactor(dialog): use correct member names after merge
@ty-ler
Copy link
Contributor Author

ty-ler commented Mar 5, 2024

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.

@ty-ler ty-ler requested a review from goetzrobin March 5, 2024 23:03
@goetzrobin goetzrobin merged commit d4ffb47 into goetzrobin:main Mar 6, 2024
7 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thatsamsonkid thatsamsonkid thatsamsonkid left review comments

@goetzrobin goetzrobin goetzrobin approved these changes

@elite-benni elite-benni elite-benni approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Can't open a dialog from a different component.
4 participants
@ty-ler @goetzrobin @thatsamsonkid @elite-benni