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

Add public event to notify when the overlay is closed #7422

Closed
web-padawan opened this issue May 17, 2024 · 6 comments
Closed

Add public event to notify when the overlay is closed #7422

web-padawan opened this issue May 17, 2024 · 6 comments
Assignees
Labels
enhancement New feature or request vaadin-dialog vaadin-notification workaround There is a workaround in the comments.

Comments

@web-padawan
Copy link
Member

Describe your motivation

Currently, vaadin-dialog and vaadin-notification don't notify about closing completed.
The opened-changed event isn't suitable since it is fired before the closing animation.

Describe the solution you'd like

  1. Expose vaadin-overlay-closed event added in feat: dispatch an event after overlay closing completed #5397 publicly in vaadin-dialog (and possibly also vaadin-confirm-dialog for consistency)
  2. Add a similar event to vaadin-notification that should be fired in _removeNotificationCard() method.

Describe alternatives you've considered

No response

Additional context

No response

@TatuLund TatuLund added bug Something isn't working and removed enhancement New feature or request labels May 20, 2024
@TatuLund
Copy link
Contributor

The event seems to be public to me. For example the code here compiles.

https://github.com/TatuLund/hilla-demo/blob/hilla2/frontend/views/list/contact-form.ts#L196

vaadin-overlay-closed

Also needed types seems to be exported.

But the event listener is not getting fired.

@web-padawan
Copy link
Member Author

The event is fired by vaadin-confirm-dialog-overlay, not the vaadin-confirm-dialog itself:

this.dispatchEvent(new CustomEvent('vaadin-overlay-closed'));

The fact that VSCode Lit plugin doesn't produce warnings related to this event doesn't mean it is correctly used.

Also, this is not a bug but an enhancement ticket.

@web-padawan web-padawan added enhancement New feature or request and removed bug Something isn't working labels May 20, 2024
@TatuLund
Copy link
Contributor

The fact that VSCode Lit plugin doesn't produce warnings related to this event doesn't mean it is correctly used.

In addition to that, there is no run-time JavaScript exception either ... So that further increases the confusion.

@web-padawan
Copy link
Member Author

I agree it's a bad DX. But it appears to be an issue of the Lit analyzer tools (including VS Code Lit plugin) that are lacking proper events support. I reported a few issues related to this e.g. runem/lit-analyzer#143 and runem/lit-analyzer#155

You can actually verify that by using e.g. @change - this will simply pick all the existing declarations of this event.

At the same time, I don't think adding an event listener to a non-existing event is supposed to produce an extension.

@TatuLund
Copy link
Contributor

Ok, got it. So there is a workaround

import { ConfirmDialog } from "@vaadin/confirm-dialog";

export class CustomConfirmDialog extends ConfirmDialog {
  _onOverlayClosed(e) {
    const event = new CustomEvent("vaadin-overlay-closed");
    console.log("dispatching custom event", event);
    this.dispatchEvent(event);
  }

  connectedCallback() {
    super.connectedCallback();
    this._overlayElement.addEventListener(
      "vaadin-overlay-closed",
      this._onOverlayClosed.bind(this)
    );
  }
}

customElements.define("custom-confirm-dialog", CustomConfirmDialog);

@DiegoCardoso
Copy link
Contributor

Done by #7471, #7472, and #7473.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request vaadin-dialog vaadin-notification workaround There is a workaround in the comments.
Projects
None yet
Development

No branches or pull requests

3 participants