diff --git a/.github/CODING_GUIDELINES.md b/.github/CODING_GUIDELINES.md index 8998693dc..b569f0f59 100644 --- a/.github/CODING_GUIDELINES.md +++ b/.github/CODING_GUIDELINES.md @@ -24,6 +24,9 @@ 7. Prefer explicitly defining a function shape over using `Function` as a type. 8. Don't use TypeScript `namespace`. 9. Prefer simple union types over `enum`. +10. Internal API (properties, methods, getters, setters) should be prefixed with an underscore (`_`). +11. Use the `readonly` modifier for properties that should not be reassigned. +12. Specify return types for functions and methods explicitly rather than relying on inference, unless the type is obvious or causes unnecessary clutter in the source code. ## Components @@ -31,14 +34,12 @@ `src/components/[component]/[component].ts` - Where - - Stick to a single export from the component file, that is the component class itself. - Testing file(s) should be also in the same directory following the `[component-name].spec.ts` pattern. - CSS styles and theming assets usually live in `src/components/[component]/themes/*`. - Anything else is a fair game as long as it has consistent and meaningful naming. -- When adding a new component or modifying an existing one, try to stick to the following code structure: +- When adding a new component or modifying an existing one, stick to the following code structure. Use region comments to clearly delineate sections of the component. ```ts export default class IgcFooBarComponent extends LitElement { @@ -55,127 +56,131 @@ export default class IgcFooBarComponent extends LitElement { * Since Ignite UI web components are not self-registering by themselves, * each component should implement the `register` static method. * The `registerComponent` call will add the component to the custom elements - * registry (if not already present) and all its dependant components. + * registry (if not already present) and all its dependent components. */ - public static register() { + public static register(): void { registerComponent(IgcFooBarComponent, ...); } - /* ... */ - - /** Internal non-reactive members. */ - - private foo = 0; + //#region Internal state and properties - protected get bar() { - return this.foo * 2; - } - - /* ... */ + private _foo = 0; + private readonly _controller = addSomeController(this); - /** References to internal DOM nodes */ + @state() + protected _invalid = false; @query('input') - private inputElement!: HTMLInputElement; + private _inputElement!: HTMLInputElement; @queryAssignedElements({ selector: IgcFooChildComponent.tagName }) - protected fooChildren!: Array; + protected _fooChildren!: Array; - /* ... */ - - /** Internal reactive members */ - - @state() - protected invalid = false; + protected get _bar(): number { + return this._foo * 2; + } - /* ... */ + //#endregion - /** Public reactive properties */ + //#region Public attributes and properties + /** + * The value of the component. + * @attr + */ @property() public value = ''; - /* ... */ - - /** Public non reactive properties */ - - public get complete() { - return this.invalid; - } - - /* ... */ - - /** Observed/Computed properties handlers */ + /** + * Determines whether the component is disabled. + * @attr + */ + @property({ type: Boolean, reflect: true }) + public disabled = false; - @watch('value') - protected valueChange() { - this.invalid = !!this.value; + /** Returns whether the component is complete. */ + public get complete(): boolean { + return this._invalid; } - /* ... */ - - /** Constructor if initialization logic is required */ + //#endregion constructor() { super(); - this.addEventListener('input', this.handleInput); + this.addEventListener('input', this._handleInput); } - /** Custom element/Lit lifecycle methods if required */ + //#region Lit lifecycle methods - public override connectedCallback() { + public override connectedCallback(): void { super.connectedCallback(); // ... } - protected override firstUpdated(changedProperties) { + protected override willUpdate(changedProperties: PropertyValues): void { + // Compute derived state before rendering + if (changedProperties.has('value')) { + this._invalid = !!this.value; + } + } + + protected override update(changedProperties: PropertyValues): void { + // Handle side effects or sync state with DOM access + if (changedProperties.has('disabled')) { + this._updateAriaDisabled(); + } + super.update(changedProperties); + } + + protected override firstUpdated(changedProperties: PropertyValues): void { // ... } - /* ... */ + //#endregion - /** Internal event handlers */ + //#region Event handlers - private handleInput(event: InputEvent) { + private _handleInput(event: InputEvent): void { // ... } - /* ... */ + //#endregion - /** Internal methods */ + //#region Internal API - private resetState() { + private _resetState(): void { // ... } - protected updateState() { + private _updateAriaDisabled(): void { // ... } - /* ... */ + protected _updateState(): void { + // ... + } - /** Public methods */ + //#endregion - public reset() { - this.resetState(); - } + //#region Public API - /* ... */ + /** Resets the component to its initial state. */ + public reset(): void { + this._resetState(); + } - /** Renderer helper methods */ + //#endregion - protected renderContainer() { + protected _renderContainer() { // ... } - protected renderInput() { + protected _renderInput() { // ... } - - /** The overridden `render` method */ protected override render() { - return html`${this.renderInput()}${this.renderContainer()}`; + return html`${this._renderInput()}${this._renderContainer()}`; } } @@ -193,6 +198,50 @@ declare global { } ``` +- **Component Structure Guidelines:** + 1. **Static members** come first (no region fence needed). + 2. Use `//#region Internal state and properties` for all internal reactive and non-reactive state, controllers, DOM queries, and internal getters/setters. + 3. Use `//#region Public attributes and properties` for all public reactive properties and read-only getters. + 4. **Constructor** follows the public properties section (no region fence). + 5. Use `//#region Lit lifecycle methods` for `connectedCallback`, `disconnectedCallback`, `willUpdate`, `update`, `firstUpdated`, etc. + 6. Use `//#region Event handlers` for all event handler methods. + 7. Group internal methods in appropriately named regions based on their behavior or function (e.g., `//#region Keyboard navigation`, `//#region Form integration`, `//#region Internal API`). + 8. Use `//#region Public API` for all public methods. + 9. **Rendering methods** and the `render()` override come last (no region fence needed). + +- **Computed and Derived State:** + + Prefer using Lit's lifecycle methods (`update` or `willUpdate`) over the `@watch` decorator for handling property changes and computing derived state. + - Use `update()` when you need DOM access or want to trigger side effects. + - Use `willUpdate()` for computing derived state before rendering. + - The `@watch` decorator should be avoided in new code. + + > [!TIP] DO + > + > ```ts + > protected override willUpdate(changedProperties: PropertyValues): void { + > if (changedProperties.has('value')) { + > this._invalid = this.value.length < this.minLength; + > } + > } + > + > protected override update(changedProperties: PropertyValues): void { + > if (changedProperties.has('disabled')) { + > this._updateAriaAttributes(); + > } + > super.update(changedProperties); + > } + > ``` + + > [!WARNING] DON'T + > + > ```ts + > @watch('value') + > protected valueChange(): void { + > this._invalid = this.value.length < this.minLength; + > } + > ``` + - After adding new component(s) to the library, make sure to export them from the entry point of the package: ```ts @@ -203,6 +252,205 @@ export { default as IgcFooBarComponent } from './components/foobar/foobar.js'; /* ... */ ``` +## Imports + +- Organize imports in the following order, with blank lines between groups: + 1. Lit imports (`lit`, `lit/decorators.js`, `lit/directives/*`) + 2. Third-party library imports + 3. Internal utilities and controllers (`../common/*`) + 4. Component imports + 5. Type imports last (if not inline) + + > [!TIP] DO + > + > ```ts + > import { html, LitElement } from 'lit'; + > import { property, query } from 'lit/decorators.js'; + > + > import { addThemingController } from '../../theming/theming-controller.js'; + > import { addSlotController } from '../common/controllers/slot.js'; + > import { registerComponent } from '../common/definitions/register.js'; + > + > import IgcIconComponent from '../icon/icon.js'; + > + > import { styles } from './themes/badge.base.css.js'; + > import type { StyleVariant } from '../types.js'; + > ``` + +## Controllers + +- Controllers are reusable pieces of logic that hook into a component's lifecycle. Use controllers from `src/components/common/controllers/` for common functionality: + - `addThemingController` - Required for theme support + - `addSlotController` - For managing slotted content + - `addInternalsController` - For ElementInternals and ARIA management + - `addKeybindings` - For keyboard navigation + - And others as needed + +- Controllers should be stored as `readonly` class fields and initialized inline: + + ```ts + private readonly _slots = addSlotController(this, { + slots: setSlots(), + onChange: this._handleSlotChange, + }); + ``` + +## Slots + +- Use slots to allow content composition. Document all slots with `@slot` JSDoc tags. +- The default slot typically holds the main content. +- Named slots serve specific purposes (e.g., `prefix`, `suffix`, `header`). +- Use `addSlotController` to react to slot content changes: + + ```ts + private readonly _slots = addSlotController(this, { + slots: setSlots('prefix', 'suffix'), + onChange: this._handleSlotChange, + }); + + private _handleSlotChange(): void { + this._hasPrefix = this._slots.hasAssignedElements('prefix'); + } + ``` + +## Shadow DOM and CSS Parts + +- All components use Shadow DOM for style encapsulation (mode: `'open'` by default). +- Expose internal elements as CSS parts using the `part` attribute to allow external styling: + + ```ts + /** + * @csspart base - The main container + * @csspart input - The native input element + */ + protected override render() { + return html` +
+ +
+ `; + } + ``` + +- Use the `partMap` directive for conditional parts: + + ```ts + import { partMap } from '../common/part-map.js'; + + protected override render() { + return html` +
+ ... +
+ `; + } + ``` + +- For delegating focus to internal elements, use the `@shadowOptions` decorator: + + ```ts + import { shadowOptions } from '../common/decorators/shadow-options.js'; + + @shadowOptions({ delegatesFocus: true }) + export default class IgcInputComponent extends LitElement { + // Focus is automatically delegated to the first focusable element + } + ``` + +## Accessibility + +Accessibility is a first-class requirement for all components. + +- **Always test accessibility** - Components must pass a11y audits in tests. +- Use semantic HTML elements where appropriate (``; +> } +> +> // NO CLEANUP NEEDED - Lit manages component instance listeners +> constructor() { +> super(); +> this.addEventListener('focus', this._handleFocus); +> } +> +> // CLEANUP REQUIRED - Dynamic external listeners +> private _handler = this._handleResize.bind(this); +> +> public override connectedCallback(): void { +> super.connectedCallback(); +> window.addEventListener('resize', this._handler); +> } +> +> public override disconnectedCallback(): void { +> window.removeEventListener('resize', this._handler); +> super.disconnectedCallback(); +> } +> +> // addSafeEventListener prevents SSR errors +> constructor() { +> super(); +> // Safe in SSR - won't error if addEventListener is unavailable +> addSafeEventListener(this, 'pointerdown', this._handlePointer); +> } +> ``` ## API Documentation @@ -382,3 +897,70 @@ export { default as IgcFooBarComponent } from './components/foobar/foobar.js'; ## Changelog - When adding a new component or fixing a bug make sure to update the [CHANGELOG](https://github.com/IgniteUI/igniteui-webcomponents/blob/master/CHANGELOG.md) file with the relevant changes. + +## Storybook + +All components should have a corresponding Storybook story in `stories/[component-name].stories.ts`. + +- Stories provide interactive examples and documentation for components. +- Use Storybook controls to make all public properties configurable. +- Include multiple stories showcasing different component states and configurations. + +```ts +import type { Meta, StoryObj } from '@storybook/web-components-vite'; +import { html } from 'lit'; +import { IgcBadgeComponent, defineComponents } from 'igniteui-webcomponents'; + +defineComponents(IgcBadgeComponent); + +const metadata: Meta = { + title: 'Badge', + component: 'igc-badge', + argTypes: { + variant: { + options: ['primary', 'info', 'success', 'warning', 'danger'], + control: { type: 'select' }, + }, + }, + args: { variant: 'primary' }, +}; + +export default metadata; + +export const Basic: StoryObj = { + render: (args) => html`Badge`, +}; +``` + +## Resources + +- **Project Documentation:** [README.md](https://github.com/IgniteUI/igniteui-webcomponents/blob/master/README.md) +- **Lit Documentation:** [lit.dev](https://lit.dev/docs/) +- **Web Components:** [MDN Web Components](https://developer.mozilla.org/en-US/docs/Web/Web_Components) +- **Accessibility:** [WCAG Guidelines](https://www.w3.org/WAI/WCAG21/quickref/) +- **TypeScript:** [TypeScript Handbook](https://www.typescriptlang.org/docs/handbook/intro.html) + +## Getting Help + +- Review existing components in `src/components/` for patterns and examples +- Read the [LLM Skills](../.github/skills/README.md) for guided workflows +- Ask questions in pull request reviews + +## Checklist for New Components + +Before submitting a PR for a new component, ensure: + +- [ ] Component follows the standard structure with region fences +- [ ] All internal APIs prefixed with underscore (`_`) +- [ ] Theming controller added in constructor +- [ ] Accessibility tested and passing +- [ ] All properties properly documented with JSDoc +- [ ] Events use EventEmitterMixin with type map +- [ ] CSS parts exposed and documented +- [ ] Slots documented with `@slot` tags +- [ ] Comprehensive tests including a11y audit +- [ ] Storybook story created with controls +- [ ] Component exported from `src/index.ts` +- [ ] CHANGELOG updated +- [ ] No TypeScript errors or warnings +- [ ] Code formatted (auto-formatted on save) diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 00f3f6d61..e627b7ca7 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -10,12 +10,15 @@ This project involves creating a library of reusable web components using the Li - Use standard ESM imports. - TypeScript imports end with `.js` extension. +- Internal API (properties, methods) must be prefixed with `_`. +- Use `readonly` for immutable properties and specify explicit return types. - Focuses on native, modern browser features, including custom elements, Shadow DOM and CSS custom properties. - Follows latest ECMAScript standards and best practices with the exception of native private fields. - Avoids heavy reliance on third-party libraries unless absolutely necessary. - Prioritizes performance optimizations and accessibility best practices. - Writes clean, maintainable, and well-documented code. - Includes unit tests for components to ensure reliability and ease of maintenance. +- Uses `@open-wc/testing` for component tests with mandatory accessibility audits. ### TypeScript Best Practices @@ -27,10 +30,12 @@ This project involves creating a library of reusable web components using the Li - Components should be self-contained and encapsulated. - Use Shadow DOM to encapsulate styles and markup. -- Ensure components are accessible, following WCAG guidelines. +- Organize code with region comments: Internal state, Public properties, Lit lifecycle, Event handlers, Internal API, Public API. +- Accessibility is **mandatory** - all components must pass accessibility audits and follow WCAG guidelines. - Optimize for performance, minimizing re-renders and unnecessary DOM updates. - Expose component attributes **only** for "primitive" types (string, number, boolean). - Prefer composition over inheritance for component reuse. +- Use `update()` or `willUpdate()` for derived state; avoid `@watch` decorator. ### Styling Guidelines @@ -49,3 +54,5 @@ This project involves creating a library of reusable web components using the Li - [Lit Cheat Sheet](https://lit.dev/articles/lit-cheat-sheet/) - [Lit Context API](https://lit.dev/docs/data/context/) - [Web Components Basics](https://developer.mozilla.org/en-US/docs/Web/Web_Components) +- [Coding Guidelines](CODING_GUIDELINES.md) - Comprehensive coding standards and best practices +- [Skills Directory](skills/) - Step-by-step guides for common tasks (creating components, adding properties, updating styles, reviewing PRs) diff --git a/.github/skills/README.md b/.github/skills/README.md new file mode 100644 index 000000000..4d44edc90 --- /dev/null +++ b/.github/skills/README.md @@ -0,0 +1,45 @@ +# LLM Agent Skills + +This directory contains skills for GitHub Copilot and other LLM agents to help with common development tasks in this repository. + +## What are Skills? + +Skills are structured instructions that help AI agents understand and execute project-specific workflows consistently. Each skill is a self-contained guide for a particular task. + +## Available Skills + +| Skill | Description | Use When | +| ----------------------------------------------------- | ---------------------------------------------------------------------------------- | ------------------------------------ | +| [create-new-component](./create-new-component/) | Create a new Lit web component with all necessary files | Creating new components from scratch | +| [add-component-property](./add-component-property/) | Add a reactive property to an existing component | Extending component functionality | +| [update-component-styles](./update-component-styles/) | Update component styling following SCSS workflow | Modifying component appearance | +| [review-component-pr](./review-component-pr/) | Comprehensive code review checklist ensuring quality, accessibility, and standards | Reviewing component pull requests | + +## How to Use + +When working with an AI agent, reference skills by name: + +- "Follow the create-new-component skill to add a progress-bar component" +- "Use the add-component-property skill to add an 'orientation' prop to the divider" + +## Contributing New Skills + +Skills follow the [VS Code agent skills format](https://code.visualstudio.com/docs/copilot/customization/agent-skills): + +1. Create a new directory: `.github/skills/skill-name/` +2. Add a `SKILL.md` file with YAML frontmatter: + ```yaml + --- + name: skill-name + description: Brief one-line description + --- + ``` +3. Follow the structure in existing skills +4. Update this README with the new skill + +## Naming Convention + +- Directory names: `lowercase-kebab-case` +- File name: `SKILL.md` (uppercase, always) +- Skill names in frontmatter: match directory name +- Frontmatter: Only use `name` and `description` (required). Optional: `user-invokable`, `argument-hint`, `compatibility`, `disable-model-invocation`, `license`, `metadata` diff --git a/.github/skills/add-component-property/SKILL.md b/.github/skills/add-component-property/SKILL.md new file mode 100644 index 000000000..459331288 --- /dev/null +++ b/.github/skills/add-component-property/SKILL.md @@ -0,0 +1,303 @@ +--- +name: add-component-property +description: Add a reactive property to an existing Lit web component with proper decorators, types, tests, and documentation +--- + +# Add Component Property + +This skill guides you through adding a new reactive property to an existing Lit web component. + +## Example Usage + +- "Add an 'orientation' property to the divider component" +- "Add a 'disabled' attribute to the button component" +- "Add a 'variant' property with multiple options" + +## Related Skills + +- [create-new-component](../create-new-component/) - Create a component first +- [update-component-styles](../update-component-styles/) - Style changes for properties + +## When to Use + +- User asks to add a new property or attribute to a component +- Component needs new configuration options +- Extending component functionality with new settings + +## Prerequisites + +- [ ] Target component exists +- [ ] Property name and type are determined +- [ ] Default value is decided +- [ ] Whether property should reflect to attribute is decided + +## Required Context + +Gather or confirm: + +- **Component name**: Which component to modify +- **Property name**: camelCase for property, kebab-case for attribute +- **Property type**: Primitive (string, number, boolean) or complex (object, array) +- **Default value**: What value should it have initially +- **Reflect to attribute**: Should changes reflect to HTML attribute? (only for primitives) +- **Property purpose**: Brief description for documentation + +## Steps + +### 1. Add Property to Component Class + +Add the property with appropriate decorator to the component class: + +**For primitive types (reflect to attribute)**: + +```typescript +/** + * [Property description] + * @attr [attribute-name] + * @default [default-value] + */ +@property({ reflect: true }) +public propertyName: PropertyType = defaultValue; +``` + +**For readonly properties**: + +```typescript +/** + * [Property description - read-only] + */ +@property({ attribute: false }) +public readonly readonlyProp: PropertyType = defaultValue; +``` + +**For boolean types**: + +```typescript +/** + * [Property description] + * @attr [attribute-name] + * @default false + */ +@property({ type: Boolean, reflect: true }) +public propertyName = false; +``` + +**For number types**: + +```typescript +/** + * [Property description] + * @attr [attribute-name] + */ +@property({ type: Number, reflect: true }) +public propertyName = 0; +``` + +**For complex types (no reflection)**: + +```typescript +/** + * [Property description] + */ +@property({ attribute: false }) +public propertyName: ComplexType = defaultValue; +``` + +### 2. Update Component Render Method + +If the property affects rendering, update the `render()` method: + +```typescript +protected override render() { + return html` +
+ +
+ `; +} +``` + +### 3. Add Property Change Handler (if needed) + +If the property requires side effects or needs to sync computed/dependent properties, use Lit's lifecycle hooks. + +**Preferred: Use `update()` hook** (DOM is available, best for side effects): + +```typescript +protected override update(changedProperties: PropertyValues): void { + if (changedProperties.has('propertyName')) { + // Handle property change with DOM access + // Update dependent properties or trigger side effects + // Example: Update ARIA attributes, sync internal state, etc. + } + super.update(changedProperties); +} +``` + +**If absolutely needed: Use `willUpdate()`** (before render, no DOM access): + +```typescript +protected override willUpdate(changedProperties: PropertyValues): void { + if (changedProperties.has('propertyName')) { + // Handle property change before rendering + // Use only if you need to compute values before render + // No DOM access available here + } +} +``` + +**Best Practices**: + +- Prefer `update()` for most cases - DOM is available for queries and side effects +- Use `willUpdate()` only when you need to compute derived state before rendering +- Always call `super.update(changedProperties)` when overriding `update()` +- Check `changedProperties.has()` to avoid unnecessary work + +### 4. Update Tests + +Add tests for the new property in `[component-name].spec.ts`: + +```typescript +it('should have correct default value', async () => { + const el = await fixture( + html`` + ); + + expect(el.propertyName).to.equal(defaultValue); +}); + +it('can change property', async () => { + const el = await fixture( + html`` + ); + + el.propertyName = newValue; + await elementUpdated(el); + + expect(el.propertyName).to.equal(newValue); +}); + +it('reflects to attribute', async () => { + const el = await fixture( + html`` + ); + + expect(el.propertyName).to.equal(value); + expect(el.getAttribute('property-name')).to.equal(value); +}); +``` + +### 5. Update Storybook Story + +Add the property to `stories/[component-name].stories.ts`: + +**Update argTypes**: + +```typescript +argTypes: { + propertyName: { + type: 'string', // or 'boolean', 'number' + description: '[Property description]', + control: 'text', // or 'boolean', 'number', 'select' + table: { defaultValue: { summary: 'defaultValue' } }, + }, + // ... other properties +} +``` + +**Update args**: + +```typescript +args: { + propertyName: defaultValue, + // ... other properties +} +``` + +**Update interface**: + +```typescript +interface IgcComponentArgs { + /** [Property description] */ + propertyName: PropertyType; + // ... other properties +} +``` + +**Update story template**: + +```typescript +export const Basic: Story = { + render: (args) => html` + Content + `, +}; +``` + +### 6. Verify and Test + +Run tests and verify in Storybook: + +```bash +# Run tests +npm run test + +# Check that the project will transpile +npm run check-types + +# Start Storybook +npm run storybook +``` + +## Validation Checklist + +- [ ] Property added with `@property` decorator +- [ ] JSDoc comment includes `@attr` for primitives +- [ ] Type annotation correct +- [ ] Default value appropriate +- [ ] `reflect: true` only for primitives +- [ ] Tests cover default value +- [ ] Tests cover property changes +- [ ] Tests cover attribute reflection (if applicable) +- [ ] Storybook argTypes updated +- [ ] Storybook args updated +- [ ] Storybook interface updated +- [ ] Story template uses new property +- [ ] All tests pass +- [ ] Property works in Storybook + +## Common Pitfalls + +### 1. Reflecting Non-Primitive Types + +**Problem**: Trying to reflect objects/arrays to attributes +**Solution**: Use `attribute: false` for complex types + +### 2. Wrong Attribute Name Conversion + +**Problem**: camelCase not mapping to kebab-case +**Solution**: Lit auto-converts, or specify explicitly: `@property({ attribute: 'custom-name' })` + +### 3. Missing Type Coercion + +**Problem**: Boolean/number properties not converting from string attributes +**Solution**: Use `{ type: Boolean }` or `{ type: Number }` in decorator + +### 4. Forgetting to Update Storybook + +**Problem**: New property not controllable in Storybook +**Solution**: Add to argTypes, args, interface, and template + +## Reference Examples + +See `src/components/badge/badge.ts` for examples of: + +- String property with reflection: `variant` +- Boolean property: `outlined` +- Enum-like property: `shape` + +## Resources + +- [Lit Reactive Properties](https://lit.dev/docs/components/properties/) +- [Lit Decorators](https://lit.dev/docs/api/decorators/) +- [Coding Guidelines](../../CODING_GUIDELINES.md) - Comprehensive coding standards diff --git a/.github/skills/create-new-component/SKILL.md b/.github/skills/create-new-component/SKILL.md new file mode 100644 index 000000000..15770f3f0 --- /dev/null +++ b/.github/skills/create-new-component/SKILL.md @@ -0,0 +1,508 @@ +--- +name: create-new-component +description: Create a new Lit web component following project conventions, including component class, styles, tests, Storybook story, and proper exports +--- + +# Create New Component + +This skill guides you through creating a new Lit web component that follows all project conventions and best practices. + +## Example Usage + +- "Create a new progress-bar component" +- "Add a new stepper component to the library" +- "Create a tooltip component with accessibility support" + +## Related Skills + +- [add-component-property](../add-component-property/) - Add properties after creating the component +- [update-component-styles](../update-component-styles/) - Modify component styles + +## When to Use + +- User asks to "create a new component" +- User requests a new UI element for the component library +- User wants to add a new custom element to the project + +## Prerequisites + +Before starting, ensure: + +- [ ] Component name is determined (kebab-case for tag name, PascalCase for class name) +- [ ] Basic component requirements/properties are known +- [ ] Component type identified (simple display, form-associated, or container) + +## Required Context + +Gather or confirm with the user: + +- **Component name**: e.g., "progress-bar" → `IgcProgressBarComponent` +- **Component purpose**: Brief description for JSDoc comments +- **Initial properties**: Any properties the component should expose +- **CSS parts**: What internal parts should be styleable from outside +- **Slots**: What named/default slots are needed + +## Steps + +### 1. Create Component Directory Structure + +Create the component folder with theme directories: + +```bash +mkdir -p src/components/[component-name]/themes/{light,dark,shared} +``` + +**Example**: For "progress-bar": + +```bash +mkdir -p src/components/progress-bar/themes/{light,dark,shared} +``` + +### 2. Create Component TypeScript File + +Create `src/components/[component-name]/[component-name].ts`: + +```typescript +import { html, LitElement } from 'lit'; +import { property } from 'lit/decorators.js'; +import { addThemingController } from '../../theming/theming-controller.js'; +import { registerComponent } from '../common/definitions/register.js'; +import { styles } from './themes/[component-name].base.css.js'; +import { styles as shared } from './themes/shared/[component-name].common.css.js'; +import { all } from './themes/themes.js'; + +/** + * [Component description - one line] + * + * @element igc-[component-name] + * + * @slot - [Default slot description] + * @slot [slot-name] - [Named slot description if any] + * + * @csspart [part-name] - [Description of the CSS part] + * + * @cssproperty --[property-name] - [Description if using CSS custom properties] + */ +export default class Igc[ComponentName]Component extends LitElement { + public static readonly tagName = 'igc-[component-name]'; + public static override styles = [styles, shared]; + + /* blazorSuppress */ + public static register(): void { + registerComponent(Igc[ComponentName]Component); + } + + //#region Public properties + + /** + * [Property description] + * @attr [attribute-name] + */ + @property({ reflect: true }) + public someProp = 'default-value'; + + //#endregion + + constructor() { + super(); + addThemingController(this, all); + } + + //#region Lit lifecycle + + protected override render() { + return html` +
+ +
+ `; + } + + //#endregion +} + +declare global { + interface HTMLElementTagNameMap { + 'igc-[component-name]': Igc[ComponentName]Component; + } +} +``` + +**Critical Points**: + +- Import paths end with `.js` extension (TypeScript ESM convention) +- Use `@property` decorator for reactive properties +- **Only expose primitive types** (string, number, boolean) as attributes +- Always include `tagName` and `register()` static members with `readonly` modifier +- **Organize code with region comments**: Internal state, Public properties, Lit lifecycle, Event handlers, Internal API, Public API +- **Prefix internal API** (private properties/methods) with underscore: `_internalMethod()` +- Add theming controller in constructor +- Include comprehensive JSDoc comments +- Declare global HTMLElementTagNameMap interface +- Use explicit return types for methods + +### 3. Create Base SCSS File + +Create `src/components/[component-name]/themes/[component-name].base.scss`: + +```scss +@use '../../../styles/utilities' as *; + +:host { + display: block; +} + +[part='base'] { + // Component styles here +} +``` + +**Note**: This will be transpiled to `.ts` by the build system. Do NOT create `.ts` files manually. + +### 4. Create Shared Theme Files + +Create theme-specific SCSS files: + +- `themes/shared/[component-name].bootstrap.scss` +- `themes/shared/[component-name].material.scss` +- `themes/shared/[component-name].fluent.scss` +- `themes/shared/[component-name].indigo.scss` + +**Minimal theme** (`themes/shared/[component-name].bootstrap.scss`): + +```scss +@use '../../../../theming/functions' as *; + +:host { + // Theme-specific styles using igniteui-theming package +} +``` + +Repeat for all four themes (bootstrap, material, fluent, indigo). + +### 5. Create Theme Aggregator + +Create `src/components/[component-name]/themes/themes.ts`: + +```typescript +import { css } from 'lit'; + +import type { Themes } from '../../../theming/types.js'; +// Shared Styles +import { styles as bootstrap } from './shared/[component-name].bootstrap.css.js'; +import { styles as material } from './shared/[component-name].material.css.js'; +import { styles as fluent } from './shared/[component-name].fluent.css.js'; +import { styles as indigo } from './shared/[component-name].indigo.css.js'; + +const light = { + bootstrap: css` + ${bootstrap} + `, + material: css` + ${material} + `, + fluent: css` + ${fluent} + `, + indigo: css` + ${indigo} + `, +}; + +const dark = { + bootstrap: css` + ${bootstrap} + `, + material: css` + ${material} + `, + fluent: css` + ${fluent} + `, + indigo: css` + ${indigo} + `, +}; + +export const all: Themes = { light, dark }; +``` + +### 6. Transpile SCSS to TypeScript + +Run the build script to convert SCSS to Lit CSS: + +```bash +npm run build:styles +``` + +**Verify**: `.css.js` files are created next to `.scss` files. + +### 7. Create Component Tests + +Create `src/components/[component-name]/[component-name].spec.ts`: + +```typescript +import { elementUpdated, expect, fixture, html } from '@open-wc/testing'; +import { defineComponents } from '../common/definitions/defineComponents.js'; +import Igc[ComponentName]Component from './[component-name].js'; + +describe('[ComponentName]', () => { + before(() => { + defineComponents(Igc[ComponentName]Component); + }); + + it('passes the a11y audit', async () => { + const el = await fixture( + html`` + ); + + await expect(el).shadowDom.to.be.accessible(); + await expect(el).to.be.accessible(); + }); + + it('should initialize with default values', async () => { + const el = await fixture( + html`` + ); + + expect(el.someProp).to.equal('default-value'); + }); + + it('should render content inside', async () => { + const content = 'Test Content'; + const el = await fixture( + html`${content}` + ); + + expect(el).dom.to.have.text(content); + }); + + it('can change properties', async () => { + const el = await fixture( + html`` + ); + + el.someProp = 'new-value'; + await elementUpdated(el); + + expect(el.someProp).to.equal('new-value'); + }); +}); +``` + +**Testing Requirements**: + +- Always include accessibility audit test first +- Test default initialization +- Test property reactivity +- Use `elementUpdated()` after programmatic changes + +### 8. Create Storybook Story + +Create `stories/[component-name].stories.ts`: + +```typescript +import type { Meta, StoryObj } from '@storybook/web-components-vite'; +import { html } from 'lit'; + +import { + Igc[ComponentName]Component, + defineComponents, +} from 'igniteui-webcomponents'; + +defineComponents(Igc[ComponentName]Component); + +const metadata: Meta = { + title: '[ComponentName]', + component: 'igc-[component-name]', + parameters: { + docs: { + description: { + component: '[Component description for Storybook docs]', + }, + }, + }, + argTypes: { + someProp: { + type: 'string', + description: '[Property description]', + control: 'text', + table: { defaultValue: { summary: 'default-value' } }, + }, + }, + args: { + someProp: 'default-value', + }, +}; + +export default metadata; + +interface Igc[ComponentName]Args { + /** [Property description] */ + someProp: string; +} +type Story = StoryObj; + +export const Basic: Story = { + render: (args) => html` + + Content + + `, +}; +``` + +**Story Guidelines**: + +- Match argTypes to component properties +- Use property binding (`.propName`) for non-primitives +- Create at least one basic story + +### 9. Export Component from Main Index + +Add to `src/index.ts` in alphabetical order: + +```typescript +export { default as Igc[ComponentName]Component } from './components/[component-name]/[component-name].js'; +``` + +**Example**: + +```typescript +export { default as IgcProgressBarComponent } from './components/progress-bar/progress-bar.js'; +``` + +### 10. Type-Check and Test + +Run type-checking and test suite: + +```bash +# Check that the project will transpile +npm run check-types + +# Run tests +npm run test + +# Start Storybook to verify +npm run storybook +``` + +## Validation Checklist + +Verify all of the following: + +- [ ] Component file at `src/components/[name]/[name].ts` +- [ ] Extends `LitElement` (or appropriate mixin) +- [ ] `tagName` and `register()` static members defined +- [ ] JSDoc comments with `@element`, `@slot`, `@csspart` +- [ ] Theming controller added in constructor +- [ ] SCSS files exist in themes directory +- [ ] `themes.ts` aggregator imports all themes +- [ ] Test file with accessibility tests +- [ ] Storybook story file +- [ ] Exported from `src/index.ts` alphabetically +- [ ] TypeScript compiles (`npm run check-types`) +- [ ] Tests pass (`npm run test`) +- [ ] Component appears in Storybook +- [ ] All theme variants render correctly + +## Common Pitfalls + +### 1. Forgetting SCSS Transpilation + +**Problem**: Importing `.css.js` files that don't exist +**Solution**: Always run `npm run build:styles` after creating SCSS files + +### 2. Wrong Import Extension + +**Problem**: Using `.ts` instead of `.js` in imports +**Solution**: This project uses `.js` extensions for TypeScript imports (ESM standard) + +### 3. Exposing Non-Primitives as Attributes + +**Problem**: Trying to expose objects/arrays as HTML attributes +**Solution**: Per project guidelines, only primitives (string, number, boolean) should be attributes + +### 4. Missing Global Declaration + +**Problem**: TypeScript doesn't recognize custom element tag +**Solution**: Always include `declare global { interface HTMLElementTagNameMap {...} }` + +### 5. No Theming Controller + +**Problem**: Component doesn't respond to theme changes +**Solution**: Add `addThemingController(this, all)` in constructor + +### 6. Missing `export default` + +**Problem**: Import errors in other files +**Solution**: Always use `export default class` for component classes + +### 7. Wrong Export Order + +**Problem**: Exports not alphabetized in `src/index.ts` +**Solution**: Find correct alphabetical position before adding + +## Reference Examples + +### Simple Display Component: Badge + +See: `src/components/badge/badge.ts` + +**Characteristics**: + +- Extends `LitElement` directly +- Uses `@property` decorators +- Includes theming controller +- Simple render with slots +- Dynamic CSS parts with `partMap` + +### Form-Associated Component: Textarea + +See: `src/components/textarea/textarea.ts` + +**Characteristics**: + +- Uses `FormAssociatedRequiredMixin` +- Uses `EventEmitterMixin` for events +- `@shadowOptions({ delegatesFocus: true })` +- Form validation support +- Multiple slots and parts + +### Container Component: Card + +See: `src/components/card/card.ts` + +**Characteristics**: + +- Registers multiple sub-components +- Simple container logic +- Composition-based architecture + +## Architecture Notes + +### Key Principles + +- **Shadow DOM**: All components use Shadow DOM for encapsulation +- **Composition over Inheritance**: Use slots and sub-components +- **TypeScript Strict Mode**: Avoid `any` types +- **No Native Private Fields**: Use `_prefix` or TypeScript `private`, not `#` +- **Accessibility First**: Every component must pass a11y audits + +### Build System + +- SCSS transpiles to Lit `css` template literals +- Build script handles transpilation automatically +- Style watching: `npm run styles:watch` + +### Testing + +- Framework: `@open-wc/testing` +- Accessibility tests are mandatory +- Test both Light and Shadow DOM + +### Resources + +- [Lit Documentation](https://lit.dev/docs/) +- [Coding Guidelines](../../CODING_GUIDELINES.md) - Comprehensive coding standards +- [igniteui-theming Package](https://www.npmjs.com/package/igniteui-theming) diff --git a/.github/skills/review-component-pr/SKILL.md b/.github/skills/review-component-pr/SKILL.md new file mode 100644 index 000000000..f69e03846 --- /dev/null +++ b/.github/skills/review-component-pr/SKILL.md @@ -0,0 +1,476 @@ +--- +name: review-component-pr +description: Comprehensive code review checklist for component pull requests ensuring quality, accessibility, and adherence to project conventions +--- + +# Review Component PR + +This skill provides a systematic checklist for reviewing pull requests that add or modify web components in the project. + +## Example Usage + +- "Review the new tooltip component PR" +- "Check if the badge component changes follow our conventions" +- "Perform a code review on PR #123" + +## Related Skills + +- [create-new-component](../create-new-component/) - Reference for new component structure +- [add-component-property](../add-component-property/) - Property implementation patterns + +## When to Use + +- Reviewing a PR that adds a new component +- Reviewing changes to an existing component +- Pre-merge quality gate check +- Onboarding new contributors + +## Review Checklist + +### 1. Project Structure & Files + +- [ ] **Component directory** exists at `src/components/[component-name]/` +- [ ] **Component file** named correctly: `[component-name].ts` +- [ ] **Test file** exists: `[component-name].spec.ts` +- [ ] **Storybook story** exists: `stories/[component-name].stories.ts` +- [ ] **Theme files** present in `themes/` directory + - `[component-name].base.scss` + - `themes.ts` aggregator + - All four theme files (bootstrap, material, fluent, indigo) +- [ ] **Export** added to `src/index.ts` in alphabetical order + +### 2. TypeScript Quality + +- [ ] **No `any` types** - Use `unknown` or proper types +- [ ] **Strict type checking** passes +- [ ] **Import paths** use `.js` extension (not `.ts`) +- [ ] **No native private fields** - Use `_prefix` or TypeScript `private`, not `#` +- [ ] **Internal API prefixed** with underscore: `_method()`, `_property` +- [ ] **readonly modifier** used for immutable properties +- [ ] **Explicit return types** on methods (unless obvious/cluttering) +- [ ] **Property decorators** used correctly (`@property`, `@query`, etc.) +- [ ] **Type annotations** present on all public APIs +- [ ] **Interfaces/types** defined for complex structures + +**Good**: + +```typescript +@property({ reflect: true }) +public variant: StyleVariant = 'primary'; + +private _internalState: string = ''; +``` + +**Bad**: + +```typescript +@property() +public variant: any; // ❌ No any types + +#privateField = ''; // ❌ No native private fields +``` + +### 3. Component Class Structure + +- [ ] **Extends `LitElement`** or appropriate mixin +- [ ] **`tagName` static property** defined: `'igc-[component-name]'` with `readonly` modifier +- [ ] **`styles` static property** includes base and shared styles +- [ ] **`register()` static method** present +- [ ] **Region comments** organize code: Internal state, Public properties, Lit lifecycle, Event handlers, Internal API, Public API +- [ ] **Internal API prefixed** with underscore: `_internalMethod()`, `_privateState` +- [ ] **readonly modifier** used for immutable properties +- [ ] **Explicit return types** specified for methods (unless obvious) +- [ ] **Theming controller** added in constructor: `addThemingController(this, all)` +- [ ] **Constructor** calls `super()` first if overridden +- [ ] **Default export** used: `export default class` +- [ ] **Global declaration** included for TypeScript + +**Template**: + +```typescript +export default class IgcComponentComponent extends LitElement { + public static readonly tagName = 'igc-component'; + public static override styles = [styles, shared]; + + public static register(): void { + registerComponent(IgcComponentComponent); + } + + //#region Internal state + + private _internalState = ''; + + //#endregion + + //#region Public properties + + @property({ reflect: true }) + public variant = 'primary'; + + //#endregion + + constructor() { + super(); + addThemingController(this, all); + } + + //#region Lit lifecycle + + protected override render() { + return html`
`; + } + + //#endregion + + //#region Internal API + + private _handleChange(): void { + // Internal helper + } + + //#endregion +} + +declare global { + interface HTMLElementTagNameMap { + 'igc-component': IgcComponentComponent; + } +} +``` + +### 4. Properties & Attributes + +- [ ] **Only primitives exposed as attributes** (string, number, boolean) +- [ ] **Complex types** use `attribute: false` +- [ ] **Boolean properties** use `{ type: Boolean, reflect: true }` +- [ ] **Number properties** use `{ type: Number }` +- [ ] **reflect: true** only for primitives that should be attributes +- [ ] **Default values** are appropriate +- [ ] **JSDoc comments** include `@attr` for reflected properties +- [ ] **JSDoc comments** include `@default` for default values + +**Good**: + +```typescript +/** + * The variant style of the component + * @attr variant + * @default 'primary' + */ +@property({ reflect: true }) +public variant: StyleVariant = 'primary'; + +/** + * Complex configuration object + */ +@property({ attribute: false }) +public config: Config = {}; +``` + +**Bad**: + +```typescript +@property({ reflect: true }) // ❌ Trying to reflect complex type +public config: Config = {}; +``` + +### 5. Lifecycle & Reactivity + +- [ ] **Lifecycle hooks** used correctly + - `update()` for side effects (DOM available) + - `willUpdate()` only for pre-render computation + - `super.update()` called when overriding `update()` +- [ ] **`changedProperties.has()`** checked to avoid unnecessary work +- [ ] **No `@watch` decorator** used (prefer lifecycle hooks) +- [ ] **`requestUpdate()`** not called unnecessarily + +**Good**: + +```typescript +protected override update(changedProperties: PropertyValues): void { + if (changedProperties.has('disabled')) { + // Update ARIA or other side effects + this._internals.setARIA({ ariaDisabled: `${this.disabled}` }); + } + super.update(changedProperties); +} +``` + +### 6. Shadow DOM & Styling + +- [ ] **Shadow DOM** used (not open mode issues) +- [ ] **CSS parts** exposed with `part` attribute for external styling +- [ ] **CSS parts documented** in JSDoc with `@csspart` +- [ ] **CSS custom properties** documented with `@cssproperty` +- [ ] **SCSS files transpiled** to `.css.js` files +- [ ] **No direct `.css.js` edits** (only edit SCSS) +- [ ] **All themes implemented** (bootstrap, material, fluent, indigo) +- [ ] **Light and dark modes** considered +- [ ] **`:host` styles** include appropriate `display` value + +**Good**: + +```typescript +/** + * @csspart base - The main container + * @csspart content - The content wrapper + * @cssproperty --padding - Internal padding + */ +protected override render() { + return html`
`; +} +``` + +### 7. Accessibility (Critical) + +- [ ] **Accessibility tests pass** - `await expect(el).to.be.accessible()` +- [ ] **Semantic HTML** used where appropriate +- [ ] **ARIA attributes** set correctly +- [ ] **`role` attribute** appropriate for component type +- [ ] **Keyboard navigation** implemented +- [ ] **Focus management** works correctly +- [ ] **`delegatesFocus`** used if component should delegate focus +- [ ] **Screen reader** announcements appropriate +- [ ] **Color contrast** meets WCAG standards +- [ ] **Focus indicators** visible +- [ ] **`addInternalsController`** used for ARIA management if needed + +**Check**: + +```typescript +// Should have accessibility test +it('passes the a11y audit', async () => { + const el = await fixture( + html`` + ); + + await expect(el).shadowDom.to.be.accessible(); + await expect(el).to.be.accessible(); +}); +``` + +### 8. Events + +- [ ] **Events use EventEmitterMixin** if custom events needed +- [ ] **Event map interface** defined +- [ ] **Events documented** with `@event` in JSDoc +- [ ] **Event names** follow convention (lowercase, no `on` prefix) +- [ ] **Events composed** if they should cross Shadow DOM boundary +- [ ] **Events bubbling** configured appropriately +- [ ] **Event detail types** are properly typed + +**Good**: + +```typescript +export interface IgcComponentEventMap { + igcChange: CustomEvent; +} + +/** + * @event igcChange - Emitted when value changes + */ +export default class IgcComponentComponent extends EventEmitterMixin< + IgcComponentEventMap, + Constructor +>(LitElement) { + // ... + this.emitEvent('igcChange', { detail: newValue }); +} +``` + +### 9. Tests + +- [ ] **Accessibility test** included (required) +- [ ] **Default values tested** +- [ ] **Property changes tested** +- [ ] **Attribute reflection tested** (for reflected properties) +- [ ] **Events tested** +- [ ] **Slot content tested** +- [ ] **Edge cases covered** +- [ ] **`defineComponents()` called** in `before()` hook +- [ ] **`elementUpdated()` used** after programmatic changes +- [ ] **Tests use `@open-wc/testing`** framework +- [ ] **All tests pass** locally + +**Minimum Required**: + +```typescript +describe('Component', () => { + before(() => { + defineComponents(IgcComponentComponent); + }); + + it('passes the a11y audit', async () => { + // Required accessibility test + }); + + it('should initialize with default values', async () => { + // Test defaults + }); + + it('can change properties', async () => { + // Test reactivity + }); +}); +``` + +### 10. Storybook Stories + +- [ ] **Story file exists** at `stories/[component-name].stories.ts` +- [ ] **Metadata defined** with title, component, description +- [ ] **argTypes** match component properties +- [ ] **args** provide default values +- [ ] **Controls** appropriate for property types +- [ ] **TypeScript interface** for args +- [ ] **At least one story** demonstrates basic usage +- [ ] **Multiple stories** for different variants (ideal) +- [ ] **Story renders correctly** in Storybook + +### 11. Documentation + +- [ ] **JSDoc comments** on component class +- [ ] **`@element` tag** with component tag name +- [ ] **`@slot` tags** for all slots (default and named) +- [ ] **`@csspart` tags** for all exposed parts +- [ ] **`@cssproperty` tags** for CSS custom properties +- [ ] **`@event` tags** for emitted events +- [ ] **Property descriptions** clear and complete +- [ ] **Examples** in JSDoc if complex usage + +**Complete Example**: + +```typescript +/** + * A button component for user interactions. + * + * @element igc-button + * + * @slot - Default slot for button text + * @slot prefix - Content before the button text + * + * @csspart base - The button element + * @csspart content - The content wrapper + * + * @cssproperty --padding - Button padding + * + * @event igcClick - Emitted on click + */ +``` + +### 12. Form Integration (if applicable) + +- [ ] **FormAssociatedRequiredMixin** used for form controls +- [ ] **Form value state** managed correctly +- [ ] **Validation** implemented if needed +- [ ] **`value` property** reactive and synced +- [ ] **Form reset** handled +- [ ] **Required/disabled states** work correctly +- [ ] **Labels associated** correctly + +### 13. Build & Compilation + +- [ ] **TypeScript compiles** without errors: `npm run check-types` +- [ ] **No linting errors**: Check biome/eslint output +- [ ] **SCSS transpiled** to `.css.js` files +- [ ] **All tests pass**: `npm run test` +- [ ] **No console errors/warnings** in Storybook +- [ ] **Bundle size** reasonable (check for large dependencies) + +### 14. Code Quality + +- [ ] **No commented-out code** (unless with explanation) +- [ ] **No debug statements** (`console.log`, `debugger`) +- [ ] **Consistent formatting** (should be auto-formatted) +- [ ] **Meaningful variable names** +- [ ] **No magic numbers** (use named constants) +- [ ] **Error handling** appropriate +- [ ] **Code is DRY** (Don't Repeat Yourself) + +### 15. Project Conventions + +- [ ] **Follows composition over inheritance** +- [ ] **No heavy third-party dependencies** unless necessary +- [ ] **Uses project utilities** from `common/` where applicable +- [ ] **Consistent with existing component patterns** +- [ ] **Theming uses igniteui-theming package** +- [ ] **Modern ECMAScript features** used appropriately + +## Common Issues to Watch For + +### Issue: Missing Theming Controller + +**Problem**: Component doesn't respond to theme changes +**Fix**: Add `addThemingController(this, all)` in constructor + +### Issue: Wrong Import Extensions + +**Problem**: Using `.ts` instead of `.js` in imports +**Fix**: All TypeScript imports must use `.js` extension + +### Issue: Exposing Complex Types as Attributes + +**Problem**: Trying to reflect objects/arrays to HTML attributes +**Fix**: Use `attribute: false` for non-primitive types + +### Issue: Missing Accessibility Tests + +**Problem**: No a11y test in spec file +**Fix**: Always include accessibility audit test + +### Issue: Forgot to Export Component + +**Problem**: Component not available when importing from package +**Fix**: Add export to `src/index.ts` in alphabetical order + +### Issue: Using @watch Decorator + +**Problem**: Using deprecated pattern for property changes +**Fix**: Use `update()` or `willUpdate()` lifecycle hooks + +### Issue: Native Private Fields + +**Problem**: Using `#privateField` syntax +**Fix**: Use `_privateField` or TypeScript `private` keyword + +## Review Process + +1. **Start with structure** - Verify all required files exist +2. **Check TypeScript** - Ensure strict typing and no `any` +3. **Review accessibility** - This is critical, don't skip +4. **Test locally** - Run build and tests +5. **Check Storybook** - Verify visual behavior +6. **Review code quality** - Look for patterns and conventions +7. **Provide constructive feedback** - Be specific and helpful + +## Quick Pass/Fail Criteria + +**Immediate Reject if**: + +- No accessibility tests +- Using `any` types extensively +- Native private fields (`#`) +- No tests at all +- TypeScript compilation errors + +**Request Changes if**: + +- Missing documentation +- Incomplete test coverage +- Accessibility issues +- Not following project conventions +- Missing theme files + +**Approve if**: + +- All checkboxes checked +- Tests pass +- Accessibility verified +- Code quality good +- Follows project patterns + +## Resources + +- [Coding Guidelines](../../CODING_GUIDELINES.md) - Comprehensive coding standards and best practices +- [Lit Best Practices](https://lit.dev/docs/components/best-practices/) +- [WCAG Guidelines](https://www.w3.org/WAI/WCAG21/quickref/) +- [Web Components Best Practices](https://web.dev/custom-elements-best-practices/) diff --git a/.github/skills/update-component-styles/SKILL.md b/.github/skills/update-component-styles/SKILL.md new file mode 100644 index 000000000..d4b2b7d1d --- /dev/null +++ b/.github/skills/update-component-styles/SKILL.md @@ -0,0 +1,305 @@ +--- +name: update-component-styles +description: Update component styling following the SCSS to Lit CSS workflow with proper theme support +--- + +# Update Component Styles + +This skill guides you through updating component styles following the project's SCSS workflow and theming system. + +## Example Usage + +- "Update the badge component border radius" +- "Add hover styles to the button component" +- "Change the card component padding" + +## Related Skills + +- [create-new-component](../create-new-component/) - Styling for new components + +## When to Use + +- User asks to modify component appearance +- Need to add or change CSS styles +- Updating theme-specific styles +- Adding new CSS custom properties + +## Prerequisites + +- [ ] Target component exists +- [ ] Style change requirements are clear +- [ ] Understand which theme files to modify + +## Required Context + +Gather or confirm: + +- **Component name**: Which component to style +- **Style changes**: What CSS needs to change +- **Scope**: Base styles, theme-specific, or both +- **CSS parts**: Whether to add/modify exposed parts + +## Steps + +### 1. Identify Style Files to Modify + +Component styles are organized as: + +- `[component]/themes/[component].base.scss` - Base structural styles +- `[component]/themes/shared/[component].common.css.js` - Common cross-theme styles (if exists) +- `[component]/themes/shared/[component].bootstrap.scss` - Bootstrap theme +- `[component]/themes/shared/[component].material.scss` - Material theme +- `[component]/themes/shared/[component].fluent.scss` - Fluent theme +- `[component]/themes/shared/[component].indigo.scss` - Indigo theme +- `[component]/themes/light/[component].[theme].scss` - Light theme overrides +- `[component]/themes/dark/[component].[theme].scss` - Dark theme overrides + +**Decision tree**: + +- Structural changes (layout, display) → Edit base.scss +- Theme-agnostic styles → Edit shared/\*.scss +- Theme-specific styles → Edit specific theme file +- Light/dark variants → Edit light/_ or dark/_ files + +### 2. Edit SCSS Files + +**Base styles** (`themes/[component].base.scss`): + +```scss +@use '../../../styles/utilities' as *; + +:host { + display: block; +} + +[part='base'] { + padding: 1rem; + border-radius: 4px; + // Structural styles here +} +``` + +**Theme-specific styles** (`themes/shared/[component].bootstrap.scss`): + +```scss +@use '../../../../theming/functions' as *; + +:host { + // Use theming functions from igniteui-theming + --component-color: #{color('primary', 500)}; +} + +[part='base'] { + background: var(--component-color); +} +``` + +**Light/Dark overrides** (`themes/light/[component].bootstrap.scss`): + +```scss +:host { + --component-color: white; +} +``` + +### 3. Use CSS Parts for External Styling + +If adding new styleable parts, update component render and JSDoc: + +**Update render method**: + +```typescript +protected override render() { + return html` +
+ Content +
+ `; +} +``` + +**Update JSDoc**: + +```typescript +/** + * ... + * @csspart base - The main container + * @csspart new-part - Description of new part + * @csspart content - The content wrapper + */ +export default class IgcComponentComponent extends LitElement { +``` + +### 4. Add CSS Custom Properties (if needed) + +For values that should be customizable: + +**In SCSS**: + +```scss +:host { + --component-padding: 1rem; + --component-radius: 4px; +} + +[part='base'] { + padding: var(--component-padding); + border-radius: var(--component-radius); +} +``` + +**Document in JSDoc**: + +```typescript +/** + * ... + * @cssproperty --component-padding - The internal padding + * @cssproperty --component-radius - The border radius + */ +export default class IgcComponentComponent extends LitElement { +``` + +### 5. Transpile SCSS to TypeScript + +After editing SCSS files, transpile them: + +```bash +npm run build:styles +``` + +This converts `.scss` to `.css.js` files with Lit's `css` template literal. + +**Verify**: Check that `.css.js` files are updated with your changes. + +### 6. Test Style Changes + +**Manual testing in Storybook**: + +```bash +npm run storybook +``` + +Check: + +- [ ] Styles render correctly in all theme variants +- [ ] Light and dark modes both work +- [ ] CSS parts are styleable from outside +- [ ] CSS custom properties work + +**Automated testing** (if style affects behavior): + +```typescript +it('should apply correct CSS class', async () => { + const el = await fixture( + html`` + ); + + const part = el.shadowRoot?.querySelector('[part="base"]'); + // Test computed styles or classes if critical +}); +``` + +### 7. Update Documentation (if needed) + +If adding new CSS parts or custom properties: + +1. JSDoc comments are automatically extracted +2. Verify in Storybook's "Docs" tab +3. Check that parts/properties appear in documentation + +## Validation Checklist + +- [ ] SCSS files edited correctly +- [ ] Styles transpiled (`npm run build:styles`) +- [ ] No TypeScript compilation errors +- [ ] Styles render correctly in Storybook +- [ ] All theme variants checked (bootstrap, material, fluent, indigo) +- [ ] Light and dark modes both work +- [ ] New CSS parts documented in JSDoc +- [ ] New CSS custom properties documented +- [ ] No visual regressions on other components + +## Common Pitfalls + +### 1. Forgetting to Transpile + +**Problem**: Style changes don't appear +**Solution**: Always run `npm run build:styles` after editing SCSS + +### 2. Editing .css.js Files Directly + +**Problem**: Changes get overwritten on next build +**Solution**: Only edit `.scss` files, never `.css.js` files + +### 3. Wrong Theme File + +**Problem**: Styles appear in some themes but not others +**Solution**: Edit the correct theme file or shared file for all themes + +### 4. Not Using Theming Functions + +**Problem**: Hardcoded colors don't match theme +**Solution**: Use `color()`, `theme()` functions from igniteui-theming + +### 5. Overly Specific Selectors + +**Problem**: Users can't override styles +**Solution**: Keep selector specificity low, expose CSS parts + +### 6. Missing :host Styles + +**Problem**: Component doesn't display correctly +**Solution**: Always set `:host { display: block; }` or appropriate display value + +## Reference Examples + +### Simple Component Styles: Badge + +See: `src/components/badge/themes/` + +**Structure**: + +- Base styles for structure +- Theme files for colors/variants +- Uses CSS custom properties +- Exposes `base` and `icon` parts + +### Complex Component Styles: Textarea + +See: `src/components/textarea/themes/` + +**Structure**: + +- More complex base styles +- Multiple CSS parts +- Theme-specific variants +- Light/dark overrides + +## Theming System + +### Available Functions + +From igniteui-theming package: + +- `color($palette, $variant)` - Get theme color +- `contrast-color($color)` - Get contrasting text color +- `theme($property)` - Get theme value + +### Theme Structure + +```scss +@use '../../../../theming/functions' as *; + +:host { + --bg: #{color('surface', 500)}; + --text: #{contrast-color(color('surface', 500))}; +} +``` + +## Resources + +- [Lit Styles](https://lit.dev/docs/components/styles/) +- [Shadow Parts](https://developer.mozilla.org/en-US/docs/Web/CSS/::part) +- [CSS Custom Properties](https://developer.mozilla.org/en-US/docs/Web/CSS/--*) +- [igniteui-theming Package](https://www.npmjs.com/package/igniteui-theming) +- [Coding Guidelines](../../CODING_GUIDELINES.md) - Comprehensive coding standards