diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md new file mode 100644 index 00000000000..e7b8665042a --- /dev/null +++ b/.github/copilot-instructions.md @@ -0,0 +1,55 @@ +# Cloudscape components — repo conventions + +You are reviewing/writing code as a Cloudscape maintainer: terse, self-explanatory code, +no parallel implementations, strong types, and the public API kept small and consistent. +When you flag something, cite the rule and say why. These are the repo's house conventions. + +## Comments & documentation +- Comment only the non-obvious WHY (why a warning is suppressed, why a workaround exists, + an a11y constraint). Delete comments that restate what the adjacent code plainly does + ("Build tree", "Compute layout"). WHY: restatements are noise; unexplained hacks bite later. +- Never silently drop an existing explanatory comment during a refactor — the rationale + is the part a reader can't reconstruct. + +## Reuse before adding +- Before writing a helper, search `src/internal/` and `@cloudscape-design/component-toolkit` + for an existing one (e.g. `useUniqueId`, `isHTMLElement`, `findUpUntil`). Reuse existing + constants instead of hardcoding magic numbers. WHY: divergent reimplementations drift and + multiply maintenance; the toolkit versions are more complete. +- Prefer ONE shared implementation over parallel branches (sticky vs virtual, VR vs one-theme, + collapsible vs not): factor common logic into one hook/mixin. WHY: parallel copies drift apart + and double the bug surface. + +## Types over runtime checks +- Constrain values with unions/enums, not a runtime keyword array + `isX` check. Remove + redundant `as X` casts. Never use `as any`. WHY: type-level constraints catch invalid values + at compile time; runtime checks fail silently and are hard to debug. + +## Naming +- Never name by physical position — `isRightmost`/`left`/`right` flip under RTL. Use logical + names (`isLastColumn`). WHY: the codebase runs RTL; position names become wrong. +- Name by purpose, not data-structure shape or generic terms (`ColumnGroupsLayout`, not + `HierarchicalThing`; `column`, not `leaf`). WHY: shape/generic names force readers to + reverse-engineer intent. + +## i18n +- Never hardcode an English fallback. Resolve translatable strings via `useInternalI18n('component')` + and let the provider supply the default; `?? ''` is only for type-safety on required string + props, never an English string. WHY: a hardcoded fallback bypasses the provider and ships + untranslated text. + +## Scope & process +- Keep PRs scoped: don't fold an unrelated change (flipping a shared constant, a drive-by + refactor) into a feature PR — land it as its own PR first. WHY: unrelated changes hide + regressions and enlarge the blast radius. +- Treat the public API of an already-public component as a stable contract. Flag changes that break + consumers — removing/renaming a public prop or export, narrowing a prop's type, tightening + optional→required, changing an observable default, or altering published test-utils behavior — and + prefer deprecation (keep it, mark `@deprecated`, add the replacement) over removal. Additive changes + and required→optional relaxations are fine. WHY: this is a published component library; a breaking + change ripples across every consumer. (See `.github/instructions/component-structure.instructions.md` + for the file-level detail.) + +--- +Path-specific conventions (component structure & API, tests, SCSS, dev pages, test-utils) are in +`.github/instructions/*.instructions.md` and apply automatically to matching files. diff --git a/.github/instructions/component-structure.instructions.md b/.github/instructions/component-structure.instructions.md new file mode 100644 index 00000000000..508116f3f12 --- /dev/null +++ b/.github/instructions/component-structure.instructions.md @@ -0,0 +1,92 @@ +--- +applyTo: "src/*/{index,internal,interfaces,implementation,internal-interfaces}.{ts,tsx}" +--- +# Component structure & doc voice + +Apply the contract rules below — they hold regardless of a component's shape. But **structure has several +legitimate shapes** in this repo, so before flagging anything shape-related, check the accepted-variants +list further down and don't flag a component for matching one of them. Cite the rule when flagging; never +invent a divergence. + +## Contract rules — hard, shape-independent (a violation is almost certainly a bug) +- **Wrapper checklist (new, non-single-impl components):** `index.tsx` starts with `'use client';` as its + first statement, calls `applyDisplayName(X, '${Name}')`, and has `export { ${Name}Props }` + one default + export. These are near-universal in the repo — flag any that are absent. +- **forwardRef ⇔ `Props.Ref`.** If (and only if) the component exposes a `${Name}Props.Ref`, it must use + `React.forwardRef` and forward the ref. A component with **no** `Ref` correctly uses a plain function — + absence of `forwardRef` is only a problem when a `Ref` exists (or a widgetized seam blocks it). +- **`Ref` exposes methods, not the DOM node** (`focus()`, `select()`), never `HTMLElement`/`element:`. +- **This component's own sub-types live under `export namespace ${Name}Props`** — its `Ref`, its + `*Detail`/`*ChangeDetail` event payloads, and its variant/size/type aliases must NOT be loose top-level + exports. (Private sub-component prop types are exempt — see the allowlist.) +- **`getBaseProps(...)` reaches the root:** the impl (wherever it lives) calls `getBaseProps` and spreads it + on the root — on EVERY return branch (incl. loading/early returns) — so consumer `className`/`id`/`data-*` + are not dropped. Flag only if genuinely dropped. +- **`__internalRootRef` is attached to the root element** (directly or via `useMergeRefs`). +- **Internal-only props are `__`-prefixed.** Concrete test: a prop on the internal props type but NOT on the + public `${Name}Props` is internal-only → it must start with `__`. Flag any that doesn't. +- **Internal props derive from the public API** — don't re-declare a prop the public `${Name}Props` already + has on the internal props type (promote with `SomeRequired<…>`). + +## Interface typing +- A union representing a **visual variant / size / type** must be a named alias in the namespace + (`${Name}Props.Variant`). **Do NOT flag** behavioral-mode unions (`keyboardActivationMode`, + `contentRenderStrategy`) or unions referencing another component's type. +- Event handler props (`on*`) use `CancelableEventHandler` / `NonCancelableEventHandler`, + never a bare function. WHY: the build fails for non-standard event-handler types. +- Doc comments must be Documenter-extractable and describe the prop, not the component. Only flag a MISSING + doc when sibling props in the same interface are documented and this one conspicuously is not — do not + demand JSDoc on every namespace/`i18nStrings` sub-field. + +## API doc voice & tone (SOFT — surface as non-blocking nits, not blocking flags) +Existing docblocks vary, so treat these as gentle nudges toward the house voice, not hard failures. +- **Open with a third-person present-tense verb**, declaratively: `Specifies …`, `Determines …`, `Adds …`, + `Called when …`. Avoid imperative openers ("Set this to…"), questions, and addressing "you" in the FIRST + sentence (a following `Use this to…` guidance sentence may). +- **Booleans:** a third-person verb + "whether" (`Determines whether …` / `Specifies whether …`), optionally + `If \`true\`, …`. **Events:** `Called when …` (+ `The event \`detail\` contains …`). **aria props:** + `Adds \`aria-x\` to `. +- **Enums (3+ values):** a bulleted list of backticked values marking `(default)`. Inline `(default)` / + "Defaults to…" prose is fine; only flag a redundant hand-written scalar `@default` **tag** (Documenter + extracts defaults). **Mechanics:** capitalize + terminating period; backtick code tokens and sibling-prop + references; terse and factual (no marketing, no inline rationale beyond one guidance clause); a + `@deprecated` note names its replacement. + +## Data flow (in the impl, not the wrapper — subject to the wrapper-side-hooks variant below) +- State/controllability (`useControllable`), context (`useFormFieldContext`), events + (`fireNonCancelableEvent`), i18n (`useInternalI18n`, no hardcoded English fallback) live in the impl. +- Call `warnOnce` directly — it already gates on the dev environment; don't wrap it in a separate + `isDevelopment` check. +- `useBaseComponent`'s config arg (`{ props, metadata }`) is a fire-once-on-mount usage-telemetry + snapshot, so put only stable, low-cardinality config in it. `props` carries actual public config prop + values passed through directly — enumerated `variant`/`size`/`type`/density unions, booleans, and + low-cardinality numbers (a numeric prop value like `limit` in `props` is fine). `metadata` carries + derived facts — booleans (`hasX: Boolean(...)`) or numbers (counts) — especially reductions of + high-cardinality inputs. Never pass `children`, i18n strings, handlers, ids/identifiers, labels, or + user values; reduce any high-cardinality signal to a boolean/number in `metadata`. Omit the arg when + nothing is trackable. + +## Accepted architectural variants — DO NOT flag these +- **Single-impl:** a component with no `internal.tsx` folds its implementation into `index.tsx` (plus + helper files). `getBaseProps`, state, events, context, `forwardRef` and the root node legitimately live + in `index.tsx` here (e.g. tabs, tag-editor, badge, textarea). +- **Widgetized / loadable:** `internal.ts(x)` creates or re-exports a widgetized component, optionally + wrapping its implementation in `createLoadableComponent`; the real implementation may live in + `implementation.tsx`, and internal props may live there or in `internal-interfaces.ts` (e.g. flashbar, + breadcrumb-group, drawer, split-panel, side-navigation, help-panel). Don't expect the implementation + body, `getBaseProps`, or root element in `internal.tsx`; `forwardRef` may be impossible through the seam. +- **Wrapper-side hooks:** `index.tsx` may run analytics/funnel hooks, `useSetGlobalX`, + `getAnalyticsMetadataAttribute`, or a loading/skeleton fallback (alert, modal, breadcrumb-group, button, + input). "Thin wrapper" is a tendency, not a rule — only flag wrapper logic with no such purpose. +- **`internal.tsx` holding private sub-components** (`TagControl`, `UndoButton`) rather than an `InternalX` is fine. +- **Sanctioned imports:** importing a sibling's `internal.tsx`/`implementation.tsx`, or another component's + **publicly exported** interface, is normal. Only flag importing another component's **styles** or clearly + private files across `src//`. +- **Top-level prop types of private SUB-COMPONENTS** (`BreadcrumbItemProps`, `TagControlProps`) are not + `${Name}Props` sub-types — do not force them into the namespace. (This exemption is ONLY for sub-component + prop types — see the namespace contract rule for the component's own sub-types.) + +## Generic sweep (best effort, lowest priority) +- After the above, compare against 2–3 sibling components and flag any OTHER real divergence not covered + here (an undocumented prop where siblings document theirs, unusual file naming). Respect the accepted + variants — do not flag a shape that matches a known variant. Name the sibling you compared against. diff --git a/.github/instructions/dev-pages.instructions.md b/.github/instructions/dev-pages.instructions.md new file mode 100644 index 00000000000..696460b7b62 --- /dev/null +++ b/.github/instructions/dev-pages.instructions.md @@ -0,0 +1,19 @@ +--- +applyTo: "pages/**" +--- +# Dev / demo page conventions +- Build pages with the `SimplePage` helper (h1, content paddings, i18n, screenshot area), not + hand-rolled bespoke scaffolding. WHY: consistent, low-boilerplate, avoids a11y violations. + EXCEPTIONS — don't flag these page types for not using `SimplePage`: (a) **AppLayout demo pages**, + which can't nest inside `SimplePage` (itself a content wrapper); (b) **permutations pages** — the + `*.permutations.page.tsx` shape built from `createPermutations` + `PermutationsView` + `ScreenshotArea` + with a bare `

` heading is the established structure for screenshot-matrix pages. +- Keep pages minimal: render the components under test directly (a single `SpaceBetween` is enough); + no decorative containers/subtitles, no hand-maintained prose list of components. WHY: test pages + should isolate behavior; decorative structure and lists drift. +- Drive page configuration (variant, selection, toggles) from URL query params via `AppContext`, + not local `useState` or click-only buttons. WHY: URL state is directly targetable by integ tests + and shareable as a link. +- Reuse shared fixtures (`generateItems`, shared column configs, `common-props`) instead of + duplicating; add `screenshotArea={{}}` for coverage and keep the page header outside it. WHY: + deduplicates fixtures, keeps screenshots isolated. diff --git a/.github/instructions/styles.instructions.md b/.github/instructions/styles.instructions.md new file mode 100644 index 00000000000..f900fa7afad --- /dev/null +++ b/.github/instructions/styles.instructions.md @@ -0,0 +1,12 @@ +--- +applyTo: "src/**/*.scss" +--- +# SCSS conventions +- Use existing design tokens instead of hardcoded values (`$box-shadow-focused-width`, radius + tokens). WHY: keeps theming consistent. +- Put classes that exist only for test selection in `src//test-classes/styles.scss`. + WHY: segregated test classes keep integ selectors stable. +- Don't disable lint rules or depend on raw DOM structure / `nth-child` merely as a shortcut — add + class names to inner elements and target those when possible. When a structural selector or suppression + is necessary for component behavior, scope it narrowly and document why. WHY: unqualified + structure-dependent selectors and rule overrides are brittle and re-break as markup changes. diff --git a/.github/instructions/test-utils.instructions.md b/.github/instructions/test-utils.instructions.md new file mode 100644 index 00000000000..eda8e22ae88 --- /dev/null +++ b/.github/instructions/test-utils.instructions.md @@ -0,0 +1,10 @@ +--- +applyTo: "src/test-utils/**" +--- +# Test-utils (public contract) conventions +- Selector methods must generate a STATIC CSS selector with no access to query results — they + can't branch on `nth-child` conditionally. Design the DOM (e.g. always-present `data-column-index`) + so one static selector works, or take an explicit mode param. WHY: test-utils are a cross-version + consumer contract; a selector that needs results to decide its shape can't be generated. +- Test-utils methods declare explicit return types; wrapper classes expose a static `rootSelector`; + always-present results use a non-null assertion. WHY: documented public-contract conventions. diff --git a/.github/instructions/tests.instructions.md b/.github/instructions/tests.instructions.md new file mode 100644 index 00000000000..49a884b5a62 --- /dev/null +++ b/.github/instructions/tests.instructions.md @@ -0,0 +1,23 @@ +--- +applyTo: "**/{__tests__,__integ__,__a11y__,__motion__}/**" +--- +# Testing conventions +- Assert the observable EFFECT (focus lands on the specific element, value/count/position is + correct), never `toBeDefined()`/"renders"/"does not crash". A test with no real assertion isn't + a test. WHY: `toBeDefined()` passes even when focus lands on `document.body`. +- Never write filler tests just to keep patch/codecov green — restructure or test real behavior. + WHY: coverage-gaming tests assert accidental behavior and are frequently AI-generated noise. +- Test through the component/public surface; find elements with official test-utils + (`createWrapper().findX()`), not raw class-name/DOM selectors. WHY: proves the helper works AND + is wired correctly; test-utils are the stable contract. EXCEPTION: when no test-utils finder exists + for the element/state (e.g. an animation/settle state, or a styling primitive like Box whose public + contract IS its class names), a scoped class assertion is acceptable. +- Split unit vs integ by what each can verify: explicit handlers/logic in unit, real focus/Tab in + integ. Don't fire an event on an element the user can't focus. WHY: emulating an impossible + interaction verifies nothing; integ repetition is expensive. +- Don't `istanbul ignore` testable code (e.g. keyboard nav — testable on a plain ``). + Restructure so the util is reachable. WHY: ignored branches hide real, testable behavior. +- Assert dev warnings by mocking `warnOnce` from `@cloudscape-design/component-toolkit/internal` + and asserting `(componentName, message)`. WHY: the established deterministic pattern. +- Name tests by user-observable behavior, not internal util names. WHY: internal names in titles + rot and don't communicate intent. diff --git a/AGENTS.md b/AGENTS.md index d9c849c1580..bce852371f8 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -11,6 +11,14 @@ See [docs/SETUP.md](docs/SETUP.md) for setup, building, and running locally. See [docs/CLOUDSCAPE_COMPONENTS_GUIDE.md](docs/CLOUDSCAPE_COMPONENTS_GUIDE.md) for guides on component conventions, styling, writing tests, and more. For running tests and configs, see [docs/RUNNING_TESTS.md](docs/RUNNING_TESTS.md). +## Coding conventions + +Conventions for writing and reviewing code in this repo live in +[.github/copilot-instructions.md](.github/copilot-instructions.md) (repo-wide) and +[.github/instructions/](.github/instructions/) (file-scoped: component structure & public API, tests, +SCSS, dev pages, test-utils). Tools that support these files apply them automatically; they also serve as +the review bar for hand-written code. Follow them when writing or changing code here. + ## Conventions to watch -- **Commit and PR titles: `type: subject`, no scope.** The PR-title lint (`cloudscape-design/actions/.github/workflows/lint-pr.yml`) allows exactly these types: `chore`, `feat`, `fix`, `refactor`, `test`, `revert`. The title must start with `type:` followed by the subject (for example `feat: Add multi-column sort`). Scope parentheses are not supported (`feat(table): …` fails the check), and other Conventional Commits types such as `docs` or `style` are not allowed — use `chore:` for documentation and tooling changes. +- **PR titles: `type: subject`, no scope.** The PR-title lint (`cloudscape-design/actions/.github/workflows/lint-pr.yml`) allows exactly these types: `chore`, `feat`, `fix`, `refactor`, `test`, `revert`. The title must start with `type:` followed by the subject (for example `feat: Add multi-column sort`). Scope parentheses are not supported (`feat(table): …` fails the check), and other Conventional Commits types such as `docs` or `style` are not allowed — use `chore:` for documentation and tooling changes. Only the PR title is checked; individual commit messages are not.