-
Notifications
You must be signed in to change notification settings - Fork 0
Review 5538
Versions: Review Loop 1.1.0 · Component Audit Rubric 1.10
Verdict: approve
#5538 fix(TransferList): clear the three open audit BLOCKs (A10, T17, I1) by ernestt (bucket: internal — DESIGNOWNERS, repo owner)
0fdf93d01486da1dbaf1cbca7b4f6e77836c35ea
LOOP VERSION: 1.1.0 AUDIT RUBRIC: 1.10
Touch users had two header actions below the 24px minimum; theme authors could not replace the remove glyph because TransferList shadowed the semantic close icon; and users of mixed grouped/ungrouped lists saw an English fallback under other locales. The v1.8 ledger measured these as 35×20 / 47×20, one shadowed registry glyph plus two local glyphs, and a reachable Other literal.
VERDICT: clear — the three stated user/builder failures are concrete and reproduce on the parent commit.
The component makes the two text actions tall enough to hit, asks the shared icon system for the close symbol, and asks the translation system for the ungrouped heading. Add and reorder now go through Icon's sizing/color/a11y pipeline but keep local SVG component glyphs. The responsibility remains in TransferList and its existing core primitives.
SOLUTION (3 decisions · 57 runtime/catalog changed lines of 78 total changed lines)
- Give header actions the shared 28px small-element minimum. [A10 problem]
- Resolve remove by semantic
close; wrap local add/grip component icons inIcon. [T17 problem] - Add and consume
@astryx.transferList.ungroupedLabel. [I1 problem]
Three decisions is above the loop's smell threshold, but each is a named audit defect, independently verifiable, and small; splitting would add review latency without reducing design risk.
VERDICT: note — three independent fixes are bundled, but all are stated and independently settled.
OWNER: TransferList owns transfer/reorder behavior and the two-panel composition.
TIER 1: Button/IconButton, List/Item, useAnnounce, VisuallyHidden, useTranslator, themeProps, mergeProps.
TIER 2: sizeVars['--size-element-sm']; semantic close registry entry; add/grip remain component-mode icons.
SEAMS: built-in list, renderOption, TransferListSelector popover/top layer, nested Theme + InternationalizationProvider, root DOM passthrough.
BEHAVIOR UNIT: inline — this diff adds no interaction logic; the existing reorder state machine is pre-existing C22-era promotion debt, not changed here.
| seam | driven result |
|---|---|
| built-in surface | add, reorder, Escape-cancel, focus recovery and announcements pass |
custom renderOption
|
custom <strong> content survives; built-in actions still move items |
TransferListSelector popover |
390×568 coarse-pointer run opens, fits, and does not document-scroll |
| consumer root props | class, inline style, data attribute and bubbling onClick all survive |
| theme + locale providers | close glyph swaps from the active theme; ungrouped heading resolves the French override |
The implementation lives with the owner and survives every documented seam exercised. The close glyph now uses the semantic registry correctly. Add and grip still bypass semantic registry keys, so their glyph shape is not theme-replaceable; current T17 classifies that as FIX, not BLOCK, because neither shadows an existing name.
VERDICT: note — all architecture paths pass; add/reorder retain FIX-level registry debt.
Touch users gain real 28px targets instead of 20px targets. A theme can replace the remove glyph, and locale overrides can replace the ungrouped heading. The header row grows 8px; the whole component grows 8px side-by-side and 16px when stacked. No clipping or horizontal overflow appeared at 326–360px, in the 390×568 selector popover, or at 200% root text size.
This is lab/canary-only, so no stable installed consumer is broken.
VERDICT: note — the intended hit-area fix has a small real layout delta, contrary to the body’s “visually invisible” wording, but no bad outcome was found.
No public API change. The only new function signatures are private SVG component props, and the existing TransferList root/renderOption contracts are unchanged. No new surface ossifies.
VERDICT: clear — no API or theme-target addition/removal/change.
No target, variant, component variable, or token value changes. The target-size floor uses the shared size token. In Chromium, icon color followed neutral light (rgb(23,23,23)), dark (rgb(250,250,250)), and forced-colors (rgb(0,0,0)); a theme override replaced the remove glyph. Add/grip receive Icon color/size classes but remain fixed local glyphs.
VERDICT: note — no regression; semantic replaceability is complete for remove and incomplete for add/reorder.
- API: no — signatures, defaults, exports, and call sites are unchanged.
- Visual: yes, intentional — each panel header grows 37→45px; root grows 273→281px wide and 417→433px stacked. This traces to the stated target-size fix, but is not literally invisible.
- Theme: no — existing targets and overrides still resolve;
closebecomes more themeable. - Behaviour: no — add/remove/bulk/reorder/search, disabled, empty, and boundary paths are unchanged; browser probes preserved transfer focus, keyboard reorder, Escape rollback, and announcements.
VERDICT: note — deliberate geometry growth only; no breaking or regression path found.
Effects: +0 / ~0 / −0. The existing drag-preview useLayoutEffect (DOM clone) and useEffect (native top-layer sync) are untouched. No listener, observer, layout-read, dependency, render-loop, or bundle addition. The new translator call and icon registry lookup are constant work already used throughout the component.
VERDICT: clear — no measured or structural performance/resource regression.
All listed frames were captured from the same Storybook dev server, parent files restored in-place for BEFORE and reviewed head restored for AFTER, then opened with the image reader.
| case | before | after | result |
|---|---|---|---|
| wide light | shots/before/fine-wide.png |
shots/after2/fine-wide.png |
target 20→28; header/root +8px; no overlap |
| stacked narrow | shots/before/fine-narrow.png |
shots/after2/fine-narrow.png |
both headers +8px; root +16px; no overflow |
| coarse selector 390×568 | shots/before/selector-390x568-coarse.png |
shots/after2/selector-390x568-coarse.png |
popover 353→369px; fits viewport; target 28px after |
| dark | shots/before/dark.png |
shots/after2/dark.png |
same geometry; icons remain visible and inherit dark color |
| translated custom seam | shots/before/custom-architecture.png |
shots/after2/custom-architecture.png |
Other→Autres options sans groupe; custom close glyph appears |
| RTL + keyboard | — | shots/after2/rtl-keyboard.png |
panel order mirrors; neutral glyphs do not; reorder/cancel passes |
| forced colors | — | shots/after2/forced-colors.png |
focus outline and every action glyph remain visible |
| translated 200% text | shots/before/translated-zoom-200.png |
shots/after2/translated-zoom-200.png |
no horizontal overflow; long heading remains readable |
The only pixel/layout difference beyond glyph shape and translated text is the intentional transparent target growth. The author's linked image returned 404 to an unauthenticated fetch, so it was not counted as evidence.
VERDICT: note — intended geometry is real, bounded, and visually sound; no unintended difference found.
Auto-covered: pr-a11y, pr-rtl, lint, test, build, check:i18n-catalog, check:changesets, and check:sync are green on the reviewed head; no baseline grew. Checked in Chromium: header targets are 33.875×28 and 43.922×28 in both fine and coarse contexts; icon descendants are decorative under named buttons; forced-colors keeps glyphs/focus visible; add focus advances to the next available action; reorder announces position, Escape restores order and focus; RTL mirrors the panels without flipping neutral glyphs; a French override renders the new ungrouped heading. Not looked at: native screen-reader speech output (the live-region DOM updates were observed).
A10's blocking <24px failure is cleared. The coarse-pointer target remains 28px rather than the rubric's ~44px preference, a FIX-level residue. T17's blocking shadow of close is cleared; local add/grip remain FIX-level. I1 is fully cleared.
VERDICT: note — zero BLOCKs; two explicit FIX-level promotion residues remain.
| slot | verdict |
|---|---|
| PROBLEM | clear |
| SOLUTION | note — three independent audit fixes |
| ARCHITECTURE | note — add/reorder not semantic-registry replaceable |
| IMPACT | note — bounded 8/16px intentional growth |
| API | clear |
| THEMING | note — close fully themeable, add/reorder shape fixed |
| BREAKING | note — intentional geometry only |
| PERFORMANCE | clear |
| VISUAL | note — claimed invisibility is not literal, no regression |
| A11Y & I18N | note — zero BLOCKs, two FIX residues |
GOAL: met — Chromium changed target height 20→28, custom close began resolving, and Other began resolving to the supplied French override; the same probe confirms all three regress on the parent.
DISPOSITION: coarse target stays 28px → accepted for this PR because A10 marks missing coarse expansion FIX and every target clears the 24px minimum; local add/grip SVG components remain → accepted for this PR because T17 marks non-shadowing local glyphs FIX and the change removes the sole BLOCK; 8/16px layout growth → accepted as the stated target-size effect, with no clipping/overflow in the driven containers.
ADVICE: proven direction — before promotion, add coarse-pointer expansion and semantic add/reorder registry entries; the current rubric and the landed RichText extension-key path establish the mechanisms.
AUTHOR CAN PROCEED: yes — all named blockers are gone and the remaining notes do not make this PR unsafe.
WORST OUTCOME: none found → approve.
JUDGEMENT NEEDED: none — defect fixes against existing contracts.
approve
Thanks, all three blockers are clear.
[Reviewed by Robohands]
-
packages/lab/src/TransferList/TransferList.tsx:266— This remains 28px on coarse pointers; might add the ~44px expansion before promotion. -
packages/lab/src/TransferList/TransferList.tsx:61— These still bypass semantic registry keys, so themes can’t replace add/reorder.
- The added icon assertions fail on the parent, but they prove Icon-pipeline rendering rather than semantic replacement; the custom-theme browser probe establishes the latter for
close. - 20 current checks: 19 success, one expected skipped anchor; targeted 38/38 tests pass.
- Current main advanced in the locale file but did not touch any
transferListkey or TransferList source, so it does not invalidate this head.