Review 5483

Review #5483 — Banner theme targets

Versions: Review Loop 1.0.0 · Component Audit Rubric 1.10
Head: 29eabdb7 · Verdict: request changes

PR

#5483 feat(Banner): theme targets for the description and actions row by freddymeta (bucket: collaborator/internal)

HEAD REVIEWED

29eabdb70474c8db367117a9a28fdb0a293924be

LOOP VERSION: 1.0.0 AUDIT RUBRIC: 1.10

PROBLEM

A real adopter needs Banner's supporting text to use its text treatment and wants the 44px status header to grow to 52px around its controls. banner-description is currently unreachable without a child-order selector. The height outcome is already reachable by changing padding-block on the existing banner target.

VERDICT: BLOCKS — one of the two proposed targets serves an outcome the existing header target already provides

SOLUTION

The PR adds stable targets to the description painter and the end-area layout row, then changes the title-only centering branch to count a collapse toggle as an action.

SOLUTION (3 decisions · ~16 runtime lines of 700)

Add banner-description to the element painting supporting typography and color.
Add banner-actions to the row owning flex, wrap, gap, margins, and edge compensation.
Count the collapse toggle in isSingleLine centering.

Decisions 2–3 are coupled: with shipped styles, decision 3 changes align-items but zero pixels; it changes pixels only after the row's private margin is overridden.

VERDICT: BLOCKS — the actions-row target and centering branch jointly expose and support private layout machinery

ARCHITECTURE

OWNER: Banner owns its stable painted anatomy; Button owns both controls' interaction and paint; Icon owns each glyph; consumer endContent keeps its own contracts. TIER 1: themeProps is composed on the description and action row; root BaseProps, ref, class/style, and capture handlers continue through mergeProps and composeEventHandlers. TIER 2: Banner keeps info | success | warning | error, muted status surfaces, and the accent/success/warning/error icon family. SEAMS: default/custom icon, arbitrary endContent, collapse toggle, dismiss Button, content mount/unmount, root DOM passthrough. BEHAVIOR UNIT: inline is correct; the only new behavior is one derived boolean term and useCollapsible retains the state machine.

Fresh Chromium drove root DOM/ref/handler composition: neutral props, consumer class/style and ref reached the root; consumer capture handlers fired; keyboard dismissal restored focus. Both controls remained ordinary Buttons and their glyphs ordinary Icons.

Theming Infrastructure says: “A target must sit on the element that actually paints the thing you're theming — never a wrapper that exists only for layout, positioning, or event handling.” The new action row has transparent base paint and owns only flex/wrap/gap/max-width/margins/edge compensation. Audit T7 also keeps margins, positioning, display, clipping, stacking and sibling relationships component/layout-owned.

VERDICT: BLOCKS — banner-actions is a target on non-painting layout machinery

IMPACT

Default users see no visual or interaction change. Themes opting into banner-description gain a stable painter for text, type and internal spacing; themes opting into banner-actions would gain control over Banner's private row geometry and freeze that wrapper as public API.

Fresh Chromium showed the existing banner { padding-block: 16px } seam changes the title-only toggle case from 44px to 52px while keeping title/control centers equal, without the action-row target or the centering change.

VERDICT: BLOCKS — the permanent row contract is unnecessary for the stated 52px result

API

defineTheme({
  name: 'brand',
  components: {
    'banner-description': {base: {color: 'var(--color-text-primary)', paddingBlockStart: '2px'}},
    banner: {base: {paddingBlock: '16px'}},
  },
});

change	public?	class / precedent	verdict
`+ banner-description` target	yes — permanent target	painted supporting text; `empty-state-description` is landed precedent	`[Needs human judgement]` — recommend accept
`+ banner-actions` target	yes — permanent target	no landed `*-actions` target; layout-only row	BLOCKS — existing `banner` padding serves the stated need

Both names follow <component-kebab>-<part>. Removing either later silently breaks themes. The class for description is real and demonstrated; the action-row class is not, because its use is changing component-owned margins and layout.

VERDICT: BLOCKS — banner-actions does not earn permanence; banner-description still needs the want-it decision

THEMING

banner-description sits on a visible element with base typography/color and gives its internal padding to the theme; it is a sound target. banner-actions sits on a transparent layout wrapper and exposes margin, display, wrapping, alignment, gap, and edge compensation—the exact properties the current anatomy keeps private.

No token, existing target, status value, or semantic family changes. Status remains info | success | warning | error; header fills remain --color-{accent|warning|error|success}-muted, and icons remain the matching semantic colors.

VERDICT: BLOCKS — the description target is sound; the actions target violates the painting-element boundary

BREAKING

API: additive public targets; no existing call site stops compiling.
Visual: current main and head are byte-identical in light, dark, RTL, and forced colors; the centering commit is also byte-identical to its parent under shipped styles.
Theme: no existing target/token/override changes; banner-actions would newly freeze an internal wrapper.
Behaviour: no default rendered behavior changes. With only banner-actions {margin-block: 0}, the centering commit changes title/control center delta from -4px to 0px.

VERDICT: clear

PERFORMANCE & RESOURCES

Effects: 0. Listeners: 0. Observers/timers: 0. Async work: 0. O(N) work: 0. Two constant themeProps calls are added. No measurement was warranted.

VERDICT: clear

VISUAL EVIDENCE

Chromium 149, current head, 760px viewport. Every listed frame was opened.

case	light	dark	RTL	forced colors	result
current main → head, All Features	`main-head/*default.png`	same	same	same	byte-identical in all four
pre-centering simulation → current, shipped styles	`focused/{parent,current}-default__light.png`	same	same	same	byte-identical in all four; 44px, center delta 0px
pre-centering simulation → current, action margin 0	`focused/{parent,current}-actions-margin0__light.png`	same	same	same	52px; center delta -4px → 0px
existing `banner` padding 16px, pre-centering simulation	`focused/existing-banner-padding16__light.png`	dark/RTL/forced captured	captured	captured	52px; center delta remains 0px

Current-head target injection reached 6 descriptions and 5 action rows in each mode; no dismiss target remains. Description color/spacing and action-row geometry changed as expected. Forced colors substituted authored colors and retained geometry.

VERDICT: clear

A11Y & I18N

No role, accessible name, keyboard path, focus order, locale string, or direction contract changes. pr-a11y and pr-rtl ran and passed on this head. Chromium confirmed the root passthrough/ref, both consumer capture handlers, keyboard focus on Dismiss, and focus restoration after dismissal.

VERDICT: clear

JUDGEMENT

slot	verdict
PROBLEM	BLOCKS — existing header padding already serves one stated need
SOLUTION	BLOCKS — action target and centering are coupled private-layout work
ARCHITECTURE	BLOCKS — target on a non-painting wrapper
IMPACT	BLOCKS — permanent unnecessary row contract
API	BLOCKS — actions target does not earn permanence; description needs human judgement
THEMING	BLOCKS — layout machinery exposed
BREAKING	clear
PERFORMANCE & RESOURCES	clear
VISUAL EVIDENCE	clear
A11Y & I18N	clear

GOAL: partly met — the description target reaches a demonstrated painted need; the 52px action-row goal is already met by the existing header target.

DISPOSITION: banner-actions + centering commit → blocks now; banner-description → ready for Cindy's public-surface judgement, recommend accept.

ADVICE: proven remedy — keep banner-description; drop banner-actions and the centering commit; use banner padding for the 52px theme. Fresh Chromium reproduced 44→52px with equal centers.

WORST OUTCOME: “Themes opting into banner-actions would gain control over Banner's private row geometry and freeze that wrapper as public API.” → BLOCKS

PRIOR FINDINGS: the dismiss-control target was removed, appropriately narrowing speculative surface. The banner-actions and pixel-identical-centering findings are unchanged: range-diff shows the centering commit is byte-for-byte equivalent and only the dismiss target was removed from the target commit.

JUDGEMENT NEEDED: API — whether to accept banner-description; recommend yes because it is a stable painter with a demonstrated class and landed description precedent.

request changes

banner-actions exposes the private flex/margin/edge-compensation row → theme authors freeze Banner's layout for a 52px result the existing header padding already provides · Banner.tsx:633
The centering branch changes no shipped pixels and only supports that private margin override → it adds a visual branch and tests for the target that should not ship · Banner.tsx:541

REVIEW

Thanks for narrowing this. banner-description now names a painted element with a demonstrated use, but banner-actions still exposes the flex/wrap/margin/edge-compensation row rather than a visual surface.

In Chromium, the centering commit changes zero pixels under shipped styles in light, dark, RTL, and forced colors; it only changes pixels after that private row margin is overridden. Setting padding-block on the existing banner target gets the same 44→52px result and keeps the title/control centers equal without exposing the row.

Could we keep banner-description and drop banner-actions plus the centering commit?

[Reviewed by Robohands]

INLINE (0-3, only if genuinely line-anchored)

packages/core/src/Banner/Banner.tsx:633 — This target exposes layout-only margins; the existing header padding already provides the requested height.

EVIDENCE I DID NOT SPEND

350 targeted tests passed: 56 Banner + 294 theming-target consistency.
All required CI jobs ran and passed at this head; review-required is the only pending status.
No open PR overlaps Banner theming, and current main has not changed Banner since the reviewed base.

Round 2 — delta re-review at `d4a0a719`

Versions: Review Loop 1.6.0 · Component Audit Rubric 1.13
Head: d4a0a719a24cf86a95105e3187731a0ab82b8e46 · Verdict: approve proposed (no public PR action taken)

PR

#5483 feat(Banner): theme targets for the description and actions row by freddymeta (bucket: collaborator)

HEAD REVIEWED

d4a0a719a24cf86a95105e3187731a0ab82b8e46

VERSIONS

LOOP VERSION: 1.6.0 AUDIT RUBRIC: 1.13

LANE

LANE: full WHY: re-review after request-changes, public theme surface, and manual unchanged-pixels evidence required.

PROBLEM

WHY 1: Theme authors need Banner's supporting description line to be addressable without a structural child-order selector. WHY 2: A structural selector silently breaks when Banner's header DOM changes, so the theme can lose the intended text treatment without a testable public seam. WHY 3: Banner exists as a stable status surface; theming its stable painted anatomy should not require depending on private layout structure. USER-FACING PROBLEM: a theme author changing Banner's supporting line has to select a private DOM position, so their theme is brittle instead of using a named part. PROBLEM SEVERITY: missing capability — the task works only through a brittle selector, not a stable target.

VERDICT: clear

SOLUTION

The branch now names only the description line as a stable theme part. The layout row and title-only centering branch requested in round 1 are gone, so the remaining surface lives on the element that paints the supporting text. Themes can style the description directly while Banner keeps its action-row geometry private.

SOLUTION (1 decision · ~6 runtime lines of 25 current PR lines)

Add banner-description to the supporting description painter, documented in English and Chinese docs.

BURDEN: low — one additive theme target, no state, effects, listeners, observers, behavior, or layout branch. BURDEN MATCH: proportionate — the missing stable painter is exactly what the target adds.

VERDICT: clear

ARCHITECTURE

OWNER: Banner owns its stable painted anatomy; the existing banner, banner-icon, banner-content, and new banner-description targets name those visual surfaces. TIER 1: themeProps on the description painter; no new shared primitive or owner. TIER 2: Status vocabulary remains info | success | warning | error. SEAMS: custom icon, end content, collapse toggle, dismiss button, content panel, root passthrough. BEHAVIOR UNIT: inline — no new behavior remains.

seam	driven result
description target	exact-head Chromium renders one `astryx-banner-description` and a theme-layer rule changes its color and margin
action row	exact-head Chromium renders zero `astryx-banner-actions` targets
prior centering branch	source now has `hasActions = endContent

Theming Infrastructure says, “Target the element that carries the styling.” The remaining target sits on the element carrying description color/type/spacing; the prior layout-row target is gone.

VERDICT: clear

IMPACT

Default users see no pixel change: light, dark, RTL, and forced-colors screenshots were exact-RGBA identical before vs after. Theme authors gain one named seam for the supporting description line. No user or theme author gets the previously proposed action-row layout contract.

VERDICT: clear

API

defineTheme({
  name: 'brand',
  components: {
    'banner-description': {
      base: {color: 'var(--color-text-primary)', marginBlockStart: '2px'},
    },
  },
});

	change	public?	class	doc'd?	verdict
+	`astryx-banner-description` theme target	yes — public theme surface	`empty-state-description` is landed precedent for a description painter	en+zh	ok

OSSIFICATION: This is one more instance of an existing target shape: a description text painter. Being wrong would freeze a part name, but the part is stable and visually owned by Banner. A prior same-head maintainer approval also recorded that this is the painted surface to keep.

VERDICT: clear

THEMING

banner-description follows <component-kebab>-<part>, is documented in both doc blocks, and sits on the visible description element. banner-actions is absent from source, docs, and Chromium DOM. No token, var, existing target, or status value changes.

VERDICT: clear

BREAKING

BEHAVIOR: no changed state/default/conditional remains in the current PR diff; the old toggle-centering change was reverted. API: additive theme target only; no existing call site stops compiling or changes meaning. VISUAL: no pixel change in the covered story across light, dark, RTL, and forced-colors; each pair was 0 / 360000 differing pixels. THEME: no existing target/token/var removed; a new target is added.

VERDICT: clear

PERFORMANCE & RESOURCES

EFFECTS: zero added/changed. RENDER: no state or render-path work added. LISTENERS/OBSERVERS: none. LAYOUT: no measurement or layout reads. BUNDLE: no dependency or build-surface change.

VERDICT: clear

VISUAL EVIDENCE

VISUAL CHECK: manual frames required WHY: the diff claims unchanged pixels while adding a rendered class to visible Banner DOM.

Banner with description — warm main baseline vs exact head

Case	Before	After	Result
light			0 / 360000 pixels changed
dark			0 / 360000 pixels changed
RTL			0 / 360000 pixels changed
forced colors			0 / 360000 pixels changed

SENSOR RECEIPT: each frame used Storybook story core-banner--with-description, theme neutral, viewport 900×400@1, semantic body text sensor, target .astryx-banner, zero Storybook/page errors, and build SHA matching either warm main or d4a0a719. Light/dark/RTL/forced-colors receipts passed; before/after differed only by build and the new DOM class verified separately.

VERDICT: clear

REMEDY SEARCH

REMEDY SEARCH: not triggered — no proven visual defect

A11Y & I18N

No role, accessible name, keyboard path, focus order, user string, AT string, catalog key, physical CSS, or direction behavior changes in the current PR diff. CI pr-a11y and pr-rtl both passed at this head.

VERDICT: clear

JUDGEMENT

slot	verdict
PROBLEM	clear
SOLUTION	clear
ARCHITECTURE	clear
IMPACT	clear
API	clear
THEMING	clear
BREAKING	clear
PERFORMANCE	clear
VISUAL	clear
A11Y & I18N	clear

GOAL: met — the remaining target reaches the description painter; exact-head Chromium shows one description target and no action target.

DISPOSITION: prior banner-actions finding → fixed; prior centering-branch finding → fixed; prior banner-description human/API question → settled by existing maintainer approval at this head.

ADVICE: omitted — no blocking finding remains. Public note can ask for the stale PR title/body to be updated before merge.

AUTHOR CAN PROCEED: yes — no code change needed; update PR metadata only.

WORST OUTCOME: none found.

JUDGEMENT NEEDED: none — the only remaining public surface is an accepted instance of an existing description-target class.

approve proposed (no merge/auto action)

REVIEW

Thanks, this addresses my review: banner-actions and the centering branch are gone, and banner-description is the painted target to keep. Please update the stale title/body before merge.

[Reviewed by Robohands]

INLINE (0–3, only when genuinely line-anchored)

None.

EVIDENCE I DID NOT SPEND

Current main has not touched the PR's Banner files since this branch's base.
Same-head CI is green except the separate visual-acceptance status, which remains pending.
The public PR title/body still need metadata cleanup before merge.

TIME

TIME total 20m setup 6m rules, safety scan, exact-head worktree, fast install, one build, Storybook dev (warm main reused: yes) reading 5m prior review, PR body/comments, range-diff, source, convention pages measuring 4m 4 before/after browser pairs, target reachability probe, 1 focused Vitest run writing 4m presentation, wiki record, index update waste 1m first screenshot probe used the wrong story text sensor, then recaptured

WHAT I COULD NOT VERIFY

Nothing code-related. The public PR title/body still need a metadata cleanup before merge.

What changed before posting

Drafted only; not posted to GitHub.

Review 5483

Review #5483 — Banner theme targets

PR

HEAD REVIEWED

PROBLEM

SOLUTION

ARCHITECTURE

IMPACT

API

THEMING

BREAKING

PERFORMANCE & RESOURCES

VISUAL EVIDENCE

A11Y & I18N

JUDGEMENT

REVIEW

INLINE (0-3, only if genuinely line-anchored)

EVIDENCE I DID NOT SPEND

Round 2 — delta re-review at d4a0a719

PR

HEAD REVIEWED

VERSIONS

LANE

PROBLEM

SOLUTION

ARCHITECTURE

IMPACT

API

THEMING

BREAKING

PERFORMANCE & RESOURCES

VISUAL EVIDENCE

Banner with description — warm main baseline vs exact head

REMEDY SEARCH

A11Y & I18N

JUDGEMENT

REVIEW

INLINE (0–3, only when genuinely line-anchored)

EVIDENCE I DID NOT SPEND

TIME

WHAT I COULD NOT VERIFY

What changed before posting

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Round 2 — delta re-review at `d4a0a719`