Skip to content

refactor(edit-content): share side-panel behaviour across panels (#37965) - #37970

Open
fmontes wants to merge 1 commit into
issue-37965-shared-velocity-grammarfrom
issue-37965-side-panel-helpers
Open

fmontes wants to merge 1 commit into
issue-37965-shared-velocity-grammarfrom
issue-37965-side-panel-helpers

Conversation

@fmontes

@fmontes fmontes commented Oct 9, 2026 •

Copy link
Copy Markdown
Member

Proposed Changes

  • injectSidePanelChrome (edit-content/src/lib/services/dot-side-panel-chrome.ts) gives a side panel the behaviour every side panel shares: Escape through the shortcut registry, a click on its own mask, joining and leaving the side-panel stack, and the full-width preference.
  • The Edit Content side panel and Content Drive's legacy-editor panel use it, so each keeps only its own content and close guard. The copies of the full-width preference helpers and of the Escape, mask and stack code are gone.
  • No behaviour change: same keys, same guards, same stored preference.

Checklist

  • Tests (a new spec for the helper; both panels' existing specs pass unchanged)
  • Translations (none needed)
  • Security Implications Contemplated (UI behaviour only)

Additional Info

Refs #37965

🤖 Generated with Claude Code

)

Escape, a click on the mask, the side-panel stack and the full-width preference were copied into each side panel. injectSidePanelChrome now gives every panel the same behaviour; the Edit Content panel and Content Drive's legacy-editor panel use it, so each only writes its own content and close guard.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@fmontes
fmontes added this pull request to stack #37971 October 9, 2026 16:59
@github-actions github-actions Bot mentioned this pull request Oct 9, 2026
3 tasks
@claude

claude Bot commented Oct 9, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @fmontes's task in 1m 51s —— View job


Code Review: PR #37970

  • Read the new shared service and spec
  • Review both refactored side-panel components
  • Analyze for bugs, design issues, test gaps
  • Post review

This PR extracts the shared side-panel behaviour (Escape, mask-click close, side-panel stack, full-width toggle) from DotEditContentSidePanelComponent and DotLegacyEditorSidePanelComponent into a single injectSidePanelChrome helper. I checked the extraction for behavior equivalence, injection-context correctness, teardown symmetry, and change-detection implications.

New Issues

No issues found.

The refactor is behavior-preserving and the risky spots check out:

  • Injection context / field-init order — injectSidePanelChrome calls inject(), afterNextRender(), and inject(DestroyRef).onDestroy(), all valid because it runs from a field initializer (constructor-time injection context). In both components #chrome is initialized before $expanded = this.#chrome.expanded, so the ordering holds. The $drawer signal is passed by reference (not read), so the viewChild being undefined at construction is fine.
  • Change detection on mask click — the handler moved from an Angular (document:click) host binding to a raw document.addEventListener. I verified the app is zone-based (apps/dotcms-ui/src/polyfills.ts:49 imports zone.js), so the raw listener is still monkey-patched and runs inside the Angular zone → CD still fires after requestClose(). No regression. (Worth a note if this app ever goes zoneless, since a raw listener wouldn't schedule CD the way the host binding does — but that's not this PR.)
  • Teardown symmetry — DestroyRef.onDestroy releases the nav stack, withdraws the Escape claim, and removes the click listener; both ngOnDestroys correctly dropped their now-duplicate teardown. The spec's destroy test confirms Escape/mask stop acting after destroy.
  • No stale imports — signal is still used in the legacy component (#dirty = signal(false)); afterNextRender/DestroyRef/Injector/takeUntilDestroyed are still used in the edit-content component.
  • Escape identity — the registered handler and the returned escape are the same closure, so the legacy iframe-keydown path (this.#chrome.escape()) behaves identically to the registry path.
  • Test coverage — dot-side-panel-chrome.spec.ts covers all four behaviours (Escape frontmost/not-frontmost/forwarded, own-mask vs foreign-mask vs not-frontmost, stack acquire/release, expanded persistence across panels). Good coverage for the extracted unit.

Minor (non-blocking)

  • dot-legacy-editor-side-panel.component.ts:131 passes escapeLabel: 'edit.content.side-panel.shortcut.close' — an edit.content.* i18n key used from the content-drive portlet. This cross-library key reuse is pre-existing (the legacy panel used the same key before this PR), so not introduced here, but it's a small coupling worth keeping in mind if the shortcut label ever needs to differ per panel.
    · issue-37965-side-panel-helpers

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

1 participant