fix(e10): review round 1, part 2 — the docs the epic invalidated, and three contracts made real
Finishes round 1. Part 1 took the runtime bugs; this takes the prose that had gone false and the three "the comment says so" gaps behind it. CLAUDE.md is auto-loaded into every agent session and ui/README.md is not, so an architecture paragraph that still described a flat components bag was the most expensive stale sentence in the repo: the next agent adding a popover reads it, sees no platform layer, and writes the generic component into components/ — the invisible-layer failure this epic exists to end. It now names components/ui/ and says the boundary is lint-enforced. The canonical-patterns table gained the three rows the epic established: ui/Button over a hand-rolled <button>, a keymap row over a private keydown listener, and disabled-when-busy vs inert-when-inapplicable. ui/README.md's inventory was written in this branch's first commit and never learned about CommandPalette, ContextMenu/useContextMenu, KeyboardHelp or gridColumns — in a section whose own closing rule is that a second component painting the same thing is how the last inconsistency happened. It also claimed "everything is rem-derived" while VirtualGrid's minCol/GRID_GAP are deliberately px; that is now stated as the exception it is, and a new section records which promises here are tests rather than sentences. Three contracts stopped being prose: - A text-less Button must carry a name. "Required for iconOnly" was a doc comment, which is why the migration shipped two nameless icon buttons through eight green lanes. It is a union now, covering both spellings (iconOnly, and size="icon"/"icon-sm"), satisfied by label, aria-label or title. It caught one more on the way in: Banner's dismiss took an optional dismissTitle, so it could render unnamed — it falls back to common.close now. - gridColumns' gutter arithmetic. Every existing assertion survived replacing floor((w + GAP) / (minCol + GAP)) with the naive floor(w / (minCol + GAP)): the fixed widths agree, the capped cases are cap-bound, and the loops compare columnsFor against itself. Pinned at the exact-fit boundaries where the two first disagree (536 and 828). - ShareDialog's link row. Only Copy was migrated in S1, leaving one 2rem button between two 1.75rem ones — and on a coarse pointer .touch-target grew only the migrated one to 2.75rem while the destructive Revoke stayed small. Also: the maxCols docstring said 8 columns of 384px where the test it shipped with asserts 369.
This commit is contained in:
@@ -1,5 +1,5 @@
|
||||
import { forwardRef } from "react";
|
||||
import type { ButtonHTMLAttributes, ReactNode } from "react";
|
||||
import type { ButtonHTMLAttributes, ReactNode, RefAttributes } from "react";
|
||||
import clsx from "clsx";
|
||||
import { Loader2 } from "lucide-react";
|
||||
|
||||
@@ -88,7 +88,7 @@ const SIZES: Record<ButtonSize, string> = {
|
||||
const BASE =
|
||||
"inline-flex items-center justify-center transition disabled:opacity-50 disabled:cursor-not-allowed";
|
||||
|
||||
export interface ButtonProps extends ButtonHTMLAttributes<HTMLButtonElement> {
|
||||
interface ButtonOwnProps extends ButtonHTMLAttributes<HTMLButtonElement> {
|
||||
variant?: ButtonVariant;
|
||||
/** Defaults to "md", except for `variant="link"`, whose default is "none" (a link owns no padding). */
|
||||
size?: ButtonSize;
|
||||
@@ -103,8 +103,22 @@ export interface ButtonProps extends ButtonHTMLAttributes<HTMLButtonElement> {
|
||||
fullWidth?: boolean;
|
||||
}
|
||||
|
||||
// A button that renders NO TEXT has to get its accessible name from somewhere, and "required for
|
||||
// `iconOnly`" was a sentence in a doc comment — so the S1 migration shipped two icon buttons that
|
||||
// announce themselves as a bare "button" and every lane stayed green (E10 review round 1). The rule
|
||||
// is a type now, in the file that owns the contract.
|
||||
//
|
||||
// Both spellings of a text-less button are covered: `iconOnly`, and the far commoner `size="icon"` /
|
||||
// `"icon-sm"` (a square box that fits one glyph). Any of the three naming props satisfies it —
|
||||
// `title` included, because the accname spec does fall back to it and ~16 call sites legitimately
|
||||
// use it for the tooltip that IS the name.
|
||||
type NamedSomehow = { label: string } | { "aria-label": string } | { title: string };
|
||||
type TextlessButton = ({ iconOnly: true } | { size: "icon" | "icon-sm" }) & NamedSomehow;
|
||||
type TextfulButton = { iconOnly?: false; size?: Exclude<ButtonSize, "icon" | "icon-sm"> };
|
||||
export type ButtonProps = ButtonOwnProps & (TextlessButton | TextfulButton);
|
||||
|
||||
/** The shared action button. `type` defaults to "button" so it never accidentally submits a form. */
|
||||
export const Button = forwardRef<HTMLButtonElement, ButtonProps>(function Button(
|
||||
const ButtonImpl = forwardRef<HTMLButtonElement, ButtonOwnProps>(function Button(
|
||||
{
|
||||
variant = "primary",
|
||||
size,
|
||||
@@ -157,3 +171,9 @@ export const Button = forwardRef<HTMLButtonElement, ButtonProps>(function Button
|
||||
</button>
|
||||
);
|
||||
});
|
||||
|
||||
// The implementation takes the permissive shape (it has to destructure every prop); the EXPORT is
|
||||
// what call sites see, and that is where the "a text-less button must be named" union binds.
|
||||
export const Button = ButtonImpl as (
|
||||
props: ButtonProps & RefAttributes<HTMLButtonElement>,
|
||||
) => ReturnType<typeof ButtonImpl>;
|
||||
|
||||
Reference in New Issue
Block a user