Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 19 additions & 1 deletion docs/guides/migration.en.md
Original file line number Diff line number Diff line change
Expand Up @@ -1035,7 +1035,7 @@ for each option it deselected and reporting the shortened value to the form cont

### 18. Component review (20.3.0)

Ten components went through a full review in 20.3.0: notification-center, popover, search-expandable, select, split-button, title, toast, tooltip, tree and tree-select. Each review closed the members that were never part of the component's contract, moved inputs to signals where that was the point of it, and fixed the behavior it uncovered along the way. Only the changes that reach a consumer are listed here.
Components went through a full review in 20.3.0, in two waves. The first covered notification-center, popover, search-expandable, select, split-button, title, toast, tooltip, tree and tree-select; the second is the one each subsection below belongs to. Each review closed the members that were never part of the component's contract, moved inputs to signals where that was the point of it, and fixed the behavior it uncovered along the way. Only the changes that reach a consumer are listed here.

Every schematic named below runs automatically:

Expand All @@ -1049,6 +1049,24 @@ Most of them report rather than rewrite: what replaces a removed member or a sig
ng g @koobiq/components:<schematic-name> --project <your project>
```

#### Description list

`KbqDlComponent` was already fully signal-based, so the review had nothing to migrate. What it found were the five inputs that never got a coercion transform, sitting next to siblings that had one:

```ts
readonly verticalBreakpoint = input(400, { transform: numberAttribute });
readonly minWidth = input<number | undefined>(); // no transform
readonly wide = input(false); // no transform
```

**`<kbq-dl wide>` and `<kbq-dl vertical>` used to do nothing.** A valueless attribute passes the empty string, which is falsy — while `<kbq-dl resizable>` right next to it worked. Both are coerced now and the attribute means true.

`vertical` is tri-state: `null` means "decide from `verticalBreakpoint`". `booleanAttribute` would have folded that into `false`, so it uses a transform that preserves `null`.

**`minWidth`, `dtMinWidth` and `ddMinWidth` are numeric inputs** reporting `number | undefined`, which is what an unbound description list always held. A static attribute used to reach the layout arithmetic as a string, which coerced in a comparison but not in `Math.max`.

Reported by `dl-attribute-coercion`.

#### Popover

Hover mode was broken end to end by a dead expression. `this.leaveDelay ?? 500` looks like a default, but the base class sets the field to `0`, and `0 ?? 500` is `0` — so the panel closed before the pointer could cross the 8px gap to it, the documented interactive content was unreachable even for pointer users, and the auto-hide watchdog spun as an `interval(0)` for as long as the panel stayed open.
Expand Down
20 changes: 19 additions & 1 deletion docs/guides/migration.ru.md
Original file line number Diff line number Diff line change
Expand Up @@ -1039,7 +1039,7 @@ ng g @koobiq/components:list-tree-multiple-input --project <your project>

### 18. Ревью компонентов (20.3.0)

В 20.3.0 полное ревью прошли десять компонентов: notification-center, popover, search-expandable, select, split-button, title, toast, tooltip, tree и tree-select. Каждое ревью закрывало члены, которые никогда не были частью контракта компонента, переводило входы на сигналы там, где в этом и был его смысл, и попутно исправляло найденные ошибки поведения. Ниже перечислено только то, что доходит до потребителя.
В 20.3.0 полное ревью прошло в две волны. В первую вошли notification-center, popover, search-expandable, select, split-button, title, toast, tooltip, tree и tree-select; ко второй относится каждый из подразделов ниже. Каждое ревью закрывало члены, которые никогда не были частью контракта компонента, переводило входы на сигналы там, где в этом и был его смысл, и попутно исправляло найденные ошибки поведения. Ниже перечислено только то, что доходит до потребителя.

Все схематики, названные ниже, запускаются автоматически:

Expand All @@ -1053,6 +1053,24 @@ ng update @koobiq/components@20
ng g @koobiq/components:<schematic-name> --project <your project>
```

#### Description list

`KbqDlComponent` уже был полностью на сигналах, поэтому мигрировать в нём было нечего. Ревью нашло другое: пять входов, которым так и не досталось приведение типов, — рядом с соседями, у которых оно есть:

```ts
readonly verticalBreakpoint = input(400, { transform: numberAttribute });
readonly minWidth = input<number | undefined>(); // без трансформации
readonly wide = input(false); // без трансформации
```

**`<kbq-dl wide>` и `<kbq-dl vertical>` раньше ничего не делали.** Атрибут без значения передаёт пустую строку, которая ложна, — при том что `<kbq-dl resizable>` по соседству работал. Теперь оба приводятся, и атрибут означает `true`.

`vertical` трёхзначный: `null` означает «решай по `verticalBreakpoint`». `booleanAttribute` свернул бы это в `false`, поэтому используется трансформация, сохраняющая `null`.

**`minWidth`, `dtMinWidth` и `ddMinWidth` стали числовыми входами** и возвращают `number | undefined` — именно это и содержал список без привязок. Статический атрибут раньше доходил до арифметики раскладки строкой, которая приводилась к числу в сравнении, но не в `Math.max`.

Сообщается схематиком `dl-attribute-coercion`.

#### Popover

Режим по наведению был сломан целиком из-за мёртвого выражения. `this.leaveDelay ?? 500` выглядит как значение по умолчанию, но базовый класс присваивает полю `0`, а `0 ?? 500` — это `0`. Панель закрывалась раньше, чем указатель успевал пересечь зазор в 8px до неё, задокументированное интерактивное содержимое было недостижимо даже для мыши, а таймер автоматического скрытия крутился как `interval(0)` всё время, пока панель открыта.
Expand Down
123 changes: 122 additions & 1 deletion packages/components/dl/dl.component.spec.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
import { FocusMonitor } from '@angular/cdk/a11y';
import { Directionality } from '@angular/cdk/bidi';
import { SharedResizeObserver } from '@angular/cdk/observers/private';
import { Injectable, Provider, Type } from '@angular/core';
import { Component, Injectable, Provider, Type } from '@angular/core';
import { ComponentFixture, fakeAsync, flush, TestBed, tick } from '@angular/core/testing';
import { By } from '@angular/platform-browser';
import { enUSLocaleData, kbqA11yLocaleConfigurationProvider } from '@koobiq/components/core';
import { BehaviorSubject, EMPTY, Observable } from 'rxjs';
import { KbqDlAlign, KbqDlComponent } from './dl.component';
import { KbqDlModule } from './dl.module';

/** `SharedResizeObserver` stand-in: the real one never emits in jsdom, where `ResizeObserver` is a no-op stub. */
@Injectable()
Expand Down Expand Up @@ -626,4 +628,123 @@ describe(KbqDlComponent.name, () => {
expect(getDlElement(fixture).classList).toContain('kbq-dl_vertical');
flush();
}));
it('should re-evaluate the layout when the breakpoint changes', fakeAsync(() => {
const fixture = createComponent(KbqDlComponent);

Object.defineProperty(getDlElement(fixture), 'getClientRects', {
configurable: true,
value: () => [{ width: 600 } as DOMRect]
});

tick(100);
fixture.detectChanges();

expect(getDlElement(fixture).classList).not.toContain('kbq-dl_vertical');

// The comparison used to run only on resize, so a new breakpoint left the old answer in place.
fixture.componentRef.setInput('verticalBreakpoint', 700);
fixture.detectChanges();

expect(getDlElement(fixture).classList).toContain('kbq-dl_vertical');
flush();
}));

it('should hand the decision back to the breakpoint when vertical returns to null', fakeAsync(() => {
const fixture = createComponent(KbqDlComponent);

Object.defineProperty(getDlElement(fixture), 'getClientRects', {
configurable: true,
value: () => [{ width: 600 } as DOMRect]
});

fixture.componentRef.setInput('verticalBreakpoint', 700);
tick(100);
fixture.detectChanges();

expect(getDlElement(fixture).classList).toContain('kbq-dl_vertical');

fixture.componentRef.setInput('vertical', false);
fixture.detectChanges();

expect(getDlElement(fixture).classList).not.toContain('kbq-dl_vertical');

fixture.componentRef.setInput('vertical', null);
fixture.detectChanges();

expect(getDlElement(fixture).classList).toContain('kbq-dl_vertical');
flush();
}));

it('should treat a valueless wide attribute as true', () => {
const fixture = createComponent(DlWithValuelessAttributes);

fixture.detectChanges();

const dl = fixture.debugElement.query(By.directive(KbqDlComponent)).nativeElement as HTMLElement;

expect(dl.classList).toContain('kbq-dl_wide');
});

it('should keep null as the vertical default', () => {
const fixture = createComponent(KbqDlComponent);

fixture.detectChanges();

// `null` is the "decide from the breakpoint" state, so it must survive the transform.
expect(fixture.componentInstance.vertical()).toBeNull();
});

it('should treat a valueless vertical attribute as true', () => {
const fixture = createComponent(DlWithValuelessAttributes);

fixture.detectChanges();

const dl = fixture.debugElement.query(By.directive(KbqDlComponent)).nativeElement as HTMLElement;

expect(dl.classList).toContain('kbq-dl_vertical');
});

it('should coerce the numeric width attributes', () => {
const fixture = createComponent(DlWithStringWidths);

fixture.detectChanges();

const dl = fixture.debugElement.query(By.directive(KbqDlComponent)).componentInstance as KbqDlComponent;

expect(dl.minWidth()).toBe(700);
expect(dl.dtMinWidth()).toBe(120);
expect(dl.ddMinWidth()).toBe(80);
});

it('should leave the optional widths undefined when unbound', () => {
const fixture = createComponent(KbqDlComponent);

fixture.detectChanges();

expect(fixture.componentInstance.minWidth()).toBeUndefined();
expect(fixture.componentInstance.dtMinWidth()).toBeUndefined();
expect(fixture.componentInstance.ddMinWidth()).toBeUndefined();
});
});

@Component({
imports: [KbqDlModule],
template: `
<kbq-dl wide vertical>
<kbq-dt>term</kbq-dt>
<kbq-dd>description</kbq-dd>
</kbq-dl>
`
})
class DlWithValuelessAttributes {}

@Component({
imports: [KbqDlModule],
template: `
<kbq-dl minWidth="700" dtMinWidth="120" ddMinWidth="80">
<kbq-dt>term</kbq-dt>
<kbq-dd>description</kbq-dd>
</kbq-dl>
`
})
class DlWithStringWidths {}
47 changes: 41 additions & 6 deletions packages/components/dl/dl.component.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,15 @@ import {
computed,
contentChildren,
DestroyRef,
effect,
ElementRef,
inject,
Injector,
input,
model,
numberAttribute,
signal,
untracked,
viewChild,
ViewEncapsulation
} from '@angular/core';
Expand All @@ -25,6 +28,10 @@ import { KBQ_WINDOW, kbqInjectA11yLocaleConfiguration, kbqInjectNativeElement }
import { KbqResizable, KbqResizer, KbqResizerDirection, KbqResizerSizeChangeEvent } from '@koobiq/components/resizer';
import { debounceTime, startWith } from 'rxjs/operators';

/** Coerces an optional numeric input, keeping `undefined` distinguishable from `0`. */
const optionalNumberAttribute = (value: unknown): number | undefined =>
value == null ? undefined : numberAttribute(value);

/** Supported alignment values for description list items. */
export type KbqDlAlign = 'start' | 'center' | 'end';

Expand Down Expand Up @@ -110,10 +117,10 @@ export class KbqDlComponent {
* @deprecated The name is misleading (it is a breakpoint, not a min width). Use `verticalBreakpoint` instead.
* Will be removed in a future major release. When both are set, `minWidth` takes precedence.
*/
readonly minWidth = input<number | undefined>();
readonly minWidth = input<number | undefined, unknown>(undefined, { transform: optionalNumberAttribute });

/** Whether the list uses the wide two-column layout. */
readonly wide = input(false);
readonly wide = input(false, { transform: booleanAttribute });

/** Whether the `kbq-dt` area can be resized by dragging the separator. */
readonly resizable = input(false, { transform: booleanAttribute });
Expand All @@ -122,10 +129,10 @@ export class KbqDlComponent {
readonly dtWidth = model<number | null>(null);

/** Minimum width of the `kbq-dt` area in pixels; defaults to the rendered term width. */
readonly dtMinWidth = input<number | undefined>(undefined);
readonly dtMinWidth = input<number | undefined, unknown>(undefined, { transform: optionalNumberAttribute });

/** Minimum width retained for the `kbq-dd` area in pixels; defaults to the rendered term width. */
readonly ddMinWidth = input<number | undefined>(undefined);
readonly ddMinWidth = input<number | undefined, unknown>(undefined, { transform: optionalNumberAttribute });

/** Accessible name of the column resize separator; falls back to the localized default when omitted. */
readonly resizerAriaLabel = input<string | undefined>(undefined);
Expand All @@ -137,7 +144,10 @@ export class KbqDlComponent {
readonly horizontalAlign = input<KbqDlAlign>('start');

/** Forces the vertical layout; `null` lets the list decide based on `verticalBreakpoint`. */
readonly vertical = input<boolean | null>(null);
readonly vertical = input<boolean | null, unknown>(null, {
// Not `booleanAttribute`: it would fold `null` — the "decide for me" state — into `false`.
transform: (value) => (value == null ? null : booleanAttribute(value))
});

/** @docs-private */
protected readonly resizeDirection = signal<KbqResizerDirection>([1, 0]);
Expand Down Expand Up @@ -194,6 +204,7 @@ export class KbqDlComponent {
private readonly platform = inject(Platform);
private readonly window = inject(KBQ_WINDOW);
private readonly destroyRef = inject(DestroyRef);
private readonly injector = inject(Injector);
private readonly resizeObserver = inject(SharedResizeObserver);
private readonly directionality = inject(Directionality, { optional: true });
private readonly focusMonitor = inject(FocusMonitor);
Expand All @@ -217,10 +228,34 @@ export class KbqDlComponent {
afterNextRender(() => {
this.measureDtWidth();

// The resize subscription owns the first measurement: it is debounced, so the host has been
// laid out by the time it runs. The effect below defers to it rather than measuring a host
// that has no box yet.
let layoutMeasured = false;

this.resizeObserver
.observe(this.nativeElement)
.pipe(startWith(null), debounceTime(this.resizeDebounceInterval), takeUntilDestroyed(this.destroyRef))
.subscribe(() => this.updateLayout());
.subscribe(() => {
layoutMeasured = true;
this.updateLayout();
});

// The breakpoint comparison was only ever re-run on resize, so changing any of its three
// inputs left the layout on the answer computed for the previous ones — including `vertical`
// going back to null, which hands the decision back to a stale `autoVertical`.
effect(
() => {
this.vertical();
this.verticalBreakpoint();
this.minWidth();

if (!layoutMeasured) return;

untracked(() => this.updateLayout());
},
{ injector: this.injector }
);
});
}

Expand Down
5 changes: 5 additions & 0 deletions packages/schematics/src/collection.json
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,11 @@
"description": "Reports the breaking parts of the toast review — the members KbqToastComponent lost when it moved from KbqToastService to the KBQ_TOAST_STACK token, the narrowed animation subject and showTemplate context — and notes that the data passed to show() is no longer written to, so its style and icon defaults are resolved inside the toast",
"factory": "./migrations/toast-stack-and-defaults/index",
"schema": "./migrations/toast-stack-and-defaults/schema.json"
},
"dl-attribute-coercion": {
"description": "Reports the <kbq-dl> attributes whose coercion changed in the description list review. KbqDlComponent was already fully signal-based; five inputs never got a transform, so a static attribute reached them as a string. A valueless wide or vertical attribute passed the empty string, which is falsy, so it did nothing - both are coerced now and mean true. minWidth, dtMinWidth and ddMinWidth are numeric inputs reporting number | undefined. vertical keeps null as its default, the state that lets the list decide from verticalBreakpoint, so it uses a transform that preserves null rather than booleanAttribute. Warn-only: every one of these is a template change the call site owns.",
"factory": "./migrations/dl-attribute-coercion/index",
"schema": "./migrations/dl-attribute-coercion/schema.json"
}
}
}
Loading
Loading