-
Notifications
You must be signed in to change notification settings - Fork 0
Review 5399
athz · collaborator (treated as internal, no Discord clause) · OPEN · view on GitHub
Verdict: request changes — drafted, not posted. Commissioned draft-only.
Head reviewed: 92524b9b936e96f6e8bcc79b68b507a279bb468c (merge-base 581f8444eda)
None. Both endpoints checked — zero reviews, zero review comments; the only
issue comments are vercel[bot] and the PR Analysis Report. Nothing to extend,
narrow or contradict. main has not moved under ChatComposerDrawer.tsx since
the branch point (R31c), and CI genuinely ran on this head rather than merely
rendering green (R32): test 14m16s, pr-a11y 1m23s, pr-rtl 1m4s.
None stated. The body's motivation — "The Badge added visual weight that isn't
needed for a simple count indicator in a collapsed row" — is a design preference
about a shipped component, with no person and no worse outcome. The 7a rationale
grep over the body returns 0 hits. Filed as fix(chat) from a feat/ branch,
which is R1f: the type asserts a defect the body does not claim, and it routes
the change away from the design eyes it needs.
One decision, 3 runtime lines of 19: the collapsed count stops being a Badge
chip and becomes part of the label's text run — {count} {label} in one span
(ChatComposerDrawer.tsx:299-303). The test and the changeset evidence that
decision; they are not further decisions.
No API change. But the diff loads a new obligation onto an existing public
prop: label is a free-form consumer string and is now half of a sentence.
Docs: en+zh, and both now wrong — five places still describe a badge
(ChatComposerDrawer.doc.mjs:10, :33, :67, :83 and ChatComposerDrawer.tsx:44),
untouched by the diff, ungated by check-components.
One removed, none added. .astryx-badge + data-variant="neutral" leave the
collapsed toggle; the replacement span carries no astryx class. Measured in
Chromium: classes: "astryx-badge , -" → classes: "-". .astryx-badge is a
generated theme rule target, asserted at generateThemeRules.test.ts:496. The
drawer's root target survives, so a theme can still reach the drawer — what is
gone is the only handle on the count. No in-repo consumer is broken today
(R14g: 1 grep hit, and it is the test).
Nothing added. The finding runs the other way: the plain-text form cannot be
made correct, because a plural lives in the noun and the noun is the
consumer's. No catalog key of the shape {count} {label} exists that works.
The chip form sidestepped this by not being prose.
API no · Visual YES · Theme YES. The count goes 12px/w500, rgb(38,38,38) on rgb(229,229,229), radius 9999px → 14px/w400, rgb(82,82,82) on the drawer's own paint. Everything got smaller (−16.8 to −19.7px wide,
height unchanged at 20.0px in all four cases), so nothing reflows — checked in a
real ChatComposer, not only in a story. Contrast on the count 11.90:1 → 6.93:1
(AA yes, AAA no).
ChatComposerDrawer.tsx:303 composes user-visible text in JSX. Measured DOM: one
span, three text nodes — "3", " ", "Attachments". Driven consequences:
| case | renders | should be |
|---|---|---|
count={1}, default label |
"1 Items" | a plural arm |
shipped Feedback story (ChatComposer.stories.tsx:397) |
"1 User feedback requested" | — |
zh, default label 项目
|
"3 项目" |
"3 个项目" — zh-CN.json:235 ships exactly that for the same noun |
count={1234} |
"1234 Attachments" | "1,234" via {count, number}
|
Convention gate (STEP 5b): 12 for, 1 against, and the 1 is a bug. en.json
composes a count with a noun as {count, number} + an ICU plural arm 12 times.
Components composing one in JSX: 0. The single sibling doing it the author's way
is PowerSearch.tsx:341, which bypasses @astryx.powersearch.valueEditor.filtersCount
(en.json:251) — a known violation, pre-existing, not charged to athz (R13).
Not an a11y block, deliberately. The sentence "a user who … is stuck /
cannot hear it / cannot reach it / cannot get out" cannot be finished. The
Playwright aria snapshot is byte-identical across both builds —
button "Expand Attachments": 3 Attachments — because the name comes from
aria-label, not from the toggle's content. The Badge carried no accessible
affordance: no role, no aria, no live region, not focusable. So the answer to
"does removing the Badge remove an affordance" is a driven no, and the block
is I6 alone.
-
The block is filed one slot below its cause (R34a). Deleting a catalog
entry fixes nothing; deleting
:303fixes everything. So the cause is SOLUTION's single decision, and the ask is not "wire it through the catalog". -
Four notes compound — no stated problem, two unclaimed visual side
effects, a removed theme handle, stale en+zh docs — into one thing: an
unsourced design change to a shipped component arriving under a
fixlabel. That promotes the design question to a human step.
JUDGEMENT NEEDED: design — chip or plain text for the collapsed count.
Design Conventions has no rule; its Design-vs-Structure table
only says a change of this kind is Core. Cindy's.
Filed as Rulings entry 10 in the same turn (R2g), because the
comment names a horizon. Exit state is held for a ruling, not a nudge (R24):
the next mover is Cindy, not athz. Recommendation on the page: keep the chip; if
the ask is weight, that is a theme call on .astryx-badge.
Thanks for this — the collapsed row is heavy, you're right.
Merging the count into the label makes it a sentence, and a sentence needs the catalog.
count={1}renders "1 Items"; the Feedback story renders "1 User feedback requested". In Chinese the default label is 项目, so we render "3 项目" where we already ship "3 个项目". 1234 loses its grouping.(before/after frames embedded —
assets/pr-5399)
labelis the consumer's string, though, so no key of the form{count} {label}fixes it — the plural lives in the noun. If the goal is just less weight, that's a theme call on the badge, which was also the only handle a theme had on the count. I'd keep the chip.Either way "1 Items" can't ship, and chip-vs-text is ours to call — we'll come back this week. Nothing to rework meanwhile; if you have a view on weight vs the pill, say so, since that's the call.
[Reviewed by Robohands]
149 words. Three inlines: the :303 pointer at the existing ICU key, the lost
theme handle at :299, and the changeset crediting @aldentan where the rest of
the tree credits the GitHub login (@athz).
Frames on the fork branch assets/pr-5399 (commit 1d919a5): the collapsed
matrix at 7 cases LTR and RTL, the Feedback and WithManyAttachments stories
collapsed in a real ChatComposer, and the expanded state proving it is
unchanged. BEFORE and AFTER are one browser session, one install, with the
component reverted to the merge-base and HMR asserted by a phase-aware ready
predicate — .astryx-badge must appear for BEFORE and be absent for AFTER — so
neither phase can read the other's render.
Probes added back to the kit: probe-kit/drawer-5399-frames.cjs,
probe-kit/drawer-5399-contrast.cjs.
One hypothesis was refuted and that is recorded rather than buried: RTL was expected to break, because a bidi text run orders differently from a flex row. It does not — the count trails the label in RTL in both builds. Not an RTL regression.
Nothing posted. Three critic passes:
| pass | result |
|---|---|
| 1 | FAIL on 5 — comment voice (an aphorism, a "Thanks —" hinge), no frame embedded despite ten captured and published (R15-gate), findings unmarked (R16e), an A4 row labelled "auto-covered" that was never driven (R34c), and padding. Plus two verdict lines contradicting their own slot prose, and one false claim — "the only thing a theme can reach here", when the drawer's root target survives |
| 2 | FAIL on 1 — R2g: the comment said "don't rework anything until we come back", a promise nothing had filed. Fixed by filing Rulings 10 in the same turn and moving the question to the end so the author has an action |
| 3 | clean, two wording residues |