diff --git a/.claude/skills/component/SKILL.md b/.claude/skills/component/SKILL.md index 546bf035..829ae8a3 100644 --- a/.claude/skills/component/SKILL.md +++ b/.claude/skills/component/SKILL.md @@ -182,6 +182,7 @@ See [props.md](references/props.md) for naming conventions. | [polymorphism.md](references/polymorphism.md) | render vs asChild patterns | | [collection.md](references/collection.md) | Collections, portals, virtualization | | [anti-patterns.md](references/anti-patterns.md) | Common component mistakes | +| [videojs.md](references/videojs.md) | Video.js component architecture | For accessibility patterns (ARIA, keyboard, focus), load the `aria` skill. diff --git a/.claude/skills/component/references/videojs.md b/.claude/skills/component/references/videojs.md new file mode 100644 index 00000000..4c9316e0 --- /dev/null +++ b/.claude/skills/component/references/videojs.md @@ -0,0 +1,246 @@ +# Video.js Component Architecture + +Video.js components use a three-layer architecture separating framework-agnostic logic from platform implementations. + +## Architecture Overview + +``` +┌─────────────────────────────────────────────────────────────┐ +│ @videojs/core Framework-agnostic business logic │ +│ @videojs/core/dom Shared DOM utilities │ +└─────────────────────────────────────────────────────────────┘ + │ + ┌─────────────────┴─────────────────┐ + ▼ ▼ + @videojs/html @videojs/react + Web Components (Lit) React Components +``` + +| Package | Responsibility | +|---------|----------------| +| `@videojs/core` | Core classes with `getState()`/`getAttrs()`/actions | +| `@videojs/core/dom` | DOM utilities, selectors, button behavior | +| `@videojs/html` | Web Components consuming core via controllers | +| `@videojs/react` | React components consuming core via hooks | + +--- + +## Core Class Pattern + +Every UI component has a `*Core` class in `@videojs/core`. + +### Props Interface + +```ts +interface PlayButtonProps { + /** Custom label for the button. */ + label?: string | undefined; + /** Whether the button is disabled. */ + disabled?: boolean | undefined; +} +``` + +- All props optional with `| undefined` (explicit optionality) +- JSDoc for each prop + +### State Interface + +```ts +interface PlayButtonState extends Pick {} +``` + +- Primitives only — no methods +- Use `Pick` to select relevant fields + +### Core Class + +```ts +class PlayButtonCore { + static readonly defaultProps: NonNullableObject; + + setProps(props: Props): void; // Merge with defaults + getLabel(state: FeatureState): string; // Computed label + getAttrs(state: FeatureState): ElementProps; // ARIA only + getState(state: FeatureState): State; // Primitives only + toggle(state: FeatureState): Promise; // Action +} + +namespace PlayButtonCore { + export type Props = PlayButtonProps; + export type State = PlayButtonState; +} +``` + +**Rules:** + +- `static readonly defaultProps` with `NonNullableObject` type +- `getAttrs()` returns ARIA attributes only (no `data-*`) +- `getState()` returns primitives only (no methods) — converted to `data-*` for CSS +- Action methods receive feature state from store +- Namespace exports `Props` and `State` types + +--- + +## State vs Attrs Separation + +| Method | Returns | Purpose | +|--------|---------|---------| +| `getAttrs()` | ARIA attributes | Accessibility (`aria-label`, `aria-disabled`) | +| `getState()` | Primitives | CSS styling via `data-*` attributes | + +**Why separate:** + +- CSS targets `[data-paused]`, `[data-ended]` selectors +- ARIA attrs remain semantically accurate +- Can update independently + +--- + +## Data Attribute Enums + +Enums provide single source of truth + API reference tooling: + +```ts +export enum PlayButtonDataAttrs { + /** Present when the media is paused. */ + paused = 'data-paused', + /** Present when the media has ended. */ + ended = 'data-ended', +} +``` + +JSDoc comments generate API documentation. + +--- + +## ElementProps + +Shared interface in `core/element.ts`. Extend as components need new attributes: + +- Add `aria-*` attributes used by any component +- Use string literal types where ARIA spec defines allowed values +- `undefined` removes the attribute + +--- + +## Web Component (Lit) + +```ts +class PlayButtonElement extends MediaElement { + static readonly tagName = 'media-play-button'; + static override properties = { label: { type: String }, disabled: { type: Boolean } }; + + readonly #core = new PlayButtonCore(); + readonly #state = new PlayerController(this, playerContext, selectPlayback); + #disconnect: AbortController | null = null; + + // Lifecycle: see flow below +} +``` + +**Lifecycle flow:** + +1. `connectedCallback` — Create `AbortController`, apply button props via `applyElementProps()` +2. `disconnectedCallback` — Abort controller for cleanup +3. `willUpdate` — Sync component props to core via `setProps()` +4. `update` — Apply `getAttrs()` and `getState()` to element + +**Key utilities:** + +- `PlayerController(host, context, selector)` — Store subscription +- `applyElementProps(el, props, signal)` — Apply attrs + events +- `applyStateDataAttrs(el, state)` — State → `data-*` +- `logMissingFeature(name, feature)` — Deduped warning + +--- + +## React Component + +```tsx +const PlayButton = forwardRef(function PlayButton(props, ref) { + const playback = usePlayer(selectPlayback); + const [core] = useState(() => new PlayButtonCore()); + const { getButtonProps, buttonRef } = useButton({ onActivate, isDisabled }); + + return renderElement('button', { render, className, style }, { + state: core.getState(playback), + ref: [ref, buttonRef], + props: [core.getAttrs(playback), elementProps, getButtonProps()], + }); +}); +``` + +**Flow:** + +1. `usePlayer(selector)` — Subscribe to store slice +2. `useState(() => new Core())` — Lazy init core class +3. `useButton()` — Get accessible button behavior +4. `renderElement()` — Render with state→data-attrs, ref composition, props merge + +**Props type:** `UIComponentProps` allows `className`/`style` as functions of state. + +--- + +## Shared Utilities + +### @videojs/core/dom + +| Utility | Purpose | +|---------|---------| +| `createButton(options)` | Accessible button (Enter/Space, click, disabled) | +| `applyElementProps(el, props, signal?)` | Apply attrs and events to DOM | +| `applyStateDataAttrs(el, state)` | State object → `data-*` attributes | +| `getStateDataAttrs(state)` | State → data-attrs object (React) | +| `logMissingFeature(name, feature)` | Deduped console.warn | +| `selectPlayback` / `selectVolume` | Store selectors | + +### @videojs/react/utils + +| Utility | Purpose | +|---------|---------| +| `renderElement(tag, props, params)` | Render with state, refs, props merge | +| `mergeProps(...propSets)` | Chain events, concat className, merge style | +| `composeRefs(...refs)` | Compose refs (React 19 cleanup support) | + +--- + +## File Organization + +``` +packages/ +├── core/src/ +│ ├── core/ +│ │ ├── element.ts # ElementProps interface +│ │ └── ui/{component}/ +│ │ ├── {component}-core.ts # Core class +│ │ ├── {component}-core.test.ts # Core tests +│ │ └── {component}-data-attrs.ts # Data attr enum +│ └── dom/ui/ # createButton, utils +├── html/src/ +│ ├── ui/{component}/ # Web Component +│ ├── define/ui/ # Side-effect registration +│ └── player/player-controller.ts # Store controller +└── react/src/ + ├── ui/{component}/ # React component + ├── ui/hooks/ # Behavior hooks + └── utils/ # renderElement, mergeProps +``` + +--- + +## Component Registration + +```ts +// define/ui/play-button.ts +customElements.define(PlayButtonElement.tagName, PlayButtonElement); + +declare global { + interface HTMLElementTagNameMap { + [PlayButtonElement.tagName]: PlayButtonElement; + } +} +``` + +- Tag name: `static readonly tagName = 'media-{name}'` +- Registration in `define/ui/` directory +- Augment `HTMLElementTagNameMap` for TypeScript diff --git a/.claude/skills/component/review/checklist.md b/.claude/skills/component/review/checklist.md index 5714e8ea..88ca5496 100644 --- a/.claude/skills/component/review/checklist.md +++ b/.claude/skills/component/review/checklist.md @@ -1,334 +1,30 @@ # Component Review Checklist -Comprehensive checklist for reviewing UI components against architecture patterns and conventions. +Checklists for reviewing UI components against architecture patterns and conventions. ---- +## Checklists -## Architecture +| Checklist | Use For | +|-----------|---------| +| [general.md](checklists/general.md) | Standard headless component patterns (architecture, state, props, styling) | +| [videojs.md](checklists/videojs.md) | Video.js-specific patterns (core class, platform adapters) | +| [severity.md](checklists/severity.md) | Anti-patterns and severity classification | -### Compound Components +## Quick Selection -- [ ] Component uses compound structure (Root, Trigger, Content, etc.) -- [ ] Each part maps 1:1 to a DOM element -- [ ] Parts can be reordered or omitted freely -- [ ] No prop explosion (>10 props suggests need for decomposition) +**Building a new Video.js component?** +→ Use [general.md](checklists/general.md) + [videojs.md](checklists/videojs.md) -**Detection:** Single component with many configuration props +**Reviewing component architecture?** +→ Use [general.md](checklists/general.md) -```tsx -// BAD: Prop explosion - - -// GOOD: Compound - - - - ... - - - -``` - -### Standard Hierarchies - -| Type | Expected Parts | -| ----------- | ---------------------------------------------------- | -| Popups | Root → Trigger → Portal → Positioner → Popup → Arrow | -| Collections | Root → List → Trigger + Panel | -| Forms | Root → Label → Control → Description → Error | - -### Context Scoping - -- [ ] Each Root creates isolated context -- [ ] Nested instances don't interfere - -**Detection:** Nested components share unintended state - ---- - -## State Management - -### Controlled & Uncontrolled Support - -- [ ] Both modes supported: `value` (controlled) and `defaultValue` (uncontrolled) -- [ ] Works correctly in either mode -- [ ] No state desync between modes - -**Detection:** Only `defaultValue` or only `value` supported - -| State | Uncontrolled | Controlled | Handler | -| ------- | ---------------- | ---------- | ----------------- | -| Open | `defaultOpen` | `open` | `onOpenChange` | -| Value | `defaultValue` | `value` | `onValueChange` | -| Checked | `defaultChecked` | `checked` | `onCheckedChange` | - -### Change Event Details - -- [ ] Handler receives value and details object -- [ ] Details includes `reason` (click, keyboard, blur, escape, etc.) -- [ ] Details includes `event` (original DOM event) -- [ ] Details includes `cancel()` for preventing change - -**Detection:** Handler receives only value, no context - -```typescript -// BAD -onOpenChange?: (open: boolean) => void; - -// GOOD -onOpenChange?: (open: boolean, details: ChangeDetails) => void; -``` - -### Imperative Actions - -- [ ] `actionsRef` prop exposes imperative methods where needed -- [ ] Common actions: `open()`, `close()`, `toggle()`, `focus()` - ---- - -## Props & API - -### Boolean Props - -- [ ] Use positive adjectives: `disabled`, `required`, `open` -- [ ] Avoid `is`/`has` prefixes: not `isDisabled`, `isOpen` - -| Good | Avoid | -| ---------- | ------------ | -| `disabled` | `isDisabled` | -| `open` | `isOpen` | -| `loading` | `isLoading` | - -### Event Handler Naming - -- [ ] Pattern: `on` + Noun + Verb -- [ ] Specific names over generic: `onValueChange` not `onChange` - -| Good | Avoid | -| --------------- | ----------------------- | -| `onOpenChange` | `handleOpen`, `setOpen` | -| `onValueChange` | `onChange` | -| `onSelect` | `onItemSelected` | - -### Standard Props by Category - -**Interaction:** - -- [ ] `disabled?: boolean` -- [ ] `required?: boolean` -- [ ] `readOnly?: boolean` - -**Collections:** - -- [ ] `multiple?: boolean` -- [ ] `loopFocus?: boolean` -- [ ] `orientation?: 'horizontal' | 'vertical'` - -**Popups:** - -- [ ] `modal?: boolean` -- [ ] `closeOnEscape?: boolean` -- [ ] `closeOnOutsideClick?: boolean` -- [ ] `keepMounted?: boolean` - -### Refs - -- [ ] Ref forwarded to root DOM element -- [ ] Parent can access DOM for focus, measurement - -**Detection:** `forwardRef` not used - -```tsx -// BAD -const Button = ({ children }) => ; - -// GOOD -const Button = forwardRef(({ children, ...props }, ref) => ( - -)); -``` - -### Polymorphism - -- [ ] Uses `render` prop or `asChild`, not `as` prop -- [ ] `render` function receives props and state -- [ ] State accessible for conditional rendering - -**Detection:** Component has `as` prop - -```tsx -// BAD: TypeScript performance issues - -``` - ---- - -## Data Attributes & Styling - -### State via Data Attributes - -- [ ] State exposed via `data-*` attributes -- [ ] Enables CSS-only styling without JS - -**Detection:** Inline styles for state, no data attributes - -| Attribute | When Present | -| ------------------ | --------------------------- | -| `data-open` | Component is open | -| `data-closed` | Component is closed | -| `data-checked` | Toggle is checked | -| `data-disabled` | Component is disabled | -| `data-highlighted` | Item has focus within group | -| `data-side` | Popup position side | -| `data-align` | Popup alignment | - -```tsx -// BAD -