-
Notifications
You must be signed in to change notification settings - Fork 4
fix(textarea)!: errors following a full review of the component (#DS-5482) #1990
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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: | ||
|
|
||
|
|
@@ -1113,6 +1113,28 @@ A `<kbq-split-button>` with no projected button no longer throws outside dev mod | |
|
|
||
| Reported by `split-button-optional-disabled`. | ||
|
|
||
| #### Textarea | ||
|
|
||
| `KbqTextarea` implements `KbqFormFieldControl`, which declares `value`, `id`, `placeholder`, `required`, `disabled`, `focused`, `empty` and `errorState` as plain members — that interface is how the form field reads them, so they stay plain accessors. What moved are the four inputs the textarea owns. | ||
|
|
||
| `canGrow` was the odd one: its getter returned `!maxRowLimitReached && bound`, so it reported `false` once the textarea hit `maxRows` even though the consumer had asked for growth. The folded value drives the resize handle and is internal now; `canGrow()` reports what was bound. | ||
|
|
||
| | Pattern | Manual migration | | ||
| | ------------------------------------------------------- | --------------------------------------------------------------------- | | ||
| | `.maxRows` / `.freeRowsHeight` / `.maxRowLimitReached` | Read as calls — rewritten for you | | ||
| | `.canGrow` | `canGrow()`, and expect what was bound — not `false` at the row limit | | ||
| | `.canGrow = …` / `.maxRows = …` / `.freeRowsHeight = …` | Bind them in the template; the inputs are read-only | | ||
|
|
||
| **`maxRows` and `freeRowsHeight` report `number | undefined`.** Both were declared non-nullable while an unbound textarea held `undefined`, and `maxRowLimitReached` compared against it — `rowsCount > undefined` is false, which is why unlimited growth worked at all. | ||
|
|
||
| **`freeRowsHeight` no longer writes itself.** It defaulted to the measured line height by assigning its own input in `ngOnInit`; the fallback is a computed now, so binding it later actually takes effect instead of being overwritten on the next init. | ||
|
|
||
| **The `kbq-textarea_max-row-limit-reached` class follows the row count directly.** It is derived from a signal written inside `runOutsideAngular`, so the class used to wait for an unrelated change detection pass to appear. | ||
|
|
||
| **Generated ids changed shape**, from `kbq-textarea-1` to `kbq-textarea-a1`. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The generated-id change does not happen for consumers — CDK's if (this._appId !== 'ng') { prefix += this._appId; }with the CDK's own comment "Omit the app ID if it's the default The per-prefix counter also still starts at A consumer who greps their snapshots for Mirrored at |
||
|
|
||
| Handled by `textarea-signals`: the value-safe reads are rewritten, the rest is reported. | ||
|
|
||
| #### Title | ||
|
|
||
| `kbq-title` measures its host and opens a tooltip when the text is truncated. The review kept that surface — the `kbq-title` input and the tooltip it opens — and closed the measurement machinery behind it. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -308,6 +308,40 @@ describe('KbqTextarea', () => { | |
|
|
||
| expect(getTextareaElement(fixture).classList.contains('kbq-textarea_max-row-limit-reached')).toBe(false); | ||
| }); | ||
|
|
||
| it('should treat a valueless canGrow attribute as true', () => { | ||
| const fixture = createComponent(KbqTextareaValuelessCanGrow); | ||
|
|
||
| fixture.detectChanges(); | ||
|
|
||
| const textarea = fixture.debugElement.query(By.directive(KbqTextarea)).injector.get(KbqTextarea); | ||
|
|
||
| expect(textarea.canGrow()).toBe(true); | ||
| expect(getTextareaElement(fixture).classList.contains('kbq-textarea-resizable')).toBe(false); | ||
| }); | ||
|
|
||
| it('should report the bound canGrow rather than folding in the row limit', () => { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The regression test for the headline change passes identically against the old code. It asserts It can't be reached here at all: jsdom returns Stubbing |
||
| const fixture = createComponent(KbqTextareaGrowWithMaxRows); | ||
|
|
||
| fixture.detectChanges(); | ||
|
|
||
| const textarea = fixture.debugElement.query(By.directive(KbqTextarea)).injector.get(KbqTextarea); | ||
|
|
||
| expect(textarea.canGrow()).toBe(true); | ||
| expect(textarea.maxRowLimitReached()).toBe(false); | ||
| }); | ||
|
|
||
| it('should leave maxRows and freeRowsHeight undefined when unbound', () => { | ||
| const fixture = createComponent(KbqTextareaForBehaviors); | ||
|
|
||
| fixture.detectChanges(); | ||
|
|
||
| const textarea = fixture.debugElement.query(By.directive(KbqTextarea)).injector.get(KbqTextarea); | ||
|
|
||
| expect(textarea.maxRows()).toBeUndefined(); | ||
| expect(textarea.freeRowsHeight()).toBeUndefined(); | ||
| expect(textarea.maxRowLimitReached()).toBe(false); | ||
| }); | ||
| }); | ||
|
|
||
| describe('grow behavior', () => { | ||
|
|
@@ -547,3 +581,15 @@ describe('KbqTextarea', () => { | |
| })); | ||
| }); | ||
| }); | ||
|
|
||
| @Component({ | ||
| imports: [KbqTextareaModule, KbqFormFieldModule, FormsModule], | ||
| template: ` | ||
| <kbq-form-field> | ||
| <textarea kbqTextarea canGrow [(ngModel)]="value"></textarea> | ||
| </kbq-form-field> | ||
| ` | ||
| }) | ||
| class KbqTextareaValuelessCanGrow { | ||
| value = ''; | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rewritten intro contradicts the subsections it introduces.
The
####subsections actually present in §18 are: Popover, Search expandable, Split button, Textarea, Title, Toast, Tooltip. Six of those seven are named in the first-wave list in the same sentence, so the guide now assigns six already-shipped migrations to the wrong release wave. Only the Textarea subsection is new.Mirrored verbatim at
migration.ru.md:1042. Naming the second wave explicitly ("the second covered textarea") avoids the contradiction and survives the next component review.