Add image lightbox for documentation images - #47352
Conversation
✅ Deploy Preview for home-assistant-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
@mrdarrengriffin Hi! Would you mind giving this a look? I'm not sure whether vendoring this is the best approach. This isn't urgent. |
There was a problem hiding this comment.
Pull request overview
Adds an on-page image lightbox for documentation and blog images by vendoring PhotoSwipe and wiring it into the site so readers can open/zoom screenshots without navigating away.
Changes:
- Vendor PhotoSwipe (JS + CSS) locally and include MIT license + update instructions.
- Add a small adapter (
image-lightbox.js) that makes eligible images interactive and opens them in PhotoSwipe. - Hook the feature into the global site layout (CSS in
<head>, JS in scripts include, plus SCSS overrides for focus/cursor).
Reviewed changes
Copilot reviewed 9 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| source/stylesheets/photoswipe/photoswipe.css | Vendored PhotoSwipe stylesheet. |
| source/stylesheets/photoswipe/LICENSE | MIT license copy for the vendored stylesheet. |
| source/javascripts/photoswipe/README.md | Documents vendoring/update procedure and version sync point. |
| source/javascripts/photoswipe/photoswipe.esm.min.js | Vendored PhotoSwipe core module (minified). |
| source/javascripts/photoswipe/photoswipe-lightbox.esm.min.js | Vendored PhotoSwipe lightbox module (minified). |
| source/javascripts/photoswipe/LICENSE | MIT license copy for the vendored JS. |
| source/javascripts/image-lightbox.js | Site adapter: selects eligible images, adds keyboard/click handling, and opens PhotoSwipe. |
| source/_includes/site/head.html | Adds the PhotoSwipe CSS to the site head. |
| source/_includes/javascripts/scripts.html | Loads the image lightbox adapter site-wide. |
| sass/homeassistant/base/_image_lightbox.scss | Adds focus-visible styling and minor PhotoSwipe overrides. |
| sass/homeassistant/_homeassistant.scss | Includes the new lightbox SCSS in the main bundle. |
Suppressed comments (1)
source/javascripts/image-lightbox.js:211
- If the dynamic import fails (offline, blocked request, etc.), this promise rejection will currently bubble up as an unhandled rejection. Add a catch around the open call (even a no-op) to avoid noisy console errors and keep the page functional.
event.preventDefault();
getLightbox().then((lightbox) => {
lightbox.loadAndOpen(
index,
getLightboxItems(images),
getInitialPoint(event)
);
});
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 11 changed files in this pull request and generated no new comments.
Suppressed comments (4)
source/javascripts/image-lightbox.js:74
- The filename heuristic also excludes substantial screenshots whose names contain
logoorbrands. For example,custom_logo.pngandsupported-brands.pngare screenshots with screenshot alt text, but both fail this check. Apply the existing screenshot exception to the path heuristic as well so these images remain eligible.
/(?:^|[-_/])(logo|logos|brand|brands|wordmark)(?:[-_.\/]|$)/i.test(path) ||
((/\blogo(s)?\b|\bwordmark\b/.test(alt)) &&
!alt.includes("screenshot"))
source/javascripts/image-lightbox.js:173
- The PhotoSwipe root is created with
role="dialog", but neither the vendored module nor this adapter gives it an accessible name. As a result, screen readers announce an unnamed dialog. Register a PhotoSwipe lifecycle handler (for example,uiRegister) that sets anaria-labelsuch as “Image viewer” onlightbox.pswp.element.
return new PhotoSwipeLightbox({
source/javascripts/image-lightbox.js:196
- The PR description says a tap zooms the image in and out, but this setting makes a single tap only toggle the controls; zoom requires a double-tap or pinch. Set the tap action to
zoom, or update the documented interaction if the current behavior is intentional.
tapAction: "toggle-controls",
sass/homeassistant/base/_image_lightbox.scss:10
- Many documentation screenshots are inside
p.img, which hasoverflow: hidden; when an image fills that container, this outward focus outline is clipped on the container edges. Use an inset outline so keyboard users retain a visible focus indicator.
outline-offset: 4px;
|
I also asked in HA Discord for a Designer review. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 11 changed files in this pull request and generated no new comments.
Suppressed comments (1)
source/javascripts/image-lightbox.js:205
- The viewer traps focus and blocks interaction with the page, but PhotoSwipe creates only
role="dialog"; this handler adds a label without identifying it as modal. Assistive technologies may therefore continue exposing the underlying page as active content. Setaria-modal="true"on the dialog element when registering the UI.
lightbox.on("uiRegister", () => {
lightbox.pswp.element.setAttribute("aria-label", "Image viewer");
});
| image.naturalWidth && | ||
| image.naturalHeight && | ||
| !image.closest("a, button") && | ||
| !image.closest(".no-lightbox, [data-no-lightbox]") && |
There was a problem hiding this comment.
Should we document this opt-out somewhere (create page doc ?)
There was a problem hiding this comment.
| const alt = (image.getAttribute("alt") || "").toLowerCase(); | ||
| const isScreenshot = alt.includes("screenshot"); |
There was a problem hiding this comment.
Alt text is the wrong place for it. It describes the image for screen reader users, and once it also controls the lightbox, people will write "screenshot" to get the zoom rather than to describe the image.
It also does nothing today. I wan not find any image matching the logo path or alt rules contains "screenshot", so it changes the outcome for zero images.
Can we drop isScreenshot? If a real screenshot ever gets caught, an explicit opt-in would be clearer, and symmetric with data-no-lightbox.
Proposed change
Add a lightbox for larger documentation images, so screenshots can be opened and zoomed without leaving the page.
The implementation uses PhotoSwipe 5.4.4, vendored into the website and served locally. A small adapter enables it only for substantial images inside documentation and blog articles. It skips linked images, buttons, small inline images, SVGs, and brand logos.
Navigation:
Privacy and maintainability notes:
Alternatives considered:
package.json: this site does not bundle or serve files fromnode_modules, so that would not work by itself.node_modules: workable, but more moving parts for this static Jekyll site.Comparisons
Type of change
currentbranch).currentbranch).nextbranch).nextbranch).Additional information
Checklist
currentbranch.nextbranch.