Updates on Sites page by gcamacho079 · Pull Request #18558 · craftcms/cms

gcamacho079 · 2026-03-13T17:56:57Z

Description

Marking as a draft because I temporarily created a ModalFormNew.vue component rather than refactoring/replacing the existing one everywhere it’s used. Only the modals on the Sites page have been replaced.

Updated craft-modal (previously craft-dialog) to use Lion’s overlay system. In addition, added a name property that applies an accessible name to the dialog element.
Modified ModalForm (ModalFormNew) to fire a close-modal event that targets the web component it’s nested inside
Rewrites craft-action-menu:
- Uses Lion properties for accessing overlay nodes
- Removes close-overlay events attached to action items. Instead, I handled this on the Vue side; alternatively, we could use the isBlocking property to achieve the same visual result.
Adds a Storybook test for checking focus management on craft-modal open and close events
Modifies colors slightly to meet minimum contrast requirements

Related issues

…log story to test focus management, update color tokens to pass automated tests

…d begin refactoring the ModalForm via ModalFormNew

…contentNode after firstUpdate

Copilot

Pull request overview

This PR updates the Control Panel “Sites” settings page to use the new craft-modal overlay pattern (instead of the previous modal form approach), introduces a new CraftModal web component (replacing CraftDialog), and updates Storybook tooling to support querying within Shadow DOM for component tests.

Changes:

Refactor Sites group “New Group” and “Rename Group” flows to use craft-modal invokers and an externally-triggered rename modal.
Add CraftModal (Lit + Lion overlays) and migrate exports/wrapper generation from craft-dialog to craft-modal.
Improve Storybook play-function ergonomics for Shadow DOM components via shadow-dom-testing-library and type augmentation.

Reviewed changes

Copilot reviewed 16 out of 20 changed files in this pull request and generated 9 comments.

Show a summary per file

File	Description
resources/js/pages/SettingsSitesIndex.vue	Refactors group actions to use `craft-modal`, adds menu/modal refs and helpers.
resources/js/components/ModalFormNew.vue	Adds a new form+Pane wrapper component used inside modals.
resources/build/assets/cp.css	Rebuilt compiled CSS output.
resources/build/assets/SettingsSitesIndex.css	Rebuilt compiled page-scoped CSS output.
resources/build/SettingsSitesIndex.js	Rebuilt compiled JS bundle reflecting the Sites page changes.
resources/build/InputCombobox.js	Rebuilt compiled JS bundle output.
packages/craftcms-cp/src/types/storybook-shadow-dom.d.ts	Adds TS augmentation so Storybook canvas supports Shadow DOM queries.
packages/craftcms-cp/src/styles/shared/tokens.css	Adjusts semantic token values (brand/danger “on loud”, success/warning loud fill/border).
packages/craftcms-cp/src/index.ts	Switches public export from `CraftDialog` to `CraftModal`.
packages/craftcms-cp/src/components/modal/modal.ts	Introduces `craft-modal` web component based on Lion overlays.
packages/craftcms-cp/src/components/modal/modal.styles.ts	Adds styling for slotted modal content.
packages/craftcms-cp/src/components/modal/modal.stories.ts	Adds Storybook story/play test for `craft-modal`.
packages/craftcms-cp/src/components/dialog/dialog.ts	Removes `craft-dialog` component.
packages/craftcms-cp/src/components/dialog/dialog.stories.ts	Removes Storybook story for `craft-dialog`.
packages/craftcms-cp/src/components/action-menu/action-menu.ts	Refactors invoker/content setup to use overlay nodes; removes item click-to-close behavior.
packages/craftcms-cp/scripts/generate-vue-wrappers.js	Updates Vue wrapper generation to target `craft-modal` instead of `craft-dialog`.
packages/craftcms-cp/package.json	Adds `shadow-dom-testing-library` as a dev dependency.
packages/craftcms-cp/.storybook/preview.ts	Adds `beforeEach` hook to mix Shadow DOM queries into the Storybook canvas.
package-lock.json	Locks newly added dependency.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-25T20:25:49Z

resources/js/pages/SettingsSitesIndex.vue

+  async function openRenameModal() {
+    // Close the action menu first — modal is no longer nested inside it
+    if (siteGroupActionsMenu.value) {
+      siteGroupActionsMenu.value.opened = false;
+    }
+    // Wait one tick for the hide to begin before showing the modal
+    await nextTick();
+    if (renameGroupModal.value) {
+      renameGroupModal.value.opened = true;
+    }
+  }


openRenameModal() opens the rename modal without calling setFormProperties('update'). That drops the previous behavior of prefilling with group.rawName (and ensuring id is set), and can lead to stale/incorrect form values if the form was previously used for “New Group”. Call setFormProperties('update') before opening the rename modal.

resources/js/components/ModalFormNew.vue

Copilot · 2026-03-25T20:25:49Z

packages/craftcms-cp/src/index.ts

 export {default as CraftNavItem} from './components/nav-item/nav-item.js';
 export {default as CraftDrawer} from './components/drawer/drawer.js';
-export {default as CraftDialog} from './components/dialog/dialog.js';
+export {default as CraftModal} from '@src/components/modal/modal.js';


CraftModal is exported using the @src/... path alias, while all other exports in this barrel file use relative paths. That alias likely won’t resolve for downstream consumers of the published package. Use a relative import (e.g. ./components/modal/modal.js) for consistency and compatibility.

Suggested change

export {default as CraftModal} from '@src/components/modal/modal.js';

export {default as CraftModal} from './components/modal/modal.js';

packages/craftcms-cp/src/components/modal/modal.ts

Copilot · 2026-03-25T20:25:50Z

packages/craftcms-cp/src/components/modal/modal.styles.ts

+  ::slotted([slot='content']) {
+    display: block;
+    background-color: white;
+    border-radius: var(--c-modal-radius);
+    border-width: var(--c-modal-border-width);
+    border-style: var(--c-modal-border-style);
+    border-color: var(--c-modal-border-color);


background-color: white; hardcodes a light theme value. Since design tokens already define modal surface variables (e.g. --c-modal-fill / --c-surface-overlay), this will render incorrectly in dark mode. Consider using the appropriate CSS variable instead of a literal color.

Copilot · 2026-03-25T20:25:50Z

resources/js/pages/SettingsSitesIndex.vue

    form.clearErrors().submit(store(), {
      onSuccess: () => {
-        modalActive.value = false;
        form.reset();


saveGroup() no longer closes whichever <craft-modal> is open. Since ModalFormNew only emits submit and doesn't dispatch close-overlay, the create/rename modals will remain open after a successful save (previously modalActive was set to false). Consider closing the active modal on onSuccess (e.g., dispatch a bubbling close-overlay event or explicitly set the modal element’s opened = false).

Suggested change

form.reset();

form.reset();

// Close whichever craft-modal is currently open

const openModal =

document.querySelector<HTMLElement & { opened: boolean }>(

'craft-modal[opened]',

);

if (openModal) {

openModal.opened = false;

}

Copilot · 2026-03-25T20:25:51Z

resources/js/pages/SettingsSitesIndex.vue

@@ -242,6 +263,87 @@
            </craft-action-item>


The “Delete Group” action uses @click.prevent, but nothing here closes the action menu. With the recent removal of click-to-close behavior from CraftActionMenu, this can leave the menu open after triggering delete/confirm. Consider explicitly closing the menu (e.g., siteGroupActionsMenu.value.opened = false or dispatching close-overlay) in handleDeleteClick() or the click handler.

Copilot · 2026-03-25T20:25:51Z

resources/js/components/ModalFormNew.vue

+      <!-- Forward all slots from parent to Pane -->
+      <template v-for="(_, slotName) in $slots" :key="slotName" #[slotName]>
+        <slot :name="slotName"></slot>
+      </template>
+
+      <slot></slot>
+      <template #secondary-action>


The slot-forwarding loop includes the default slot (slotName === 'default'), and the component also renders <slot></slot> separately. This will render the default slot content twice inside <Pane>. Exclude the default slot from the loop or remove the extra <slot></slot>.

Copilot · 2026-03-25T20:25:51Z

packages/craftcms-cp/src/components/action-menu/action-menu.ts

  _defineOverlayConfig() {
    return {
      ...withDropdownConfig(),
    };
  }

-  private _addEventListeners() {
-    // Close the menu when an item is clicked.
-    // @TODO is this good or bad?
-    this.actionItems.forEach((item) => {
-      item.addEventListener('click', (e) => {
-        e.target?.dispatchEvent(new Event('close-overlay', {bubbles: true}));
-      });
-    });
-  }
-
-  private _setupInvoker() {
-    const firstInvoker = this.invokerNodes[0];
-    if (firstInvoker) {
-      firstInvoker.setAttribute('id', `invoker-${this.uid}`);
-      firstInvoker.setAttribute('aria-controls', `content-${this.uid}`);
+  private __setupInvoker() {
+    const invoker = this._overlayInvokerNode;
+    if (invoker) {
+      invoker.setAttribute('id', `invoker-${this.uid}`);
+      invoker.setAttribute('aria-controls', `content-${this.uid}`);
    }
  }

-  private _setupContent() {
-    const firstContent = this.contentNodes[0];
-    if (firstContent) {
-      firstContent.setAttribute('id', `content-${this.uid}`);
-      firstContent.setAttribute('role', 'none');
+  private __setupContent() {
+    const content = this._overlayContentNode;
+    if (content) {
+      content.setAttribute('id', `content-${this.uid}`);
+      content.setAttribute('role', 'none');
    }
  }

  override _setupOverlayCtrl() {
    super._setupOverlayCtrl();
-    this._setupInvoker();
-    this._setupContent();
+    this.__setupInvoker();
+    this.__setupContent();
  }

  override firstUpdated() {
    this.uid = uuid();
-    this._addEventListeners();
  }


CraftActionMenu no longer closes when a menu item is clicked (the previous click listeners that dispatched close-overlay were removed). If withDropdownConfig() doesn’t hide on inside clicks, this is a UX regression across all menus. Consider restoring click-to-close behavior via event delegation on _overlayContentNode (or an overlay config option) so selecting an action consistently closes the menu.

…ed in the web component

brianjhanson

I think the modal updates look great! I still lean towards using the name dialog instead of modal, mainly because of the <dialog/> element, but it’s not a huge deal to me.

Most of my comments are minor details, but my main question is about how we’re managing action menus. I think action menus should close when an item is selected (maybe that’s a bit of a stretch), but even if they don’t, I believe we need to find a way to handle their state that doesn’t require the parent to manage all the action menus inside it.

I’m not entirely sure what that entails. It seems like something that might be covered by Lion’s OverlayController, but I’d need to dive a bit deeper into how that works and what it does.

brianjhanson · 2026-03-26T14:29:07Z

packages/craftcms-cp/src/components/modal/modal.ts

+    }
+  }
+
+  override updated(c: PropertyValues<this>) {


What does c stand for in this case? In general, I'm a fan of long, explicit varaible names over brevity.

Ah, agreed! Changed to changed

brianjhanson · 2026-03-26T14:32:55Z

resources/js/pages/SettingsSitesIndex.vue

+  const MODAL_ACTIONS = {
+    rename: {
+      label: t('Rename Group'),
+    },
+    create: {
+      label: t('New Group'),
+    },
+  };


I feel like it would be easier just to throw these into the components vs. abstracting them up here

brianjhanson · 2026-03-26T14:37:02Z

resources/js/components/ModalFormNew.vue

+  <form @submit.prevent="submitHandler">
+    <Pane :title="title">


Pane takes an as parameter so you can determine what element will be used to render it.

Suggested change

<form @submit.prevent="submitHandler">

<Pane :title="title">

<Pane as="form" :title="title" @submit.prevent="submitHandler">

brianjhanson · 2026-03-26T14:45:47Z

resources/js/pages/SettingsSitesIndex.vue

+    if (siteGroupActionsMenu.value) {
+      siteGroupActionsMenu.value.opened = false;
+    }


It seems a bit overbearing for the page level component to have to manage which action menus are open on any given page. Could we make action menus more independent?

Is there some kind of blanket rule? Like when a modal opens, make sure all action menus are closed first?

I'm just thinking on complex pages, we could have 20 – 30 lines of action menu state management making the component harder to parse.

brianjhanson · 2026-03-26T14:46:42Z

resources/js/pages/SettingsSitesIndex.vue

+    if (renameGroupModal.value) {
+      renameGroupModal.value.opened = true;
+    }


For this one, I think we can just use a regular old ref and throw it on the :opened prop

const renameModalOpen = ref(false); // Then <craft-modal :opened="renameModalOpen">

Update --c-color-brand-on-loud to meet min contrast

4c853f9

gcamacho079 added the accessibility 👤 features related to accessibility label Mar 13, 2026

gcamacho079 added 12 commits March 15, 2026 16:49

Add shadow dom testing library package, add ts definition, update dia…

8870c05

…log story to test focus management, update color tokens to pass automated tests

Start refactoring dialog/modal to use Lion component

8bc92c9

Rename dialog to modal

f57df1f

Rename dialog to modal for clarity, refactor to extend overlay mixin

6ca856d

Start refactoring SettingsSitesIndex to use craft-modal component, an…

842a367

…d begin refactoring the ModalForm via ModalFormNew

Run build

e0ba8c3

Continue refactor of action-menu

c4b6669

Fix the nesting

fe84f7e

Remove comment, add labels to create/rename modals

5102b38

Remove unused imports from action-menu.ts, set the modal name on the …

0a952b1

…contentNode after firstUpdate

Add name to craft-modals

cccc78a

Finish adding name property to modal

92ab155

gcamacho079 requested review from brianjhanson and Copilot March 25, 2026 20:20

Copilot started reviewing on behalf of gcamacho079 March 25, 2026 20:21 View session

Copilot AI reviewed Mar 25, 2026

View reviewed changes

gcamacho079 added 2 commits March 25, 2026 13:57

Remove overlay default value from ModalFormNew since overlay is handl…

bfd3650

…ed in the web component

Set default name value to null, handle in __setAccessibleName

5b62a15

brianjhanson reviewed Mar 26, 2026

View reviewed changes

Update changed param name, try a new method for handling close on click

58f2069

	export {default as CraftModal} from '@src/components/modal/modal.js';
	export {default as CraftModal} from './components/modal/modal.js';

-        form.reset();
+        form.reset();
+        // Close whichever craft-modal is currently open
+        const openModal =
+          document.querySelector<HTMLElement & { opened: boolean }>(
+            'craft-modal[opened]',
+          );
+        if (openModal) {
+          openModal.opened = false;
+        }

	<form @submit.prevent="submitHandler">
	<Pane :title="title">
	<Pane as="form" :title="title" @submit.prevent="submitHandler">

Conversation

gcamacho079 commented Mar 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related issues

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

brianjhanson left a comment

Choose a reason for hiding this comment

Uh oh!

brianjhanson Mar 26, 2026

Choose a reason for hiding this comment

Uh oh!

gcamacho079 Mar 26, 2026

Choose a reason for hiding this comment

Uh oh!

brianjhanson Mar 26, 2026

Choose a reason for hiding this comment

Uh oh!

brianjhanson Mar 26, 2026

Choose a reason for hiding this comment

Uh oh!

brianjhanson Mar 26, 2026

Choose a reason for hiding this comment

Uh oh!

brianjhanson Mar 26, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gcamacho079 commented Mar 13, 2026 •

edited

Loading