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
WIP: refactore dialog DOM creation #1671
WIP: refactore dialog DOM creation #1671
Conversation
This is looking good so far @netcitylife can't wait to see this evolve. I think an important thing to note is for such a change we will require:
But, you're probably already planning on those. Just thought I would mention. |
readonly overlay: HTMLElement; | ||
readonly contentHost: HTMLElement; | ||
export interface IDialogDomRenderer extends IDisposable { | ||
render(dialogHost: Element, controller: IDialogController): HTMLElement; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit concerned by this interface changes. This change implies states are being brought into the renderer. Doing it this way make it questionable what's the relationship between rendered elements and the renderer. Are they tied to each other until disposed?
Previously renderer is just renderer, after rendering, nothing is needed from the renderer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, renderer is now transient, not singleton. It is merged together with DialogDom, which is now gone, because it reflected a concrete DOM structure, which could not be overriden via configuration
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it reflected a concrete DOM structure
This DOM structure is quite standard for dialog though. Overlay + content host are commonly expected. Was this structure the issue leading to this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure it is quite common, but most of the times we will not need access to this structure. Previously, as I understand, DialogDom was created for keeping a statefull reference to DOM elements for these purposes:
- Allow DialogController to access DOM nodes to handle their events. This is not the case any more because it is now renderer responsibility.
- Allow styling of dialog container/overlay/host (https://docs.aurelia.io/aurelia-packages/dialog#centering-uncentering-dialog-position) in default renderer implementation. This is a very specific approach to set container element styles directly from a dialog component, but it is still available for default implementation, you just need to inject IDialogDomRenderer instead of IDialogDom in dialog content component.
Now that renderer subscribes and unsubscribes from DOM events - it needs to be statefull, so I made it transient and merged with DialogDom to avoid state duplication.
Now consider that we need to implement a new renderer for Bootstrap modal (https://getbootstrap.com/docs/5.3/components/modal/#via-javascript). We need to create Bootstrap DOM structure and instantiate it's JS modal component (or use a wrapper custom element around it). So we need to keep some state between DOM render and disposal (reference to bootstrap.Modal instance in this case), and that state would be much different than DialogDom was. And if we implement a renderer for Material Web Components Dialog - it would be the third different state. So state depends heavily on underlying renderer implementation (library API) and keeping separate DialogDom with this concrete structure (state) does not make much sense, it is a specific case for current default implementation.
Sure, when finished |
private readonly hostCss: string = 'position:relative;margin:auto;'; | ||
|
||
public render(dialogHost: HTMLElement): IDialogDom { | ||
public render(dialogHost: HTMLElement, controller: IDialogController): HTMLElement { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Associating a parameter to some state in a private property this way is not appropriate. It's brittle in a way that this render call is now supposed to be called only once, but the interface doesn't communicate that. Overall, this is where my concerns are: it's unclear at what point what states are available if we make something named "renderer" a "lazily-stateful" service.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that renderer interface should be improved. This change was just the first step, showing the main goal - incapsulation of DOM manipulation inside renderer or some DialogDom instance, cleanup of DialogService and dialogController. Now I plan to create several POC renderers to see what states and renderer capabilities would they need to integrate seemlessly into current dialog lifecycle, so next step is to design and approve final renderer interface.
@netcitylife can you elaborate what would be the challenge extending the existing way of doing things for something like a bootstrap modal? class MyRenderer {
render(_, controller) {
const dom = new BootstrapDialogDom(controller);
}
}
class BootstrapDialogDom {
constructor(private controller) {
// attach listener here and close controller when necessary.
}
} |
In this approach I see no need in a renderer at all, it does nothing but instantiating BootstrapDialogDom, which will create DOM in constructor() and then destroy it in dispose(). So it may be just called directly from controller. I see your confusion about current renderer interface, see my comment above. |
We are doing registration for plug & play so that custom renderer can be used. It's certainly a useful layer to have for customization. Can you also help tackle the other parts in my comment, about (a) the challenges of extending the existing one, and (b) why wouldn't you be able to do it with just different |
See my comment in Discord, please |
return onResolve(ctrlr.activate(ctrlr, null, LifecycleFlags.fromBind), () => { | ||
dom.overlay.addEventListener(settings.mouseEvent ?? 'click', this); | ||
return DialogOpenResult.create(false, this); | ||
return onResolve(renderer.render(controller), () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if it's desirable, we can add custom lifecycle hooks the way we have for router. Just need to figure out an acceptable API. We shouldn't be changing the renderer just to accommodate this. Rendering process should be simple, idempotent and synchronous.
Pull Request
📖 Description
The goal of this change is to refactore DialogService and DialogController to be more DOM-agnostic and allow flexible DefaultDialogDomRenderer overriding (for example create BootstrapDialogDomRenderer which renders dialog using Bootstrap's markup and native JS).
Changes include:
🎫 Issues
👩💻 Reviewer Notes
📑 Test Plan
⏭ Next Steps