feat: re-add mediumzoom as a tracked npm dependency - #133
Conversation
|
|
oc-tmueller
left a comment
oc-tmueller
left a comment
There was a problem hiding this comment.
Nice cleanup — dropping the checked-in minified bundle for a real npm dependency is the right direction, and I verified the mechanics hold up:
node_modules/medium-zoom@1.1.0/dist/medium-zoom.min.jsis byte-identical (md5348914dc…) to the bundle #130 committed, so this is a true re-land with no content drift.- Built the branch end to end (
npm ci && npm run antora): green, andassets/js/vendor/medium-zoom.min.js,assets/js/vendor/LICENSE-medium-zoom.txtandassets/css/medium-zoom.cssall publish and are referenced correctly despite the new.gitignoreentry — Antora'ssrcSupplementalFilesglobs the filesystem, not git. - The stacking claim in the CSS comment checks out for 1.1.0: the built
site.cssmaxes out atz-index: 4, Pagefind's modal is9999, the library's injected stylesheet sets noz-index, and the zoomed clone is appended to<body>after the overlay, so the 1000/1000 tie resolves in the image's favour. - The init selector binds nothing unintended: there are zero
<a class="image">wrappers in the built site, and both logos sit outsidespan.image/div.imageblock.
One thing I'd like addressed before merge, plus a few non-blocking notes.
Please add a guard for the missing bundle (blocking)
Moving the bundle from a tracked file to a gitignored build artifact introduces a silent-failure path that nothing currently catches. Verified empirically: with ui/supplemental/js/vendor/ absent, npx antora --stacktrace site.yml exits 0, publishes every page with a <script defer src=".../assets/js/vendor/medium-zoom.min.js"> that 404s, and medium-zoom-init.js:17
if (typeof window.mediumZoom !== 'function') returnswallows the missing global. Result: image zoom is dead site-wide, with a green build and nothing in the build log.
The feature now depends entirely on npm's pre hook firing — i.e. on every present and future build path going through npm run antora / antora-local. npx antora site.yml, npm ci --ignore-scripts, or a future antora:* script added without its own pre twin all reintroduce it.
This repo already guards exactly this "builds green but the output is wrong" class in test/static-files.test.js and test/go-redirect.test.js, and CI runs npm test after npm run antora, so it's a one-liner in static-files.test.js:
assert.ok(fs.existsSync(path.join(PUBLIC, 'assets/js/vendor/medium-zoom.min.js')))Non-blocking
ui/supplemental/css/medium-zoom.css:8 — narrow the z-index: 1000 selector. img.medium-zoom-image matches every bound image permanently: medium-zoom adds that class at attach time and only removes it on detach(). It's inert today only because nothing in site.css/owncloud.css/tabs.css positions doc images. The first rule that gives a doc image position: relative/absolute/sticky — a caption overlay, a figure badge — would promote every image on the page to z-index: 1000, painting over the navbar and sticky toolbar (max z-index: 4). The library adds medium-zoom-image--opened to exactly the clone(s) it appends, and always before appendChild, so img.medium-zoom-image--opened is behaviour-identical today and removes the latent trap.
ui/supplemental/css/medium-zoom.css:7 — the overlay rule ties on specificity with upstream. Our .medium-zoom-overlay rule and the library's own are both (0,1,0), and the library injects its stylesheet via styleInject() → head.appendChild(style) at script-execution time, i.e. after our <link>. The comment's premise ("that stylesheet sets no stacking order") holds for 1.1.0, but medium-zoom minors land auto-grouped in the npm-minor-patch Dependabot PR — an upstream release that adds a z-index to that block would silently win the cascade and put the navbar back over the backdrop, with a green build and no diff here. body > .medium-zoom-overlay (or !important) makes us independent of upstream staying z-index-free.
scripts/sync-vendor-assets.js:22 — the step isn't idempotent. The loop only ever copies; it never prunes a destination that is now gitignored and therefore invisible to git status. If medium-zoom is later dropped from VENDOR_FILES and package.json, CI builds from a fresh checkout and correctly publishes a site without it, while every existing local checkout (and any reused/self-hosted-runner workspace) still has the stale file on disk and keeps serving it via npm run antora-local. A fs.rmSync(path.join(ROOT, 'ui/supplemental/js/vendor'), { recursive: true, force: true }) before the loop fixes it.
README.md — new paths aren't documented. The PR adds a top-level scripts/ directory and a generated-but-untracked ui/supplemental/js/vendor/ tree, and neither appears in the README's ## Layout tree (which lists antora-extensions/, asciidoc-extensions/, ui/supplemental/, sync/), in ## Build locally, or in AGENTS.md. Right now the only record that js/vendor/ is generated rather than deleted-by-accident is the .gitignore comment.
Checked and cleared
package as a destructuring property key under 'use strict' (legal — reserved words are valid property names); no dist/style.css sync needed (the UMD bundle inlines its CSS); defer ordering (deferred classic scripts execute in document order, so the init script always sees the global); .txt LICENSE publication (Antora's classifyFile types it as an asset — confirmed published); medium-zoom in dependencies rather than devDependencies (consistent with antora itself, and the package is private: true).
|
Addressed in bc881f6 (+ acbcfdf, see below): Blocking — missing bundle guard. Added the assertion to While wiring that up, found it wasn't actually correct as suggested — CI's Non-blocking, all applied:
Re-verified end to end: |
The previous vendored copy of medium-zoom.min.js was reverted (#131) for missing dependency management. Install medium-zoom via npm so it is tracked in package.json/package-lock.json (and covered by Dependabot), and copy its prebuilt browser bundle into ui/supplemental/js/vendor/ via a preantora/preantora-local script, since the site has no client-side bundler of its own. Signed-off-by: Lukas Hirt <info@hirt.cz>
Replace the two hardcoded copyFileSync calls with a VENDOR_FILES list, so vendoring another package's browser build later means adding an entry instead of writing a new script. Signed-off-by: Lukas Hirt <info@hirt.cz>
- Add a build-output guard (extension-tests/static-files.test.js) that fails loudly if ui/supplemental/js/vendor/ wasn't repopulated before the build (e.g. npx antora run directly, bypassing the preantora hook): today it publishes green with a 404'ing <script> and image zoom silently dead site-wide. - Make scripts/sync-vendor-assets.js wipe js/vendor/ before repopulating it, so dropping an entry from VENDOR_FILES actually removes the stale file instead of leaving it on disk in any checkout that already had it. - Narrow the CSS z-index selector to img.medium-zoom-image--opened (the class only the currently-zoomed clone gets) instead of the bare medium-zoom-image class, which stays on every bound image for as long as it's attached and would promote them all to z-index 1000 the moment anything positions a doc image. - Add `body >` to the overlay selector so our rule outranks the library's own .medium-zoom-overlay by specificity rather than by injection order, in case a future medium-zoom release adds its own z-index there. - Document scripts/ and the generated ui/supplemental/js/vendor/ in README.md's layout tree. Signed-off-by: Lukas Hirt <info@hirt.cz>
5a3aeac to
182103c
Compare
|
Split the test-suite fixes out into #135, since they're unrelated to medium-zoom (broken since #119, discovered while making the blocking review guard actually run). This PR is now stacked on #135's branch and its diff is purely the medium-zoom changes — once #135 merges, GitHub will retarget this PR at |
Pull Request is not mergeable
Pull Request is not mergeable
Pull Request is not mergeable
Pull Request is not mergeable
* test: repair the extension-tests suite broken by the test/ rename #119 renamed test/ to extension-tests/ (and flattened test/helpers/ into extension-tests/) but left several things pointing at the old layout: package.json's test script still globbed test/*.test.js (so npm test silently ran zero tests, in CI too), latest-alias.test.js / next-alias.test.js / static-files.test.js still required ./helpers/latest-versions, and latest-versions.js itself still walked up two directories to find content/ instead of one. Net effect: most of this suite has been dead since #119, always green because nothing ran. Signed-off-by: Lukas Hirt <info@hirt.cz> * test: don't flag unrendered content/ocis/8.0 as a broken page alias Restoring npm test above turned this test back on, which surfaces it: content/ocis/8.0 has a page-aliases entry but isn't in site.yml's ocis source list (only 8.1-8.3 render; site.yml's own comment says an unnamed version folder "could safely be deleted if outdated"). The test walks the whole content/ tree, so it flagged that stale, never- published version as a dangling alias. Skip versions this build doesn't publish, matching the builtOrSkip pattern the rest of the suite already uses; verified it still fails when a real published stub (8.1) is removed. Signed-off-by: Lukas Hirt <info@hirt.cz> * feat: re-add mediumzoom as a tracked npm dependency (#133) * feat: re-add mediumzoom as a tracked npm dependency The previous vendored copy of medium-zoom.min.js was reverted (#131) for missing dependency management. Install medium-zoom via npm so it is tracked in package.json/package-lock.json (and covered by Dependabot), and copy its prebuilt browser bundle into ui/supplemental/js/vendor/ via a preantora/preantora-local script, since the site has no client-side bundler of its own. Signed-off-by: Lukas Hirt <info@hirt.cz> * refactor: make sync-vendor-assets.js data-driven Replace the two hardcoded copyFileSync calls with a VENDOR_FILES list, so vendoring another package's browser build later means adding an entry instead of writing a new script. Signed-off-by: Lukas Hirt <info@hirt.cz> * fix: address review on the medium-zoom vendoring approach - Add a build-output guard (extension-tests/static-files.test.js) that fails loudly if ui/supplemental/js/vendor/ wasn't repopulated before the build (e.g. npx antora run directly, bypassing the preantora hook): today it publishes green with a 404'ing <script> and image zoom silently dead site-wide. - Make scripts/sync-vendor-assets.js wipe js/vendor/ before repopulating it, so dropping an entry from VENDOR_FILES actually removes the stale file instead of leaving it on disk in any checkout that already had it. - Narrow the CSS z-index selector to img.medium-zoom-image--opened (the class only the currently-zoomed clone gets) instead of the bare medium-zoom-image class, which stays on every bound image for as long as it's attached and would promote them all to z-index 1000 the moment anything positions a doc image. - Add `body >` to the overlay selector so our rule outranks the library's own .medium-zoom-overlay by specificity rather than by injection order, in case a future medium-zoom release adds its own z-index there. - Document scripts/ and the generated ui/supplemental/js/vendor/ in README.md's layout tree. Signed-off-by: Lukas Hirt <info@hirt.cz> --------- Signed-off-by: Lukas Hirt <info@hirt.cz> --------- Signed-off-by: Lukas Hirt <info@hirt.cz>
3 participants
Summary
medium-zoom.min.jswas hand-vendored intoui/supplemental/js/vendor/with no dependency tracking.medium-zoomvia npm instead, so it's tracked inpackage.json/package-lock.jsonand covered by Dependabot/audits.ui/supplemental/is layered onto the stock Antora UI bundle as flat files), addsscripts/sync-vendor-assets.js, wired aspreantora/preantora-local, to copy the package's prebuilt browser bundle + license intoui/supplemental/js/vendor/before each build. That directory is now gitignored (generated, not committed).medium-zoom-init.jsinit script,medium-zoom.css(z-index fix for the navbar) and thefooter-scripts.hbs/head-styles.hbsreferences, same as the original feat: re-add mediumzoom #130.Test plan
npm run antorabuilds successfully;preantoracopies the vendor files automatically.public/index.htmlreferencesjs/vendor/medium-zoom.min.js,js/medium-zoom-init.js, andcss/medium-zoom.css, and all three are published topublic/assets/.node --test extension-tests/*.test.jsshows the same pre-existing failures with and without this change (unrelated to this PR).