Skip to content

Review 4590

Jump to bottom
Cindy Zhang edited this page Aug 26, 2026 · 4 revisions

Review 4590 — feat(list): add isFullBleed

#4590 · jiunshinn · aged-PR cleanup Step 3, batch 3, 2026-08-23

Verdict: changes requested + ruling

PROBLEM

ListItem insets row content by a density-dependent 8/12px, so a list under a section heading reads as misaligned and consumers reach for negative-margin CSS (#2626).

SOLUTION

A List.isEdgeAligned boolean applying a matching negative marginInline to the <ul>, mirroring Item's own --spacing-2 / --spacing-3.

API

  • List.isEdgeAligned?: boolean = false (public, core barrel)

THEME TARGETS

no new theme targets

OSSIFICATION

Class exists — Divider has isFullBleed, Table has an edge adjustment, and #3938 wants one on TabList. But every existing member is container-aware (calc(-1 * var(--container-padding-inline-start, 0px)) plus a width compensation, Divider.tsx:116-129) and spelled isFullBleed. This member picks a different name and a different mechanism, deliberately and with a written argument.

BREAKING

API no (new opt-in prop, default false) · Visual no for existing consumers; for opt-in the <ul> grows 8px past its parent on each side (measured 884 vs 868) · Theme no new targets, but see judgement

EVIDENCE

Chromium, core-list--edge-aligned at 900px on the PR head. Default theme: heading text and row text both at x=16 — the feature works. Inject .astryx-item{padding-inline:var(--spacing-3)} (the value butter already writes on eight targets): row text moves to x=20, heading stays at 16. Frames 4590__A-default.png and 4590__B-theme-item-spacing3.png, both opened and looked at.

JUDGEMENT

request changes. [BLOCKS] the cancel margin mirrors a value Item owns and themes routinely change, so it is off by the difference under such a theme — the same off-by-4 the PR's own description flags in the detail-page template, one level up (R3d, same shape one step sideways). [ruling] naming and mechanism: Cindy's own comment on #2626 says isFullBleed is 'not quite accurate' and that this 'depends on container awareness'. The PR argues against the container-padding mechanism, well. That call is hers, not mine, and I said so on the PR rather than picking.

NOTES

Needs a Rulings entry: does the edge-adjustment family take a second mechanism (cancel the item's own inset) alongside the container-padding one, and under what name?

Posted

See the review on #4590. Attribution: [Reviewed by Robohands].

What Cindy changed before posting

(posted directly by the loop under the aged-PR brief; nothing edited)

Round 2 — exact-head re-review, 2026-08-26

HEAD REVIEWED

f0422bfac22eabd2f710bb650ed1e1274b73f14e

VERSIONS

  • LOOP VERSION: 1.4.0
  • AUDIT RUBRIC: 1.12
  • LANE: full — prior changes-requested, public API, intended visible layout

PROBLEM

A builder placing a List beneath a heading sees each row label begin 8/12px farther inward because Item owns an inline inset; custom negative-margin CSS is currently required to align them.

VERDICT: clear — #2626, the PR body, and the rendered 16px case demonstrate it.

SOLUTION

List opts each ListItem into content compensation. Item publishes its density/theme-controlled inline inset; the row subtracts the smaller of that inset and the surrounding container padding, leaving its own padding and hover/selection paint intact.

1 decision · ~15 runtime lines of 228 changed lines: add optional List full-bleed compensation, with the Item inset var, theme-derived mapping, row clamp, docs, story, tests, and changeset as its evidence.

The current implementation uses the inline-start container value for both row margins. It is correct for symmetric and 0px containers, but not for the documented asymmetric container contract.

VERDICT: BLOCKS — the end margin is not clamped to the end-edge padding.

ARCHITECTURE

  • OWNER: List/ListItem presentation, consuming the container-padding protocol
  • SEAMS: themed Item padding, standalone ListItem, symmetric/zero/asymmetric containers, LTR and RTL
  • BEHAVIOR UNIT: inline StyleX — one small deterministic layout rule; no hook or utility boundary is warranted

Placement is correct: the row is the only element that can read Item's own var, and ListContext owns the opt-in. Each margin must consume its matching container edge.

VERDICT: clear — owner and seam are settled; the value error is recorded below.

IMPACT

Existing callers are unchanged because isFullBleed defaults false. Opted-in callers with symmetric padding align correctly, and a 0px container now stays unchanged.

In a supported asymmetric 16px-start/4px-end container, the selected row over-cancels the end by 4px and paints 2px beyond the fixture's outer border. RTL mirrors the spill.

VERDICT: BLOCKS — a visible opt-in layout defect remains.

API

<List isFullBleed>
  <ListItem label="Solstice Mug" />
</List>

+ List.isFullBleed?: boolean = false — public through the existing @astryxdesign/core/List surface.

Ruling 3 settled the public name and meaning. Divider.isFullBleed is the landed sibling; List is the content-compensation member of that class.

VERDICT: clear — no API decision remains.

THEME TARGETS

No theme target is added or removed. Item adds private --_item-inset-inline, documents it, and maps item.paddingInline through the derived-var registry.

VERDICT: BLOCKS — ListItem.tsx:136-137 expands the start-edge value onto both logical margins instead of reading the matching end-edge variable.

BREAKING

  • API: no — optional prop, default false
  • Visual: intentional for opted-in symmetric containers; asymmetric end padding currently over-pulls
  • Theme: no target/token/override removed; one private derived var added, but the end-edge container override is ignored
  • Behavior: no existing caller changes; opt-in only

VERDICT: BLOCKS — the documented asymmetric container path is wrong.

PERFORMANCE

No effects, listeners, observers, layout reads, or dependencies. One boolean joins the existing memoized ListContext value; CSS min()/calc() owns the geometry.

VERDICT: clear.

VISUAL EVIDENCE

VISUAL CHECK: manual frames required — the PR intentionally moves row geometry, and exact-head visual CI had no baseline for the new story.

The four frames that carry the remaining block were captured with captureWithSensors(), opened, and published below: one decisive LTR exact-head/control pair and the matching RTL control pair. The prior 0px finding is closed and its boundary pair is intentionally omitted from this minimal published set.

Sensor Exact head Per-edge arm Pass?
Build f0422bfac22eabd2f710bb650ed1e1274b73f14e same SHA + per-edge-arm.diff yes
Story zzreview-list4590r3--asymmetric same yes
Theme / mode review-4590-evidence / light same yes
Direction LTR pair: LTR; RTL pair: RTL same per pair yes
Viewport / media 920×540@1; forced-colors off; reduced-motion off; fine pointer; hover same yes
Rendered state asymmetric-16-4:enabled=true:rows=1:selected=1 same yes
Subject geometry [data-capture] at x=16, y=16, 888×172.28125; visible and inside viewport same yes
Settled / errors fonts loaded; 0 animations; no Storybook or page error same yes
Image LTR aa58acea…; RTL a65f3365… LTR 9a34acf0…; RTL b1d9af4a… nonblank, 920×540

Decisive LTR pair — 16px start / 4px end

Exact head f0422bf Per-edge arm
Exact head: selected row spills past the logical-end border in LTR Per-edge arm: selected row stays within the logical-end border in LTR
−8px / −8px margins; label aligned; 2px logical-end spill −8px / −4px margins; label aligned; spill 0

RTL control pair

Exact head f0422bf Per-edge arm
Exact head: logical-end spill mirrored in RTL Per-edge arm: logical-end spill removed in RTL
same logical-end spill, mirrored physically same correction; spill 0

Receipts: exact-head LTR · arm LTR · exact-head RTL · arm RTL. Full compact receipt and arm provenance: assets/pr-4590/round-2/README.md.

The exact-head/control pixel change is confined to a 14×56 strip at logical end (252 pixels) in each direction.

VERDICT: BLOCKS — the 0px ask is fixed; the per-edge clamp is not.

EXPERIMENTAL ARM

The correction arm changed only the two logical margins; unrelated production delta: none.

 fullBleed: {
-  marginInline:
+  marginInlineStart:
     'calc(-1 * min(var(--_item-inset-inline), var(--container-padding-inline-start, 0px)))',
+  marginInlineEnd:
+    'calc(-1 * min(var(--_item-inset-inline), var(--container-padding-inline-end, 0px)))',
 },

The source worktree was restored to the exact head after capture.

A11Y & I18N

No role, name, keyboard, focus, live-region, or user/AT string changes. Exact-head pr-a11y and pr-rtl are green; the direction-sensitive layout was driven manually in RTL.

VERDICT: clear.

TESTS AND INTEGRATION

  • Focused List/theme run: 3 files, 185 tests passed
  • pnpm check:changesets: 29 valid
  • git diff --check: clean
  • Exact-head CI: lint, test, build, Storybook, a11y, visual, RTL, docsite, dependency, and scope jobs green
  • GitHub reports CONFLICTING / DIRTY: #5288 landed List BaseProps forwarding on main; the rebase must preserve its ...props and external aria-labelledby behavior, plus both registry-test additions

RECONCILED PRIOR FINDINGS

  1. 2026-08-23 mirrored Item inset — fixed and deleted. The var, derived mapping, tests, and visible Item/List parity all hold.
  2. 2026-08-26 rename to isFullBleed — fixed and deleted across API, docs, story, context, tests, and changeset.
  3. 2026-08-26 clamp to container padding; 0px unchanged — partly fixed. Symmetric and 0px pass; the shorthand applies the start-edge value to the end edge.
  4. Current main movement — #5288 creates a conflict whose resolution must preserve the newly landed List passthrough behavior.

JUDGEMENT

JUDGEMENT NEEDED: none — Ruling 3 already made the API/design decision; this is implementation correctness and integration.

  1. BLOCKS: marginInline applies the start-edge container padding to both edges (packages/core/src/List/ListItem.tsx:136-137). → In an opted-in 16px/4px container, the selected row paints beyond logical end in LTR and RTL. The isolated per-edge arm removes the spill while preserving alignment and 0px.
  2. BLOCKS: current main conflicts with the branch. → The author must rebase while preserving both this change and List's newly landed BaseProps/label forwarding.

GOAL: partly met — symmetric padding aligns and 0px stays unchanged; asymmetric end padding remains over-cancelled.

DISPOSITION: both findings block this head.

ADVICE: proven remedy — use separate logical margins with matching per-edge variables, then resolve the main conflict preserving both behaviors.

AUTHOR CAN PROCEED: yes — acceptance is explicit: each logical edge clamps against its matching container var; 16/4 has no spill in LTR/RTL; 0px remains unchanged; the rebase retains ...props and external aria-labelledby; focused tests and required CI are green on the rebased head.

WORST OUTCOME: “With isFullBleed in an asymmetric 16px/4px container, the selected row paints beyond the outer border on logical end in both LTR and RTL.” → request changes.

Final verdict: request changes.

THE REVIEW, DRAFTED — NOT POSTED

Thanks — f0422bf keeps the var-derived inset fix and closes my isFullBleed and 0px-clamp asks.

One clamp edge is still wrong. ListItem.tsx:136 applies --container-padding-inline-start to both margins. With 16px start/4px end padding, the selected row over-cancels the end by 4px and paints past the outer border; RTL mirrors the spill. Using the matching start/end vars removes it without changing label alignment or the 0px case.

Main also moved under this in #5288, and GitHub now reports a conflict. Could you fix the end-edge clamp, then rebase while preserving List's new ...props/external aria-labelledby forwarding and both registry-test additions?

If you'd rather talk it through with someone, we're in Discord.

[Reviewed by Robohands]

CRITIC

Pass 1 failed because JUDGEMENT lacked the numbered anchored consequence and receipt tables omitted exact subject geometry. The rewrite added both. Rewrite check: PASS — no remaining critic violations.

WHAT CHANGED BEFORE POSTING

Wiki evidence publication completed under assets/pr-4590/round-2; no GitHub PR review, comment, code push, merge, auto-merge, or workflow approval was performed.

Clone this wiki locally