Skip to content

refactor: extract the gallery modal into a shared lightbox service - #2059

Draft
joelabreo227 wants to merge 1 commit into
mainfrom
feat/shared-lightbox-service
Draft

refactor: extract the gallery modal into a shared lightbox service#2059
joelabreo227 wants to merge 1 commit into
mainfrom
feat/shared-lightbox-service

Conversation

@joelabreo227

Copy link
Copy Markdown

Deliverables 1 and 2 of rtCamp/godam-plugin-wp#25. No new user-facing feature: this is the foundation the "Show in lightbox" setting sits on, landed on its own so the risky part gets reviewed by itself.

Draft until @KMchaudhary has looked at it.

Why

GoDAM has two video lightboxes:

Gallery godam-for-woo
File assets/src/blocks/godam-gallery-v2/view.js assets/src/js/shared/godam-woo-video-modal.js
First shipped 2026-04-08 (#1773) 2026-05-19 (rtCamp/godam-for-woo#75)
Rendering shared modal wrapping a lazy iframe Video.js in the host DOM
Public API none. Only window.GodamGalleryV2 = { init } window.GodamWooVideoModal

The second was written six weeks after the first. The likely reason is not an architectural disagreement: the gallery's modal was a private function inside a block's bundle, so another plugin had no way to call it. Adding a third lightbox for the Video block would have made the same mistake twice.

This PR adds the entry point that was missing, and moves the gallery onto it.

What changed

New: assets/src/js/godam-lightbox/ — the service, published as the godam-lightbox-script handle exposing window.GodamLightbox. A handle rather than a bundled import, specifically so godam-for-woo can consume it later without vendoring anything.

The service owns the modal DOM, focus management, keyboard handling, and the media lifecycle. Callers own their playlist and their own side effects. Rendering sits behind a renderer seam:

  • createIframeRenderer() ships here. It is the gallery's existing approach and the intended default: nothing loads until the lightbox opens, and player layers, ads and skins work because the iframe is a real render of the player template.
  • In-DOM Video.js playback is the second renderer the seam exists for. Deliberately not implemented in this PR. godam-for-woo already has a working in-DOM modal, and that renderer should land with its migration, shaped by its real requirements (WooCommerce cart context, swipe navigation, addon panels), not guessed at now.

Gallery migration. view.js drops 118 lines of modal code and calls the service. It passes its original class names as aliases, so the rendered DOM is byte-identical for CSS and for the QA selectors. Modal styles move to assets/src/css/godam-lightbox.scss, declared against both the canonical godam-lightbox names and the legacy godam-gallery-v2-modal names.

flushIframeAnalytics carried over intact. This is the piece most likely to be lost in a rewrite. Heatmap payloads sent from an iframe that is about to be torn down get cancelled by the browser, so the parent pulls them out and POSTs them itself first. Two tests now pin the flush-before-teardown ordering.

Tests

34 new unit tests (assets/src/js/godam-lightbox/service.test.js). Full suite: 130 passing.

Worth knowing before review: the gallery modal had no automated coverage at all. godam-plugin-automation defines galleryV2.modal selectors, but no spec consumes them, so the ticket's assumption that the existing suite proves no behaviour change does not hold for the modal. These unit tests are the first safety net under this behaviour. A QA spec driving the modal end to end is still worth having, and I have raised that on the ticket.

Things to look at closely

  1. Three paths load the gallery view script, and only one goes through block.json. The Elementor widget (class-godam-gallery.php) and the [godam_video_gallery] shortcode enqueue godam-gallery-v2-view-script directly, so they would have loaded a view.js with no window.GodamLightbox. Both now enqueue the lightbox explicitly. I chose explicit lists because that is what this codebase already does everywhere; the alternative is mutating the registered handle's deps so future callers cannot forget. Happy to switch if you prefer that.

  2. Modal DOM is now built on first open, not on gallery construction. Previously every page with a gallery got the overlay and dialog nodes on load whether or not anyone opened a video. This is a real (small) behaviour change. It is better for the Video block case, and nothing asserts pre-open existence, but it is a change.

  3. Two galleries on one page. Opening gallery B while A's modal is open now fires A's close path, so A's tile previews resume correctly. Previously the shared iframe was silently repointed and A was left thinking it was still open. Unreachable in practice, because the overlay covers the page and body scroll is locked, but the semantics did change.

  4. document.body now also gets the canonical godam-lightbox-open class alongside the legacy one. Additive; both are styled identically.

Not verified locally

PHPCS did not run. Neither composer nor a php CLI is available on the machine I built this on, so vendor/bin/phpcs could not be installed and the pre-commit hook's lint:php task could not execute. I committed with --no-verify for that reason and nothing else. phpcs_on_pull_request.yml covers it here, so please treat the CI result as the gate on the four PHP files. wp-scripts lint-js and lint-style both pass clean; the 5 remaining lint errors in godam-gallery-v2/edit.js and index.js are pre-existing on main and untouched.

No browser verification. I have not clicked through a real gallery in a browser. The unit tests cover the service's logic, not the rendered result, and the CSS move in particular deserves a visual check on a live gallery in grid, carousel and list layouts, plus mobile width.

Follow-ups this sets up

  • Deliverable 3, showInLightbox on the Video block, is now a small change: a new createIframeRenderer call with blockSource: 'video-block-lightbox' and no aliases.
  • Retiring the legacy godam-gallery-v2-modal class names, coordinated with QA so selectors/godam.js changes in the same window.
  • The in-DOM renderer, with the godam-for-woo migration.

Refs rtCamp/godam-plugin-wp#25

🤖 Generated with Claude Code

GoDAM has two video lightboxes. The gallery block built the first one
(April) inside its own view.js with no public entry point, so when
godam-for-woo needed the same thing six weeks later it wrote a second,
independent one. This adds the door that was missing.

The service is a registered script handle (godam-lightbox-script)
exposing window.GodamLightbox, so any surface that can enqueue a script
can drive the same modal, including other plugins. Rendering is behind a
renderer seam: the iframe renderer (the gallery's approach, and the
default) ships here; in-DOM Video.js playback is the second renderer the
seam exists for and lands with the godam-for-woo migration, shaped by
that plugin's real requirements rather than guessed at now.

The gallery keeps its playlist and its tile-preview side effects and
calls the service for everything else. It passes its original class names
as aliases, so the rendered DOM is unchanged for CSS and for the QA
selectors that target those names. The modal styles move to a shared
stylesheet declared against both the canonical and the legacy class
names; retiring the legacy half is a follow-up to coordinate with QA.

flushIframeAnalytics is carried over intact. Sending heatmap payloads
from an iframe that is about to be torn down loses them, so the parent
pulls them out and POSTs them itself first. Two tests now pin the
flush-before-teardown ordering, which is the thing a rewrite would break
silently.

Adds 34 unit tests. The gallery modal previously had none: the QA suite
defines selectors for it but no spec consumes them.

Refs rtCamp/godam-plugin-wp#25

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

@rtBot rtBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code analysis identified issues

action-phpcs-code-review has identified potential problems in this pull request during automated scanning. We recommend reviewing the issues noted and that they are resolved.

phpcs scanning turned up:

⚠️ 1 warning


Powered by rtCamp's GitHub Actions Library


if ( file_exists( $lightbox_path ) ) {
$lightbox_assets = file_exists( $lightbox_asset_path )
? include $lightbox_asset_path

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Warning: File inclusion using variable ($lightbox_asset_path). Probably needs manual inspection (WordPressVIPMinimum.Files.IncludingFile.UsingVariable).

@joelabreo227

Copy link
Copy Markdown
Author

CI status, so nobody has to dig:

  • PHPCS: pass. This was the gate I could not run locally (no composer/php on my machine), so this is the verification for the four PHP files.

  • CodeQL: pass.

  • Run JS unit tests: fail, and not from this branch. npm ci cannot resolve the lock file (Missing: dompurify@3.2.7 from lock file, plus ~20 more). The last 8 runs of this workflow have failed on every branch, develop and chore/2.1.0 included. My commit touches neither package.json nor package-lock.json.

    Worth flagging separately: this means JS unit tests have not actually run in CI for a while, including the 34 tests added here. They pass locally (npx wp-scripts test-unit-js, full suite 130 passing) but nothing is enforcing that on merge. Someone should refresh the lock file.

  • Run Plugin Check: fail, also pre-existing on every recent branch including develop.

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.

2 participants