-
Notifications
You must be signed in to change notification settings - Fork 0
Review 3937
This page is not a review. No review of #3937 was performed at this head, no verdict was reached, and nothing was posted to the PR. It is not counted in the review totals on All reviews. It records why the refresh step stopped.
#3937 feat(lightbox): host arbitrary React content items by
jiunshinn — community contributor, opened
2026-07-14, last pushed 2026-07-21. Labels: needs:code-review, community.
No prior review of any kind exists on the PR.
LOOP VERSION: 1.6.0 AUDIT RUBRIC: not read — the run stopped before gate 1
| PR head fetched |
d802efcbb42c4dbfc69126b51510fcbb7803f1e5 (matches the head GitHub reports) |
upstream main
|
2458b03b35a45512b996b0c2c764d9530a36a052 |
| merge base | 7a3676408fb1a8428fdbe535f1f1c2bb9c179ab9 |
| merge direction |
main merged into the PR head; no rebase, no rewrite of author commits |
| outcome | conflicts in 2 files; merge aborted; nothing pushed, head unchanged |
The PR is 7 files, +522/−89: the Lightbox component, its hook, barrel, docs,
tests, one story file and a changeset. No dependency, lockfile, workflow,
package-script or lifecycle-hook change — the process guard over the changed
files is clean.
packages/core/src/Lightbox/Lightbox.tsx (5 hunks) and
packages/core/src/Lightbox/Lightbox.doc.mjs (2 hunks).
Lightbox moved a long way while this PR sat. Three landed changes account for
all of it:
- #4367 — keyboard zoom and pan
- #3894 — dismiss on backdrop click around the media
- the i18n migration (#4920 and
the
useTranslator()sweep) — everyLightboxstring now comes from the catalog
None of the conflicting hunks is an independent addition, a regenerated
artifact, or moved context with unchanged intent. Each one is a place where a
landed contract and this PR's new type: 'custom' item kind meet and the
correct behaviour is a product decision the author has to make.
1. What a custom item announces, and in which catalog key.
main routes the gallery live-region text through the translator —
t('@astryx.lightbox.mediaPosition', {alt, index, total}), falling back to
@astryx.lightbox.imagePosition (Lightbox.tsx:395-398 on main). The PR
announces a hardcoded English template built from item.label
(Lightbox.tsx:423-424 at the PR head). Resolving this means either minting a
new locale key for custom items — a public key needing a translator
description and a place in every locale file — or deciding that a custom
item's label is passed as alt into the existing mediaPosition key. That is
a public-surface choice, not a merge.
2. The dialog's accessible name for a custom item.
main is aria-label={currentItem.alt || t('@astryx.lightbox.mediaViewer')}
(Lightbox.tsx:616). The PR is aria-label={currentLabel}
(Lightbox.tsx:564,580), and its own fallback for a media item without alt
is the literal string 'Media viewer' (Lightbox.tsx:566) — the English text
main replaced with the catalog key. Whether the translated fallback still
applies, and what a mixed gallery's dialog is called as the index moves between
kinds, is behaviour.
3. What the arrow keys do on a custom item.
main now overloads them: while zoom > 1 the arrows pan the image, otherwise
they move the gallery (Lightbox.tsx:519-532, KEYBOARD_PAN_OFFSETS at
:261-266). Custom items are deliberately never zoomable, so in a mixed gallery
the same key does two different things depending on which kind is showing — and
the PR's stated purpose is hosting interactive React content, which may itself
want the arrow keys. Nothing in either side settles who wins.
4. Whether the custom stage is in the tab order.
main makes the image wrapper a keyboard-operable zoom toggle: role="button",
tabIndex={0}, aria-pressed={isZoomed} when zoom applies
(Lightbox.tsx:662-665). The PR renders custom items in a separate wrapper with
none of that. Where focus lands when the gallery moves onto a custom item, and
whether that wrapper is focusable at all, is undecided.
5. What counts as "the backdrop" around custom content.
Both sides have a handleBackdropClick, but they are not the same handler.
The PR carries the pre-#3894 version, which closes only when the click lands on
the <dialog> element itself. main broadened it to the layout container as
well — the dark area around the media — and added a guard so the click ending a
pan drag does not dismiss (Lightbox.tsx:448-465). A custom item hosting an
interactive subtree changes what that region is, and the fifth conflict hunk is
the whole stage-and-caption block, which main re-nested to make the broadened
handler work.
The docs conflict (Lightbox.doc.mjs) follows from the same thing: both sides
rewrote the media prop's type and description from the same base, and
main's side carries a deliberate comment about why the prop is documented as a
named type rather than an expanded shape
(#1645). Reconciling it means
first knowing the answers above.
The author already anticipated this. The PR body says, of the overlap with #3894: "whichever merges first, I'll rebase the other." #3894 merged. So did #4367 and the i18n sweep.
The feature itself has not been superseded — nothing on main lets Lightbox
host arbitrary React content, and the duplicated gallery chrome the PR cites in
the docsite preview dialog is still there. This is a stale branch, not a stale
idea.
Unblocking it is the author's: merge main in, decide the five contracts above,
and the PR becomes reviewable at its new head. A review at the current head
would be a review of code that cannot land in that shape.
Nothing was posted to the PR and nothing was pushed to the author's branch. The loop is read-only on public PRs; a factual nudge to the author is a person's call, not this run's.