Skip to content

Conversation

polarbirke
Copy link
Member

@polarbirke polarbirke commented Jun 15, 2025

This PR introduces three new features:

  • All dialogs wrapped by <dialogs-utils> will now emit a "show" event. This is achieved by monkey-patching the show() and showModal() methods.
  • Page scroll is prevented if a dialog inside <dialog-utils> is opened as a modal; this is a recommended UX best practice.
  • The Web Component Class is exported to allow subclassing, e.g. to extend the onShow() and onClose() methods.

@polarbirke polarbirke changed the title Polyfill show() and showModal() methods to emit a "show" Event and prevent page scroll PAtch show() and showModal() methods to emit a "show" Event and prevent page scroll; allow subclassing of the component Jun 16, 2025
@polarbirke polarbirke changed the title PAtch show() and showModal() methods to emit a "show" Event and prevent page scroll; allow subclassing of the component Patch show() and showModal() methods to emit a "show" Event and prevent page scroll; allow subclassing of the component Jun 16, 2025
@MalteWunsch MalteWunsch requested a review from Copilot July 7, 2025 11:49
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR patches show() and showModal() on wrapped dialogs to emit a show event, prevents page scrolling when a modal dialog is open, and exports the component class for subclassing.

  • Monkey-patch native show()/showModal() to emit a show event with detail.isModal
  • Lock and restore page scroll when modal dialogs open and close
  • Export DialogUtils class to enable subclassing

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
src/dialog-utils.js Export component class; add event emission; add scroll lock logic
package.json Bump version to 0.2.0
README.md Document new show event and subclassing example
Comments suppressed due to low confidence (2)

src/dialog-utils.js:1

  • Removing the default export in favor of a named export may break existing consumers; consider adding export default DialogUtils or bumping to a major version.
export class DialogUtils extends HTMLElement {

src/dialog-utils.js:60

  • New behavior for emitting show events and scroll locking should be covered by unit/integration tests to verify event detail and overflow restoration.
    monkeyPatchShowModalMethod() {

@@ -28,8 +28,12 @@ class DialogUtils extends HTMLElement {
}

this.dialog = this.querySelector('dialog');
Copy link
Preview

Copilot AI Jul 7, 2025

Choose a reason for hiding this comment

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

You assume a <dialog> is always present; add a null check or guard so that missing dialogs don’t cause runtime errors.

Suggested change
this.dialog = this.querySelector('dialog');
this.dialog = this.querySelector('dialog');
if (!this.dialog) {
console.warn('DialogUtils: No <dialog> element found. Initialization aborted.');
return;
}

Copilot uses AI. Check for mistakes.

@@ -28,8 +28,12 @@ class DialogUtils extends HTMLElement {
}

this.dialog = this.querySelector('dialog');

this.dialog.addEventListener('show', this.onShow.bind(this));
Copy link
Preview

Copilot AI Jul 7, 2025

Choose a reason for hiding this comment

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

Event listeners are added in connectedCallback but never removed in disconnectedCallback, leading to potential memory leaks; consider cleaning them up.

Copilot uses AI. Check for mistakes.

@polarbirke
Copy link
Member Author

@Copilot What are possible risks of this approach to monkey-patch the native show() and showModal() methods of the HTMLDialogElement?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant