feat(cdk-input): change autosize to be bindable (#9884) #11167

pfeigl · 2018-05-04T20:51:18Z

This allows cdkTextareaAutosize selector to be bindable and toggle the internal
enabled / disabled state.

Upon disabling the initial height is restored (textarea.style.height before any changes
were applied).

Also added wrapper into (to be discontinued) matTextareaAutosize.

This allows cdkTextareaAutosize selector to be bindable and toggle the internal enabled / disabled state. Upon disabling the initial height is restored (textarea.style.height before any changes were applied). Also added wrapper into (to be discontinued) matTextareaAutosize. Fixes angular#9884 Currently missing unit tests, thus still WIP. Manual tests in demo-app seem to work as expected

pfeigl · 2018-05-05T00:56:36Z

@mmalerba Added unit tests and squashed commits, this should be ready for review if builds / tests pass.

mmalerba

Looks good, just some nits

mmalerba · 2018-05-08T15:55:38Z

src/cdk/text-field/autosize.spec.ts

+
+    fixtureWithoutAutosize.detectChanges();
+
+    let previousHeight = textarea.clientHeight;


nit: can be const since never re-assigned

Changed, is there any specific reason for not having https://palantir.github.io/tslint/rules/prefer-const/ enabled in tslint?

There's probably just a bunch of code that needs to be updated before we can enable it. But I don't see any reason not to

Just found this on that topic: #5959
Seems to have been discussed in the past and decided against doing it. Tried adding it here and autofixing it which worked flawlessly but was actually really a huge chunk of changes.

mmalerba · 2018-05-08T15:57:20Z

src/cdk/text-field/autosize.spec.ts

+        .toEqual(previousHeight, 'Expected textarea to still have the same size.');
+    expect(textarea.clientHeight)
+        .toBeLessThan(textarea.scrollHeight,
+          'Expected textarea height to be less than its scrollHeight');


'Expected textarea to have scrollbar.'

Changed, good improvement in readability. Honestly change based my text off of the first unit test which states "Expected textarea height to match its scrollHeight".

mmalerba · 2018-05-08T15:58:06Z

src/cdk/text-field/autosize.spec.ts

+    fixtureWithoutAutosize.detectChanges();
+
+    expect(textarea.clientHeight)
+        .toBeGreaterThan(previousHeight, 'Expected textarea to have grown after enabling.');


'...after enabling autosize.'

ups, good catch, changed

mmalerba · 2018-05-08T15:58:43Z

src/cdk/text-field/autosize.spec.ts

+    expect(textarea.clientHeight)
+        .toBeGreaterThan(previousHeight, 'Expected textarea to have grown after enabling.');
+    expect(textarea.clientHeight)
+        .toBe(textarea.scrollHeight, 'Expected textarea height to match its scrollHeight');


'Expected textarea not to have scrollbar.'

mmalerba · 2018-05-08T15:59:09Z

src/cdk/text-field/autosize.spec.ts

+    fixtureWithoutAutosize.detectChanges();
+
+    expect(textarea.clientHeight)
+        .toEqual(previousHeight, 'Expected textarea to again have the original size.');


remove 'again'

mmalerba · 2018-05-08T15:59:31Z

src/cdk/text-field/autosize.spec.ts

+        .toEqual(previousHeight, 'Expected textarea to again have the original size.');
+    expect(textarea.clientHeight)
+        .toBeLessThan(textarea.scrollHeight,
+            'Expected textarea height to be less than its scrollHeight again');


'Expected textarea to have scrollbar.'

mmalerba · 2018-05-08T16:04:08Z

src/cdk/text-field/autosize.ts

  private readonly _destroyed = new Subject<void>();

  private _minRows: number;
  private _maxRows: number;
+  private _enabled: boolean = true;
+
+  private get textarea(): HTMLTextAreaElement {


Instead of this, change the constructor:

constructor(elementRef, ...) { this._textareaElement = elementRef.nativeElement as HTMLTextAreaElement; }

Good idea and changed. Doesn't really make to big of a difference in the transpiled js because the cast is removed anyways, but yea, way cleaner this way.

mmalerba · 2018-05-08T16:12:59Z

src/cdk/text-field/autosize.ts

+   * Resets the textarea to it's original size
+   */
+  reset() {
+    if (this._initialHeight === undefined) {


Can you add a comment explaining why we skip this if it's undefined?

Added a comment for it to be more clear why this was added

pfeigl · 2018-05-08T18:20:03Z

Sent a new commit with the required changes, thanks for the feedback.

rafaelss95 · 2018-05-08T21:58:30Z

src/lib/input/autosize.ts

+
+  @Input()
+  get matTextareaAutosize(): boolean { return this.enabled; }
+  set matTextareaAutosize(value: boolean) { this.enabled = value; }


coerceBooleanProperty?

enabled is a setter within CdkTextareaAutosize which internally uses coerceBooleanProperty

rafaelss95 · 2018-05-08T21:58:39Z

src/lib/input/autosize.ts

+
+  @Input('mat-autosize')
+  get matAutosize(): boolean { return this.enabled; }
+  set matAutosize(value: boolean) { this.enabled = value; }


coerceBooleanProperty?

enabled is a setter within CdkTextareaAutosize which internally uses coerceBooleanProperty

rafaelss95 · 2018-05-08T21:59:15Z

src/lib/input/autosize.ts

@@ -34,4 +34,12 @@ export class MatTextareaAutosize extends CdkTextareaAutosize {
  @Input()
  get matAutosizeMaxRows(): number { return this.maxRows; }
  set matAutosizeMaxRows(value: number) { this.maxRows = value; }
+
+  @Input('mat-autosize')


Is this correctly named, @mmalerba?

Should be, see the selector definition: selector: 'textarea[mat-autosize], textarea[matTextareaAutosize]',

Aha, yes :)

angular-automatic-lock-bot · 2019-09-08T23:02:45Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

pfeigl requested a review from mmalerba as a code owner May 4, 2018 20:51

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label May 4, 2018

pfeigl force-pushed the autosize-bindable branch from 384d2e8 to 329a40f Compare May 5, 2018 00:54

pfeigl force-pushed the autosize-bindable branch from 329a40f to 01d1658 Compare May 5, 2018 00:54

pfeigl changed the title ~~WIP: feat(cdk-input): change autosize to be bindable (#9884)~~ feat(cdk-input): change autosize to be bindable (#9884) May 5, 2018

mmalerba reviewed May 8, 2018

View reviewed changes

Small code quality improvements (angular#9884)

3b111e9

mmalerba approved these changes May 8, 2018

View reviewed changes

mmalerba added pr: lgtm action: merge The PR is ready for merge by the caretaker target: minor This PR is targeted for the next minor release labels May 8, 2018

rafaelss95 reviewed May 8, 2018

View reviewed changes

tinayuangao merged commit 2d227b7 into angular:master May 15, 2018

pfeigl deleted the autosize-bindable branch May 15, 2018 20:00

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 8, 2019


		fixtureWithoutAutosize.detectChanges();

		let previousHeight = textarea.clientHeight;

feat(cdk-input): change autosize to be bindable (#9884) #11167

feat(cdk-input): change autosize to be bindable (#9884) #11167

Uh oh!

Conversation

pfeigl commented May 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pfeigl commented May 5, 2018

Uh oh!

mmalerba left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pfeigl commented May 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rafaelss95 May 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

angular-automatic-lock-bot bot commented Sep 8, 2019

Uh oh!

Uh oh!

pfeigl commented May 4, 2018 •

edited

Loading

rafaelss95 May 8, 2018 •

edited

Loading