Refactor: replace jQuery dialog with webcomponent - #1649
Draft
SheepFromHeaven wants to merge 8 commits into
Draft
Conversation
✅ Deploy Preview for afmg ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
…release-branches ci: run PR checks on release branches
Replaces jQuery UI's .dialog() chrome (title bar, minimize, close, actions) with a self-contained, jQuery-free web component using real Shadow DOM slots for content and actions. The Units Editor is fully rebuilt as its own <units-editor-dialog> component that owns both its markup and its previously controller-only logic, wrapping <ui-dialog> internally. Also fixes tooltips.ts's showDataTip to read event.composedPath()[0] instead of event.target, since Shadow DOM retargets target to the host for listeners outside the shadow tree - needed for the new dialog's minimize/close tooltips, and for any future shadow-DOM component. Known gaps to close next: resizing (jQuery UI dialogs default to resizable: true) and Escape-to-close are not yet implemented in <ui-dialog>.
jQuery UI dialogs default to resizable: true, and the pre-migration Units Editor relied on that default, so this was a real regression versus the old dialog. Adds a corner resize handle, switches the host to a flex column layout so the content area fills the resized space with its own scrolling, and keeps sizing correct across interactions: the resize clamp accounts for the dialog's current position (not just viewport size) so it can't be dragged past the edge, and minimizing after a manual resize collapses to the title bar instead of leaving an empty resized box. Escape-to-close needed no new code: the app's existing global Escape handler already calls closeDialogs(), which was extended earlier to close open <ui-dialog> elements too.
… e2e tests to <ui-dialog>
Resizing previously only worked from the SE corner and showed a visible
diagonal-line affordance; jQuery UI resizable dialogs expose all 4 edges
and 4 corners with no visible handle, just a resize cursor. Also fixes a
real regression found while testing: shrinking the dialog narrower than
its content caused label/slider rows to wrap and squish instead of the
content keeping its natural size and scrolling horizontally, which is
what the old jQuery UI dialog did.
Adds the accessibility behavior jQuery UI dialogs provided for free:
role="dialog", aria-label, aria-modal, focus moves into the dialog on
open and is Tab/Shift+Tab-trapped inside it, and closing restores focus
to whatever had it beforehand.
Adds a boolean `resizable` attribute (default true, matching jQuery UI's
own default) so dialogs that explicitly disabled resizing can opt out
once migrated - most existing .dialog({...}) call sites pass
resizable: false.
Replaces ad hoc Playwright scripts used to verify all of the above with
a proper tests/e2e/ui-dialog.spec.ts, following this repo's existing e2e
conventions.
Standing style preference: no added code comments and no em-dashes, regardless of project convention. Strips the comments and em-dashes I introduced across the ui-dialog/units-editor-dialog migration commits, leaving pre-existing comments in dialog-helpers.ts and units-editor.ts untouched.
…e component Per PR discussion: this PR should only introduce <ui-dialog> itself, not also migrate a consumer. Reverts src/controllers/units-editor.ts and its markup in src/index.html to their pre-migration state (jQuery UI dialog, byte-identical to the version before this branch started), and removes src/components/units-editor-dialog/ entirely. Also fixes a real bug this surfaced: UiDialog set role/aria-modal/tabindex in its constructor, which throws NotSupportedError when the element is created via document.createElement (a real browser restriction, not specific to this component) - moved that into build()/connectedCallback, which is also the general best practice for custom elements. tests/e2e/ui-dialog.spec.ts now creates a standalone <ui-dialog> test fixture instead of driving it through the Units Editor UI, so it no longer depends on a consumer existing in the app. The removed Units Editor migration is preserved on refactor/dialogs for a follow-up PR.
Addresses review comment on close(): many editors use the app's
destroyDialog(id) helper (jQuery .dialog("destroy") + DOM removal) for
dialogs that get rebuilt fresh on each open, rather than just hiding
them. <ui-dialog> had no equivalent, so destroy() removes the element
from the DOM and restores focus to the opener, and destroyDialog() now
calls it for <ui-dialog> elements the same way closeDialogs() already
does for close().
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
It combines a substantial UI refactor with additional behavioral changes (economy/zoom/CI), and there are unresolved accessibility and runtime-safety issues identified in the new dialog/tooltip code.
Pull request overview
Refactors the Units Editor dialog from a legacy jQuery UI dialog + inline index.html markup into a custom element (<units-editor-dialog> using a reusable <ui-dialog> web component), and adds Playwright E2E coverage for the new dialog behavior.
Changes:
- Replace the inline
#unitsEditordialog markup with<units-editor-dialog>and implement the new dialog using a reusable<ui-dialog>web component. - Add Playwright E2E tests covering open/close, minimize, resize, focus wrapping, and focus restoration behavior.
- Include several additional updates (tooltip event retargeting, zoom redraw scheduling tweaks, production worker cap logging/behavior, CI branch filters, version bump).
File summaries
| File | Description |
|---|---|
| tests/e2e/ui-dialog.spec.ts | Adds E2E coverage for the new <ui-dialog> behavior via Units Editor. |
| src/services/versioning.ts | Bumps app version. |
| src/index.html | Replaces legacy Units Editor dialog markup with <units-editor-dialog>. |
| src/generators/production-generator.ts | Caps burg population used for production (performance/behavior change) and logs when capped. |
| src/controllers/index.ts | Redirects UnitsEditor controller import to the new component-based implementation. |
| src/components/zoom.ts | Adjusts zoom redraw scheduling and avoids zoom-end work when view didn’t change. |
| src/components/units-editor-dialog/units-editor-dialog.ts | Implements <units-editor-dialog> wrapper around <ui-dialog> and wires existing unit-setting handlers. |
| src/components/units-editor-dialog/units-editor-dialog.html | Moves Units Editor markup into a template used by the custom element. |
| src/components/ui-dialog/ui-dialog.ts | Introduces reusable <ui-dialog> web component (drag, resize, focus trap, open/close). |
| src/components/ui-dialog/ui-dialog.html | Defines <ui-dialog> shadow DOM structure (titlebar/content/actions/resize handles). |
| src/components/ui-dialog/ui-dialog.css | Adds styling for <ui-dialog>. |
| src/components/ui-dialog/ui-dialog-slotted-content.css | Adds global styles for common controls inside <ui-dialog>. |
| src/components/tooltips.ts | Adjusts tooltip targeting to better handle composed/shadow events. |
| src/components/index.ts | Ensures new web components are registered at startup. |
| src/components/dialog/dialog-helpers.ts | Extends dialog closing helper to also close open <ui-dialog> instances. |
| package-lock.json | Lockfile churn from dependency metadata changes. |
| .github/workflows/unit-tests.yml | Runs unit tests on master and version-like branches. |
| .github/workflows/playwright.yml | Runs Playwright tests on master and version-like branches. |
Review details
Suppressed comments (1)
src/components/ui-dialog/ui-dialog.css:47
- The titlebar is the draggable surface; set
user-select: nonehere (instead of on the whole dialog) to avoid text selection during drag while keeping content selectable.
color: #ffffff;
background: var(--header, #997787);
border-bottom: 1px solid var(--dark-solid, #666666);
cursor: move;
}
- Files reviewed: 17/18 changed files
- Comments generated: 5
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+1
to
+4
| <div class="ui-dialog-titlebar"> | ||
| <span class="ui-dialog-title"></span> | ||
| <button type="button" class="ui-dialog-titlebar-collapse" data-tip="Minimize / maximize">_</button> | ||
| <button type="button" class="ui-dialog-titlebar-close" data-tip="Close">✕</button> |
Comment on lines
+14
to
+19
| background-color: var(--bg-dialogs, #ffffff); | ||
| border: 1px solid var(--dark-solid, #999999); | ||
| font-family: var(--sans-serif); | ||
| color: #333333; | ||
| user-select: none; | ||
| } |
| <label>1 population point:</label> | ||
| </slider-input> | ||
| </div> | ||
| <div data-tip="Set urban population modifier. Change to increase or descrese burgs population"> |
Comment on lines
48
to
52
| export function showDataTip(event: Event): void { | ||
| const target = event.target as HTMLElement | null; | ||
| const target = (event.composedPath()[0] ?? event.target) as HTMLElement | null; | ||
| if (!target) return; | ||
|
|
||
| const parent = target.parentNode as HTMLElement | null; |
Comment on lines
+101
to
110
| const burgPopulation = rn(burg.population || 0, 2); | ||
| const population = Math.min(burgPopulation, MAX_WORKERS); | ||
| if (population < burgPopulation) { | ||
| WARN && | ||
| console.warn( | ||
| `Burg ${burg.name} (${burg.i}) population ${burgPopulation} is capped at ${MAX_WORKERS} for production` | ||
| ); | ||
| } | ||
| const inventory: number[] = []; | ||
| const demandTargets = getDemandTargets(population); |
SheepFromHeaven
force-pushed
the
refactor/dialogs
branch
from
August 27, 2026 15:36
864b29e to
b667ea5
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description