Closed goetzrobin closed 1 year ago
Hard one to start with, due to dialog considerations :) I like the directive approach to dialog-title, description etc. I'd honestly take it further and make the entire thing a structural directive as this would allow us to pass context to the entire component & extend it, remove the internal button from it's content projection & make the current *dialogContent directive unnecessary.
Single component example:
<button [brnDialogTriggerFor]="myAlert" />
<div #myAlert *brnDialog="data; let ctx=context;" />
<!-- dialog content -->
</div>
This would also allow for a constructor based approach a la angular cdk
@Component({
// dialog
})
class DialogComponent {
private readonly ctx = inject(BrnDialogContext);
}
@Component({
template: `
<button (click)="openDialog()" />Open</button>
`
})
class ParentComponent {
private readonly dialog = inject(BrnDialog);
openDialog() {
this.dialog.open(DialogComponent, {
// options,
data
})
}
}
We could separate data & the dialog ref into separate injection tokens/passed context props or have them on the same object as I think most use both when using injection
interface BrnDialogRef<Output = unknown> {
close(output?: Output): void;
afterClosed(): Observable<Output | undefined>
// other relevant fns
}
type BrnDialogContext<Input = unknown, Output = unknown> {
readonly $implicit: Input;
readonly ref: BrnDialogRef<Output>;
readonly data: Input // getter to $implicit for easier typing within .ts files
}
Then the alert can either be done as a simple alert component, composed with the dialog component itself or as syntax sugar for the dialog directive through extension. Functionally it would do the same.
I'm a bit unsure on the separation of Cancel & Action buttons, maybe an angular material like approach with a simple [brn-dialog-close]="outputData"
would suffice?
The overlay i don't mind either way, since tw would allow for any reasonable customization through background color, opacity.. class props
We could do that too for sure. Using the wrapper component could allow people to add classes to that component which are used to style the backdrop, and "hides" the manual wiring up of the trigger by using a reference to the directive exposed by "exportAs". I think that would make it more beginner friendly, but they're also not mutually exclusive.
If you have a wrapper it does the wiring up for you if you don't you'll need to do it yourself?
Yeah I just looked at the radix UI code to see if any special aria roles are assigned by those close and action buttons and it does not seem so. I think it could be a convenience thing to add the directives, but with exposing the context it's absolutely not needed. Very open to drop them tbh
Fair :) a wrapper it is.
Not really firm on this one either, personally id always use close, but we can always add more descriptive selectors to that directive
brn
API (proposed)
Do we like the overlay being its own component? I could also see this being a structural directive that allows for some configuration through inputs it directly?
Helpful info
Dialog also allows to expose data to the dialog through dependency injection. I don't have anything against it, but might not even need that if we go the template route since we also have access to data/functions of the enclosing component. We do have to be aware of Change Detection implications of this approach though as CD of enclosing component is used for dialog in this case, as far as I understand.
hlm
API (proposed)
Since this is a pretty complex component I would just expose the same API, but with a hlm prefix and maybe also expose some standalone directives that only apply the styles for all the different parts. We should aim for simplicity as most people might not need the available customization brought by the component + (optional style) directive, but instead want the simplest API
Helpful info
radix
API
Source
https://github.com/radix-ui/primitives/tree/main/packages/react/alert-dialog/src
Aria Design Pattern
Source
https://www.w3.org/WAI/ARIA/apg/patterns/alertdialog/
WAI-ARIA Roles, States, and Properties
The element that contains all elements of the dialog, including the alert message and any dialog buttons, has role alertdialog.
The element with role alertdialog has either:
The element with role alertdialog has a value set for aria-describedby that refers to the element containing the alert message.
Keyboard Interaction
In the following description, the term tabbable element refers to any element with a tabindex value of zero or greater. Note that values greater than 0 are strongly discouraged.
When a dialog opens, focus moves to an element inside the dialog. See notes below regarding initial focus placement.
Tab:
Shift + Tab:
Escape: Closes the dialog.
Note
Helpful info
shadcn
API
Source
Other notes
This should be connected to the RFC of the general Dialog once it is created