Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
|
AI review status for this pull request.
|
PR Analysis Report📚 Storybook Preview🧪 Sandbox PreviewModified ComponentsLightbox (@astryxdesign/core) · View in Storybook
Component Size
Sizes are gzip. Import counts the JavaScript the import loads eagerly; CSS counts the package stylesheet rules referenced by the modules it can load from its own package, lazily loaded ones included.
|
| Component | JavaScript | CSS |
|---|---|---|
| core/Lightbox | +41.6KB |
+260B |
Bundle Size Summary
Gzip figures measure each package's entry file, not the code an import pulls in.
| Package | Entry (ESM) | Entry gzip before | Entry gzip after | Delta |
|---|---|---|---|---|
| @astryxdesign/core | 4.9KB | 1.2KB | 1.2KB | 0B |
| Package stylesheet | Gzip before | Gzip after | Delta |
|---|---|---|---|
| @astryxdesign/core | 34.9KB | 35.0KB | +84B |
Accessibility Audit
Status: No accessibility violations detected.
Visual Regression
57 of 504 shot(s) changed.
A change here is a question, not a failure: check whether the after is the
picture you intended. Record that review on the PR. Baseline maintenance is an
explicit dispatch of CI; this report never rewrites the baseline or adds a release gate.
| component | story | theme | mode | pixels |
|---|---|---|---|---|
| Selector | Indicator Space Bottom Sheet Narrow End RTL | neutral | light | 753,933 |
| Selector | Indicator Space Bottom Sheet Wide Start | neutral | light | 740,072 |
| RichTextEditor | Markdown parity: side by side | probe | dark | 558,840 |
| RichTextEditor | Markdown parity: side by side | probe | light | 540,756 |
| Chat | Full AI Chat | probe | light | 230,823 |
| Chat | Full AI Chat | probe | dark | 229,805 |
| AppShell | With Mobile Nav | probe | light | 217,010 |
| AppShell | With Mobile Nav | probe | dark | 216,960 |
| Collapsible | Dividers | probe | dark | 66,445 |
| Collapsible | Dividers | probe | light | 66,445 |
| AppShell | Playground | neutral | dark | 65,198 |
| AppShell | Playground | neutral | light | 63,788 |
| Chat | Default | neutral | dark | 36,304 |
| Chat | Default | neutral | light | 35,767 |
| Chat | Empty State | probe | light | 23,531 |
| Chat | Empty State | probe | dark | 22,622 |
| ChatAutoScroll | Scroll Behavior Comparison | neutral | dark | 15,378 |
| ChatAutoScroll | Scroll Behavior Comparison | neutral | light | 14,107 |
| Chat | With Attachments | probe | light | 13,406 |
| Chat | With Attachments | probe | dark | 11,232 |
and 37 more.
Generated by PR Enrichment workflow | Storybook | Sandbox | View full report
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Approval waits on an engineering-owner decision: swipe paging for Lightbox galleries rests on the draft spec:AST-094 (FR1–FR23, DEC-1, DEC-2) and on a draft component:Lightbox record that does not cover paging, so an engineering owner needs to approve or reject that direction, including its proposed amendments to architecture:theme-tokens, family:buttons, architecture:interaction-modality and spec:AST-025, by commenting /approve-spec <full head SHA> on this pull request (the author may do this as an engineering owner). Whatever that decision, these fixes are needed:
- Make gallery pages that are not current
inertandaria-hidden, since Tab can currently reach a link in the next item's caption, scrolling that item into view under the previous item's counter, and screen readers read the neighbours' captions. - Re-observe the zoom stage when the current page changes, because two items away from the opening one a rotation no longer re-fits a zoomed photo.
- Stop
usePagerfrom reporting controlled index changes from an Effect (architecture:react-component-runtimeINV2 and INV3) and return the track when a controlledindexdoes not follow a swipe. - Land the stacked pull requests first so this one carries only its own change instead of about 31,000 lines.
[Automated review]
| <div | ||
| key={i} | ||
| data-lightbox-page="" | ||
| data-current={i === index ? '' : undefined} |
There was a problem hiding this comment.
Pages that are not current stay in the tab order and the accessibility tree; only their <img> is hidden. With a four-item gallery open at 2 / 4, Tab from Previous reached a link in item 3's caption and the stage (overflow: hidden) scrolled 442 px, so item 3 showed under a 2 / 4 counter. Consider inert and aria-hidden="true" on every page without data-current.
There was a problem hiding this comment.
Right, off-screen pages stayed focusable and in the accessibility tree. Fixed in 2834675: every page but the current one is inert and aria-hidden. LightboxPaging checks the attributes and tabs through the dialog without ever entering an off-screen page.
| }); | ||
| observer.observe(stage); | ||
| return () => observer.disconnect(); | ||
| // The stage element is stable for the dialog's lifetime. |
There was a problem hiding this comment.
In a gallery stageRef is pageMediaRef, which moves to a new element on every page, so this no longer holds: the observer stays on the opening page's box, and once that page unmounts (two items away) a rotation no longer re-fits a zoomed photo. Re-observe when the stage element changes, or observe the track, whose size every page shares.
There was a problem hiding this comment.
Right, the observer stayed on the opening page's box. Fixed in 2834675: the stage watches the track, whose size every page shares, and re-attaches whenever it turns on. LightboxPaging rotates two pages on, and the zoomed photo keeps its centre point.
|
|
||
| useEffect(() => { | ||
| if (index != null && index !== pager.index) { | ||
| pager.goTo(index, {reason: 'controlled'}); |
There was a problem hiding this comment.
This Effect reports onIndexChange(index, 'controlled') through goTo and names no external system, which architecture:react-component-runtime INV2 and INV3 ask for. It also never returns the track when a controlled index refuses a swipe, because it runs only when index changes.
411109c to
d593122
Compare
There was a problem hiding this comment.
Approval waits on an engineering owner's decision on the draft touch-presentations spec (AST-094) that this pull request introduces as the authority for Lightbox swipe paging, including whether buttons and arrow keys keep sliding under Reduce Motion where its FR8 asks for a cross-fade. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner. Whatever the decision, these need fixing:
- Pages beside the current photo stay focusable and exposed: Tab reaches photo 3's caption link and scrolls it into view under “2 / 4”.
- A controlled parent that refuses a swipe is left with the swiped photo on screen.
- Only the photo the dialog first rendered re-fits when a zoomed photo is rotated.
usePagerreports controlled index changes from an Effect.- The diff carries #8045, #8146 and #8148 (31,552 authored lines); rebase or retarget it so it holds only the paging step.
[Automated review]
| }, | ||
| shouldBeginPan: direction => !stage.isZoomed() || stage.isAtEdge(direction), | ||
| onIndexChange: (next, reason) => { | ||
| if (reason === 'drag') { |
There was a problem hiding this comment.
When index is controlled and the parent refuses the change, this drag report goes through setIndex, which only calls onIndexChange, and usePager re-syncs only when index changes, so the track stays on the swiped photo: after a swipe the viewer shows photo 3 under “2 / 4”. Return the pager to the controlled index when the parent keeps it, and add a test with a refusing parent.
There was a problem hiding this comment.
Right, the track stayed on the swiped photo under the old counter. Fixed in 2834675: a swipe's report renders the viewer once more, and when the parent kept its index the pager goes back to it. The new GalleryKeepsIndex story and LightboxPaging cover it (it stayed on the swiped photo before).
d593122 to
b0ebc5a
Compare
There was a problem hiding this comment.
Approval is waiting on an engineering owner's decision about Lightbox swipe paging and the draft spec:AST-094 that would govern it: an engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request (the author may do so if they are an engineering owner), or paging waits for a current Lightbox spec. Whatever that decision, these need changes:
- Make the off-screen gallery pages
inert, so screen readers and Tab no longer reach captions and controls that are not on screen. - Move the pager back to
indexwhen a controlled parent refuses a swipe, and stop callingonIndexChangefrom the controlled-sync Effect. - Report Lightbox's base-against-head gzip size (the import grows by about 31 KB gzip) and load the touch-only code lazily or justify it.
- Re-observe the zoom stage when the current page changes, regenerate the CLI docs golden that fails
test-node (1/4), and rebase onto the stacked pull requests so this one carries only the paging commit.
[Automated review]
|
Posted by Cindy's agent on her behalf. Parts of this PR re-implement behavior that already exists, or is being specified, as a shared part of core. Converging keeps one implementation per behavior:
This is about where the behavior lives, not about the PR's intent. |
|
Agreed: one implementation.
|
b0ebc5a to
2834675
Compare
github-actions
Bot
added
the
needs:design-review
There was a problem hiding this comment.
Approval is waiting on an engineering owner's decision about Lightbox swipe paging and the draft touch-presentations spec (AST-094) proposed to govern it: an engineering owner either approves that spec and the paging direction by commenting /approve-spec <full head SHA> on this pull request (the author may do so as an engineering owner), or paging waits for a current Lightbox spec. That decision has to settle AST-094's requirements, including FR4's rule that touch motion is not themable while this branch adds theme motion roles the pager reads, and how callers keep the fine-pointer behaviour on a touch device. Whatever the decision, these need changes:
- Pause a gallery video when the viewer moves away from it; today a video the person started keeps playing, unseen and unreachable, on the neighbouring page.
- Split the UIKit gesture table so Lightbox ships only the rows it reads, and report base and head sizes with the CSS delta, loading the touch-only code lazily or justifying its cost.
- Remove the
platformadaptation axis from the theme template, sincedefineThemerejects it. - Rebase onto the stacked pull requests so this one carries only the paging commit, and split out the
astryxStylexundefinedVariablesoption.
[Automated review]
| autoPlay={isCurrent && hasAutoPlay} | ||
| preload={isCurrent ? 'auto' : 'metadata'} |
There was a problem hiding this comment.
When this page stops being current only autoPlay and preload change, and neither pauses a playing video. The page stays mounted under key i as the previous neighbour, inert, so a video the person started keeps playing with its sound until they move two items away and the page unmounts. Consider pausing every non-current page's video when the index changes, with a paging test that plays a video.
| * are primary-pointer precision, contrast preference, motion preference, and | ||
| * platform (`ios` for the browsers of iPhone and iPad, `other` for the rest). |
There was a problem hiding this comment.
defineTheme no longer accepts a platform condition: themeAdaptations.ts admits width, pointer, contrast and motion, and any other key throws is not supported. A theme author following this sentence gets a build error; consider restoring the base wording without platform.
| * UIKit's gesture thresholds and velocity filter, each with its source. | ||
| * @experimental | ||
| */ | ||
| export const UIKIT_GESTURES = { |
There was a problem hiding this comment.
Lightbox imports recognizers that read this one object, so the swipe, rotation and screenEdgePan rows and every source string ship with every Lightbox import, which component-style-authoring INV6 rules out for tuning tables. Consider one export per recognizer's rows, with the provenance strings kept out of the runtime values.
2834675 to
739d31d
Compare
…interaction harness's busy-window control) into meta#8148
…base of meta#8147 A busy-window proof can judge a component against a compositor-only control animation recorded in the same window (startBusyControl, CONTROL_RGB, busyWindowProof's control option), instead of a frame-rate bound that a two-core runner's software compositor cannot meet.
…positor-only control On a two-core runner the software compositor and the screencast share the core the blocked main thread spins on, so every compositor animation presents fewer frames there: the proof's frame-rate bound failed 7 runs in 10 pinned to two cores (the pr-interaction red on meta#8215). It now records the interaction harness's control animation in the same window and passes when the photo keeps pace with it (busyWindowProof's verdict); a main-thread animation still fails.
…ainst a compositor-only control The proof tracked the gallery track, which is wider than the screen, so the tracked box stayed pinned to the screen's edge and read as settled 7 ms into the block, though the settle ran another 275 ms: its frame-rate checks passed on a window too short to judge. It now tracks the next photo as it slides in, and judges it against the interaction harness's control animation recorded in the same window, which also fails a window too short to judge. Red: on the old marker the control verdict reads "too little to judge"; green in both engines.
…ver pooled runs A fade that lasts a fraction of a second leaves one blocked window only a few frames: in WebKit, or on a loaded machine, the control can present fewer than the four one window needs, and the proof fails as too little to judge, saying nothing about the animation. pooledBusyProof(runs) judges several runs of the same short animation as one long window is judged against the control: the control must move on at least four frames over all the runs, the component on 90% of them (one miss allowed), and in each run its longest stall may be at most one interval longer than that run's control's, since a stall belongs to its run. Red first: a 30 ms judged window holds at most three frames and one run fails as too little to judge; pooled over six runs, a compositor animation passes and a main-thread one fails, in Chromium and WebKit.
github-actions
Bot
added
the
needs:spec-owner-review
There was a problem hiding this comment.
Approval is waiting on an engineering owner to provide or identify the governing spec for Lightbox swipe paging and touch zoom, and to decide the new architecture:layer-runtime INV13 this branch adds: the Lightbox record is still a draft, the gestures here attach only under a touch pointer while spec:AST-094 FR19 and FR43 now attach gestures for every pointer, and spec:AST-017 FR59 asks for a separate decision on the new fine-pointer gallery layout. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request (the author may do so as an engineering owner); it needs to settle whether the gestures stay touch-and-pen-only, whether the every-pointer caption layout stays, which paging motion roles the Lightbox record names, and whether INV13 belongs here. Whatever the decision, please address these:
- With a mouse, keep controlled
indexchanges instant: today an app-driven change slides over 300 ms, and a Next the parent refuses shows the next photo, then slides back. - Drop the hidden copies of other items' captions that reserve the caption row (
architecture:component-style-authoringINV7), and give element counts for the added pages. - Report Lightbox's size against the base (CI measures +40,086 bytes gzip on the import) and justify the touch-only cost.
- Get
main's touch-module bundle check passing, and rebase onto the landed stack so this pull request carries only the paging change, with the lift module, exit copy and INV13 in their own pull request.
[Automated review]
| // is not reported back. Idempotent: it moves only while the two differ. | ||
| useEffect(() => { | ||
| if (index != null && index !== pager.index) { | ||
| pager.goTo(index, {reason: 'controlled'}); |
There was a problem hiding this comment.
With a mouse this controlled sync still animates: goTo defaults to animated: true, so an app-driven index change while the viewer is open slides over 300 ms, and a Next the parent refuses slides back after Lightbox has jumped the pager forward. spec:AST-017 FR59 keeps fine-pointer behavior as it was; consider moving without animation unless the touch presentation applies.
| if (!hasZoom || isVideo || next === zoom) { | ||
| return; | ||
| if (!isTouch) { | ||
| pager.goTo(next, {animated: false}); |
There was a problem hiding this comment.
This pre-move jumps the pager before the owner answers, so a controlled parent that keeps its index sees the next photo at once and then a 300 ms slide back from the controlled sync. Moving the pager only once the owner takes the change keeps a refused Next invisible, as on main.
| <div | ||
| aria-hidden | ||
| inert | ||
| {...stylex.props( | ||
| styles.captionStacked, | ||
| styles.captionSizer, | ||
| )}> | ||
| {unrenderedCaptions.map(j => ( | ||
| <div | ||
| key={j} | ||
| {...stylex.props( | ||
| styles.caption, | ||
| styles.captionStacked, | ||
| )}> | ||
| {mediaArray[j].caption} | ||
| </div> | ||
| ))} | ||
| </div> |
There was a problem hiding this comment.
These hidden copies of every caption no rendered page shows reserve the caption row, which architecture:component-style-authoring INV7 rules out ("Content renders once: no hidden twin to measure or reserve space"). A gallery of 50 captioned items mounts 47 of them, and any component in a caption runs once per item.
…ery pointer, wheel and trackpad scrolling; the harness's pooled busy proof) into meta#8147
…every pointer, wheel and trackpad scrolling; the harness's pooled busy proof) into meta#8148
…ckpad (spec:AST-094 FR43) The zoom stage's pan, double tap, tap and catch attach for every pointer; the pinch stays a finger's, and a ctrl-wheel reaches it through the gesture core (meta#8676). A double-click is the stage's double tap (the zoom core's own mouse path goes), a wheel pans a zoomed photo, a zoomed drag rubber-bands at the edges and a released one coasts. The stage fills the viewer for every pointer. A release follows the pointer that began it: under reduced motion a finger's cross-fades (DR2) and a mouse's, a trackpad's or a pen's completes at once on a fine primary pointer (DR3). Tests: LightboxEveryPointer's zoom proofs (a ctrl-wheel zooms, a wheel pans a zoomed photo, a released drag coasts); the fine-pointer and zoom mode proofs that asserted the old mouse zoom flip to FR43's; the unit tests compare a mouse's double-click with a finger's double tap. Docs in English and Chinese; a patch changeset, and the touch zoom changeset no longer says a mouse keeps the old zoom. Red: under a mouse the double-click zoomed 2x about the centre, a ctrl-wheel did not zoom, a wheel did not pan a zoomed photo, a released drag did not coast and the stage did not fill the viewer; green after.
… of meta#8197 meta#8148 now carries the gesture core of meta#8676 and meta#8665, which attaches recognizers for every pointer and reads the selection lock from the pointer alone. Two adaptations follow: usePager calls locksSelectionFor with the pointer only, and the bare pager's fine-pointer proof expects a mouse drag to page (spec:AST-094 FR43).
… pointer) into meta#8197 The zoom stage keeps this PR's pan and catch conditions on meta#8148's pointer lists, and the zoom core its surface and edge checks while its mouse path goes. The stage's options and layout take meta#8148's ungated forms; the touch presentation stays for the gallery's buttons and arrow keys. The zoom target's fine-pointer proof lists the stage's claimed touches.
…croll (spec:AST-094 FR43) usePager attaches its pan and catch for every pointer instead of only under a coarse primary pointer, and its pan takes a trackpad's or a wheel's horizontal scroll through the gesture core. Lightbox's gallery lets the mouse page too. A click beside a gallery photo with zoom on is the stage's tap for every pointer, after the double-tap interval. Tests: LightboxEveryPointer's paging proofs (a mouse drag sideways and a trackpad's horizontal scroll page a gallery); the paging mode and fine-pointer proofs and usePager's React test that asserted no input under a fine pointer flip to FR43's. Docs in English and Chinese; the every-pointer changeset gains paging, and the paging changeset no longer says a mouse pages only with the buttons. Red: under a mouse a sideways drag and a trackpad's horizontal scroll did not page, the pager attached nothing on a fine pointer and the gallery claimed no touches; green after.
…er (spec:AST-094 FR43) The zoom stage's header, its enabled flag and the drag geometry still spoke of fingers and pens only, of a mouse whose limits do not give, and of a double-click on the released zoom; the hasZoom JSDoc still limited the pinch and the double tap to a coarse pointer; the stage's styles still said the touch layer ran on a touch screen only (FR19); and the fine-pointer proofs' header still claimed touch work leaves that zoom alone. They now say what the code does: every pointer reaches the recognizers (the pinch is the fingers' and a ctrl-wheel's), the released zoom is the keyboard's, and under Reduce Motion a finger's release cross-fades while the others complete at once on a fine pointer. Comments only.
…ter) into the base of meta#8197
…very pointer (spec:AST-094 FR43) The track's style still said the stage's touches went to it under the touch layer only, the zoom stage's catch still spoke of a touch, and the gallery's navigation comment tied the pager's slide to a finger, where it follows the touch presentation. They now say what the code does. Comments only.
meta#8197 brings the zoom's and the pager's gestures for every pointer. The drag to dismiss keeps its finger and pen pointer list (the zoom's shared list is now every pointer's) and its touch-presentation gating until its own change; its imports drop meta#8197's coarse-pointer read, and the fine-pointer proofs keep meta#8197's gallery proof beside this PR's.
There was a problem hiding this comment.
Approval is waiting on an engineering owner to provide or identify the governing spec for Lightbox paging, its zoom gestures and the new gallery layout, and to decide the architecture:layer-runtime INV13 this branch adds: the Lightbox record is still a draft, spec:AST-094 leaves each component's touch presentation to its own record, and spec:AST-017 FR59 asks for a separate decision on the new fine-pointer layout. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request (the author may do so as an engineering owner). Whatever the decision, please address these:
- With a mouse, keep controlled
indexchanges instant: a Next the parent refuses shows the next photo and slides back, and an app-driven move slides. - Drop the hidden copies of other items' captions that reserve the caption row (
architecture:component-style-authoringINV7), and give element counts for the added pages. - Get
scripts/touch-support-bundle.test.mjspassing again: Core's root entry now bundles the gesture modules through Lightbox. - Show fine-pointer before-and-after for the gallery layout, and rebase onto the landed stack so this pull request carries only the paging change, with the lift module, exit copy and INV13 in their own pull request.
[Automated review]
| // is not reported back. Idempotent: it moves only while the two differ. | ||
| useEffect(() => { | ||
| if (index != null && index !== pager.index) { | ||
| pager.goTo(index, {reason: 'controlled'}); |
There was a problem hiding this comment.
With a mouse this still slides: goTo defaults to animated: true (:1023), so a Next the parent refuses shows the next photo and slides back over 300 ms, and an app-driven index change slides where main swaps the photo. spec:AST-017 FR59 keeps fine-pointer behavior as it was; make a controlled move instant under a fine pointer and add a browser test for both cases.
| {...stylex.props(styles.pageMedia)}> | ||
| {renderMedia(mediaArray[i], i === index)} | ||
| </div> | ||
| {i === index && unrenderedCaptions.length > 0 ? ( |
There was a problem hiding this comment.
These hidden copies of every caption no rendered page shows exist only to reserve the caption row. architecture:component-style-authoring INV7 says "Content renders once: no hidden twin to measure or reserve space"; size the row with CSS on the captions that are shown and drop the sizer.
Depends on #8148 and #8146, and on #8665 and #8676 (gestures for every pointer), which this stack carries. Review only this PR's change: vjeux/astryx@1180c0f...2a1354c
Why
On a phone, a Lightbox gallery moves only through two small arrow buttons. People expect to swipe between photos as in Photos, with a zoomed photo panning to its edge before the next one comes.
What led to it
The UIKit-parity work for Astryx (
spec:AST-094): after the zoom (#8148), paging, on #8146's shared pager.Options and tradeoffs
Why this choice
Only the pager keeps a pinch from paging and lets a zoomed photo keep the drag until its edge, as Photos does. Pages sit 40 pt apart, Photos' measured gap.
How to check it
In the "Gallery with zoom" story, on a phone:
Evidence
Safari on the iOS 26.2 simulator, a swipe left:
Android 15 emulator, Chromium:
The page settle's busy proof used to judge a 7 ms window, too short to judge anything: it tracked the gallery track, which is wider than the screen, so its edge stayed pinned to the screen's. It now tracks the next photo as it slides in, against a compositor-only control animation, and passes in both engines.