review-component-pr

# Review Component PR A checklist to use on a diff. The rules are in the [Coding Guidelines](../../../.github/CODING_GUIDELINES.md). Read the `spec.md` of the component before the diff. A change that contradicts the spec is a bug, or the author must also update the spec. Review in this order. The public API is hard to change after release, so review it early. ## 1. Structure - [ ] `[name].ts` (single default export), `[name].spec.ts` and `spec.md` in `src/components/[name]/` - [ ] `stories/[name].stories.ts`, with a filename that matches the tag - [ ] Complete theme scaffold, with every file in `themes.ts` - [ ] Exported from `src/index.ts` in alphabetical order. No new exports from `src/internals` beyond the approved list in [Project Structure](../../../.github/CODING_GUIDELINES.md#project-structure). - [ ] `#internals` / `#theming` / `#animations` aliases for cross-cutting imports. The theme aggregator through `#themes`. Relative imports between components. `.js` specifiers. - [ ] A new alias is in `package.json` **and** in `scripts/_package.json` ## 2. Public API and Documentation - [ ] `tagName`, `styles` (`componentBase` first), `register()` (with all rendered dependencies, directly), `HTMLElementTagNameMap`. A new component is also in `defineAllComponents.ts`. - [ ] An intended public API change comes with the updated `public-api.json` (`npm run public-api:update`). A removal or a type change has a deprecation plan. - [ ] Only primitives are attributes. Complex types use `attribute: false` and do not reflect. - [ ] Booleans default to `false`. Multi-word attributes are kebab-case and explicit. - [ ] Events use `EventEmitterMixin` with a typed map. Names are `igc` + camelCase, cancelable events end in `-ing` and the code checks their return value. - [ ] Events come from user interaction, not from property sets or method calls - [ ] JSDoc tags come after the description. `@deprecated since [SemVer]. Use the \`[new]\` [type] instead.` - [ ] No `igc-` tag names in description prose. No "…attribute of…", no `Gets/Sets`. Booleans start with "Whether" and match the `true` state. ```bash # Tag-name leak check: expect no output outside @element/@example grep -rn "igc-" --include="*.ts" src/ \ | grep -E "^\S+:[0-9]+:\s*\*" \ | grep -vE "@element|@example|\.spec\.ts" ``` ## 3. Specification Map each change to a spec section with [Keeping it current](../../../.github/CODING_GUIDELINES.md#keeping-it-current). - [ ] A new component has a `spec.md` in the splitter structure - [ ] API tables match the JSDoc for each added, renamed, deprecated or removed member - [ ] Keyboard, ARIA and limitations sections are updated where the behavior changed - [ ] Test scenarios mirror the `describe` blocks and are numbered contiguously. Gaps are listed under `### Not covered by the suite`. - [ ] `## Revision history` has a new row - [ ] New headings have TOC entries. Anchors and relative sibling links resolve. ## 4. Accessibility - [ ] The a11y audit covers `shadowDom` and the light DOM - [ ] Semantic elements are used, not `div`s with click handlers - [ ] ARIA is set through `addInternalsController` (`initialARIA`, `setARIA()`, `reflectRole`), never with `this.role = …` - [ ] The host `aria-label`, `aria-labelledby` and `aria-describedby` reach the focused or role element through `HostAriaMixin` and `ariaBindings(hostAria(this))`, with no own `observedAttributes` for them - [ ] Keyboard support uses `addKeybindings` or `addRovingFocusController`. Focus is visible. On a `delegatesFocus` item, the roving tab index is on the host, not on an inner element. - [ ] Composite hosts use `addAriaProjector` / `addAriaTarget`, with no ARIA on a `delegatesFocus` host. Cross-root relations use element reflection, not IDREFs. - [ ] Theme selectors use `data-role` / `data-haspopup`, not `role` / `aria-*` - [ ] Cross-component access uses `internalsOf()`, not new `@hidden` public members - [ ] Cross-root ARIA is tested with `runExternalLabelAssociationTests` / `runAriaProjectionTests`. Relations are checked by identity readback. `axeReflectedRelationsOptions` is used only next to such a check. ## 5. Behavior - [ ] Region fences and member order follow the guidelines. Internal members use `_`. No `#` fields. `readonly` on fields that are not reassigned. No `any`. - [ ] Derived state in `willUpdate()`. DOM side effects in `update()` with `super.update()`. Both guarded by `changedProperties.has()`. - [ ] Coercion and per-set side effects use `@coercedProperty`, not a hand-written backing-field accessor pair - [ ] Existing internals are reused (controllers, `resizable()` / `draggable()`, `createTimer`, `internals/utils`), not written again - [ ] Dynamic `window` / `document` listeners are removed in `disconnectedCallback` - [ ] User-facing strings come from `I18nMixin` / `addI18nController`, with defaults from `igniteui-i18n-core` - [ ] Form controls: the correct mixin, `createFormValueState`, `__validators` from `#internals/validators.js`, `setValueAndFormState()`, re-validation through `@coercedProperty` on constraint properties, and `_handleBlur` / `_handleEnterKeydown` on the native editor. No copied touched/pristine logic. `formResetCallback` overrides call `super`. ## 6. Styles and Themes - [ ] No generated `.css.ts` in the diff - [ ] Load-path specifiers. Values come from `var-get()` and the theming functions. - [ ] `[part~='…']` selectors. Dark files emit only `dark-overrides()`. No per-theme sheet is imported outside `themes.ts`. - [ ] All four themes work in light and dark mode. `:host` has a `display` value. Specificity is low. ## 7. Tests and Generated Artifacts - [ ] `defineComponents()` in `before()`. `elementUpdated()` after programmatic changes. - [ ] Tests cover defaults, reflection, events, interaction and edge cases - [ ] Interaction uses `#internals/testing/simulate.spec.js`. Forms use `createFormAssociatedTestBed` and the validity helpers. - [ ] No spec imports another component's spec. Shared helpers are in `src/internals/testing/`. - [ ] A new or changed parser, converter or serializer has property-based tests in `[module].property.spec.ts` - [ ] The story's `// region default` block was regenerated (`cem` + `build:meta`), not edited - [ ] CHANGELOG updated ## 8. Build and Hygiene - [ ] `npm run check`, `npm run lint` and `npm run test` pass - [ ] No `console.log`, `debugger` or commented-out code. No unexplained magic numbers. - [ ] No new heavy third-party dependency ## Frequent Findings | Finding | Why it matters | | ----------------------------------------- | ---------------------------------------------------------------- | | Missing `addThemingController` | The component ignores theme changes | | Relative import into `internals` | `npm run check` fails | | Alias only in `package.json` | Breaks only for consumers of the published package | | Relative import of a theme aggregator | The publish build fails; single-theme bundles get every theme | | `public-api.json` not updated | `npm run check` lists the additions; the snapshot goes stale | | `igc-` in a description | Goes into the API docs of every framework wrapper | | Hand-edited story region or `.css.ts` | Overwritten on the next build | | `[part='base']` with `partMap` | Stops matching when a second part name is added | | ARIA on a `delegatesFocus` host | Assistive technology reads the native editor | | New `@hidden` public member | Leaks into the public API. Use `internalsOf()`. | | Hand-written accessor pair for coercion | `@coercedProperty` does this in fewer lines | | API change with no `spec.md` change | The spec no longer describes the component | | Spec scenario with no test | Shows coverage that does not exist | ## Verdict **Request changes** if the a11y audit is missing or fails, ARIA is on the wrong element, `any` or `#` fields are in the code, generated files are edited or stale, themes are incomplete, the public API has no documentation, or `spec.md` does not match the behavior. **Approve** if the checklist passes and `check`, `lint` and `test` pass. Each comment must give the file, the line and the guideline it applies.