feat(textarea): add md-textarea element #1562

fxck · 2016-10-21T21:51:49Z

I went with the having one class and multiple selector way. Also added the autosize / autoexpand directive. Modified demo page and README aswell.

I suppose some tests are needed, I'm not really good at those, so I'd appreciate assistance with that.

Should close #546

cc @jelbourn

fxck · 2016-10-21T22:06:42Z

Not sure why this particular test is failing, nothing I've done is affecting the name attribute..

jelbourn

First pass of comments.

Unit tests should be pretty straightforward based on the existing tests; you should start with a few basic ones for the new attributes and then add tests for the autosize once we hammer out the behavior.

jelbourn · 2016-10-21T22:54:45Z

src/lib/input/input.ts

+  selector: 'textarea[mdAutosize]'
+})
+export class MdAutosize implements OnInit {
+  @HostListener('input', ['$event.target'])


You shouldn't need the second argument to @HostListener

jelbourn · 2016-10-21T22:58:52Z

src/lib/input/input.html

@@ -34,6 +35,38 @@
             [(ngModel)]="value"
             (change)="_handleChange($event)">

+      <textarea


Just FYI: we're probably going to do away with these internal elements and content-project an input from the user.

jelbourn · 2016-10-21T22:59:41Z

src/lib/input/input.scss

@@ -95,6 +95,10 @@ md-input {
  }
 }

+.md-input-element--textarea {


Stick with single dash for now- I'm going to update all of the CSS at once for the no-conflict stuff.

jelbourn · 2016-10-21T23:00:59Z

src/lib/input/input.ts

@@ -30,6 +32,9 @@ import {Observable} from 'rxjs/Observable';

 const noop = () => {};

+const MD_INPUT_SELECTOR = 'md-input';
+const MD_TEXTAREA_SELECTOR = 'md-textarea';


You can just use the strings inline without introducing constants; the values are obvious and aren't going to change frequently.

Well, it was for my own convenience, but ok.

jelbourn · 2016-10-21T23:16:59Z

src/lib/input/input.html

+             (focus)="_handleFocus($event)"
+             (blur)="_handleBlur($event)"
+             [(ngModel)]="value"
+             (change)="_handleChange($event)"></textarea>


You can remove min, max, and autocorrect. They don't exist on <textarea>

We should add rows, cols, and wrap. Need to think about how rows interacts with autosize.

https://developer.mozilla.org/en-US/docs/Web/HTML/Element/textarea

jelbourn · 2016-10-21T23:18:57Z

src/lib/input/input.ts

+})
+export class MdAutosize implements OnInit {
+  @HostListener('input', ['$event.target'])
+  public onChange() {


I don't see where this onChange method is called.

It's what the @HostListener is calling. Or you mean it should be private? I suppose..

jelbourn · 2016-10-21T23:19:36Z

src/lib/input/input.ts

+
+  private _adjust() {
+    this._elRef.nativeElement.style.overflow = 'hidden';
+    this._elRef.nativeElement.style.height = 'auto';


These two properties don't need to be set every time.

overflow I does not, height = auto does, it's what causes the textarea shrink when you remove text

jelbourn · 2016-10-21T23:21:16Z

src/lib/input/input.ts

+  private _adjust() {
+    this._elRef.nativeElement.style.overflow = 'hidden';
+    this._elRef.nativeElement.style.height = 'auto';
+    this._elRef.nativeElement.style.height = `${this._elRef.nativeElement.scrollHeight}px`;


The logic for sizing the textarea is much more complicated in material1, which makes me think this may be missing something.

@crisbeto do you have insights on this?

Seems to be working fine, seems to be supported in all browsers as well https://developer.mozilla.org/en-US/docs/Web/API/Element/scrollHeight

Well.. it's considerably more complex when you have to deal with the rows attribute. So there are two options, not allowing autosize and rows on the same element(because honestly, who's using rows nowadays, it's only good for devices not supporting css) or go with the 10 times more complex code. I started working on the latter, but I'd be much happier with the former.

Correct, most of the complexity is because of the rows, however I think that they're a pretty important part of the component, because they're what sets an empty textarea apart from an input. In Material 1 we also do the following, although I'm not the one who wrote it so I'm not sure what the reasoning is:

function getHeight() { var offsetHeight = node.offsetHeight; var line = node.scrollHeight - offsetHeight; return offsetHeight + Math.max(line, 0); }

crisbeto · 2016-10-22T09:22:02Z

src/lib/input/input.ts

+    // if line height is explicitly set(to a pixel value), use that
+    if (computedStyles.lineHeight && computedStyles.lineHeight.toLowerCase().indexOf('px') >= 0) {
+      // return stripped of the "px" and as a number
+      return +computedStyles.lineHeight.replace('px', '');


I think you can achieve the same if you used parseFloat(computedStyles.lineHeight).

Yeah, that might be faster.

crisbeto · 2016-10-22T09:23:28Z

src/lib/input/input.ts

+    );
+
+    // fill in one row
+    tempEl.innerHTML = '-';


Wouldn't it be better to use the value property?

Well since I won't ever need the actual value, it doesn't really matter.. having said that, value is apparently marginally faster https://jsperf.com/innerhtml-vs-value-on-textarea so yeah.

fxck · 2016-10-22T09:45:42Z

@jelbourn I have modified the autosize directive to work along rows attribute.. it's a little complex so I'll try to explain how it works:

A row's height is calculated from line-height of the element, but unless the line-height is explicitly set, its initial value is normal, so to be able to dynamically set rows depending on the height of the textarea, I need to find the actual line-height.

So I'm creating a temp textarea element, reseting it's paddings and margins, visually hiding it, setting the rows attribute to 1(default is 2, but I'm not sure how consistent is it across browsers, so I'm setting it explicitly) and getting it's height, removing the element afterwards. When line-height is actually explicitly set(doesn't matter if it's by ems, pixels, percents, getComputedStyles always returns pixel values), I'm simply returning it as a number and doing none of above.

Now that I have actual line-height, I'm setting the min-height of the element to be

user provided rows value * line-height I found

The rest is similar to how it was before when input event happens, height is set to auto and rows to the initial value, which allows me to get the real scrollHeight even if user is removing instead of adding characters, afterwards new rows is calculated by diving the new scrollHeight by lineHeight.

Also none of above happens unless rows is explicitly provided by the user.

// offtopic
trying to develop this lib is hell, demo-app doesn't output anything, it's crazy slow, oftentimes doesn't properly pickup changes so I have to stop and start the process, not very developer friendly at all

fxck · 2016-10-22T12:59:14Z

I tried adding a test for the rows attribute but I can't get it to succeed.

  it('supports the rows attribute', () => {
    let fixture = TestBed.createComponent(MdInputTextareaWithRows);
    let input: MdInput = fixture.debugElement.query(By.directive(MdInput)).componentInstance;

    fixture.detectChanges();

    let el: HTMLInputElement = fixture.debugElement.query(By.css('textarea')).nativeElement;
    expect(el).not.toBeNull();

    fixture.detectChanges();
    expect(el.getAttribute('rows')).toEqual(null);

    input.rows = '8';

    fixture.detectChanges();

    expect(el.getAttribute('rows')).toEqual('8');

  });

@Component({template: `<md-textarea [rows]="rows"></md-textarea>`})
class MdInputTextareaWithRows { }

fails with "expected null to equal '8'" or something

jelbourn · 2016-10-26T22:19:57Z

@fxck do you want to break the autosize into a separate PR? I want to merge the textarea, but need some more time to review/think about the autosize logic.

fxck · 2016-10-26T22:22:29Z

Yeah I can, but I still can't figure out those tests..

jelbourn · 2016-10-26T22:26:51Z

I don't see anything obviously wrong with the test; if you rebase the PR I can try to take a look

fxck · 2016-10-26T22:41:11Z

@jelbourn should be rebased + autosize removed

jelbourn

Looks like you need @Input properties for rows, cols, and wrap:

  // textarea-specific
  @Input() rows: number = null;
  @Input() cols: number = null;
  @Input() wrap: string = null;

As for unit tests, I tried this out on your PR and it passes (added to the end of the top-level describe block:

  describe('md-textarea', () => {
    it('supports the rows, cols, and wrap attributes', () => {
      let fixture = TestBed.createComponent(MdTextareaWithBindings);

      fixture.detectChanges();

      const textarea: HTMLTextAreaElement = fixture.nativeElement.querySelector('textarea');
      expect(textarea.rows).toBe(4);
      expect(textarea.cols).toBe(8);
      expect(textarea.wrap).toBe('hard');
    });
  });

With this test component at the end of the file (and then added to the testing module):

@Component({template:
    `<md-textarea [rows]="rows" [cols]="cols" [wrap]="wrap" placeholder="Snacks"></md-textarea>`})
class MdTextareaWithBindings {
  rows: number = 4;
  cols: number = 8;
  wrap: string = 'hard';
}

jelbourn · 2016-10-26T23:05:56Z

src/lib/input/README.md

+which enables auto expanding functionality.
+
+```html
+<md-textarea autosize placeholder="Textarea with autosize"></md-textarea>


Should remove autosize from the readme for this PR

jelbourn · 2016-10-26T23:08:03Z

src/lib/input/input.scss

@@ -95,6 +95,10 @@ md-input {
  }
 }

+.md-input-element-textarea {
+  min-height: 60px;
+}


Where does this 60px come from? I don't see anything in the spec and it looks like material1 uses a min-height to match the line-height. Can we remove it?

I guess it should be equal to two rows(which is default for rows), which is line-height * 2.

But line-height can be overriden by user etc.. so I guess I'll remove it completely

60px was just a number that looked nice and reasonable in the demo

jelbourn · 2016-10-26T23:10:28Z

src/lib/input/input.ts

@@ -229,6 +229,10 @@ export class MdInput implements ControlValueAccessor, AfterContentInit, OnChange

  @ViewChild('input') _inputElement: ElementRef;

+  public elementType: 'input' | 'textarea' = undefined;
+
+  constructor(private _elRef: ElementRef) { }


If you move the setting of elementType here, you don't need the private and can just call it elementRef.

jelbourn · 2016-10-26T23:11:50Z

src/lib/input/input.ts

+      this.elementType = 'input';
+    } else {
+      this.elementType = 'textarea';
+    }


You should be able to do this in the constructor. I'd also make it a bit more concise with a ternary:

this.elementType = this._elementRef.nativeElement.nodeName.toLowerCase() === 'md-input' ? 'input' : 'textarea';

jelbourn · 2016-10-26T23:13:22Z

src/lib/input/input.ts

@@ -229,6 +229,10 @@ export class MdInput implements ControlValueAccessor, AfterContentInit, OnChange

  @ViewChild('input') _inputElement: ElementRef;

+  public elementType: 'input' | 'textarea' = undefined;


You can omit the public (as the default). Can you also prefix this with an underscore (which we're using with the absence of the private keyword as "internal").

fxck · 2016-10-26T23:40:50Z

I had those inputs there, must have accidentally deleted them while rebasing.. anyway addressed all comments.

@jelbourn

jelbourn

LGTM aside from one last minor comment

jelbourn · 2016-10-26T23:44:39Z

src/lib/input/input.ts

@@ -229,6 +234,15 @@ export class MdInput implements ControlValueAccessor, AfterContentInit, OnChange

  @ViewChild('input') _inputElement: ElementRef;

+  _elementType: 'input' | 'textarea';
+
+  constructor(public _elementRef: ElementRef) {


It can just be

constructor(elementRef: ElementRef) {

Adding a public or private modifier here makes it a field on the class which isn't necessary.

Huh, can it? Changed.

fxck · 2016-10-26T23:50:36Z

Gah, needs rebase now.. sec.

fxck · 2016-10-27T00:00:06Z

Done, passing, all good I guess.

jelbourn · 2016-10-27T00:01:33Z

LGTM

jelbourn · 2016-10-27T00:02:14Z

Thanks!

angular-automatic-lock-bot · 2019-09-06T03:01:16Z

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.}

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Oct 21, 2016

jelbourn reviewed Oct 21, 2016

View reviewed changes

crisbeto reviewed Oct 22, 2016

View reviewed changes

fxck mentioned this pull request Oct 23, 2016

Multiline md-input or textarea #1575

Closed

fxck force-pushed the md-textarea branch from 80a477d to 99c3093 Compare October 26, 2016 22:38

jelbourn reviewed Oct 26, 2016

View reviewed changes

jelbourn added the pr: needs cleanup / tests label Oct 26, 2016

jelbourn reviewed Oct 26, 2016

View reviewed changes

Ales Rechtorik added 12 commits October 27, 2016 01:50

add textarea

e84365c

adress comments

42b7592

make autosize work along rows

f53e654

change innerHTML to value

17c2761

replace replace with parseFloat

ddda4ed

fix "supports a name attribute test"

41cfdd5

remove unused import

6f2354c

change rows in textarea demo to by dynamic

ba71282

rebase + remove autosize

262785c

remove autosize from demo-app

acac45e

remove unused imports

4784301

address comments, add tests

95e6bab

Ales Rechtorik added 3 commits October 27, 2016 01:52

prefix element type with underscore

6bf5613

proper type of wrap

43a6762

remove public from element ref injection

88a560b

fxck force-pushed the md-textarea branch from 565baf0 to 88a560b Compare October 26, 2016 23:52

jelbourn added pr: lgtm and removed pr: needs cleanup / tests labels Oct 27, 2016

jelbourn merged commit aff22e5 into angular:master Oct 27, 2016

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

		@@ -229,6 +229,10 @@ export class MdInput implements ControlValueAccessor, AfterContentInit, OnChange

		@ViewChild('input') _inputElement: ElementRef;

		public elementType: 'input' \| 'textarea' = undefined;

feat(textarea): add md-textarea element #1562

feat(textarea): add md-textarea element #1562

Conversation

fxck commented Oct 21, 2016 • edited Loading

fxck commented Oct 21, 2016

jelbourn left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fxck commented Oct 22, 2016 • edited Loading

fxck commented Oct 22, 2016

jelbourn commented Oct 26, 2016

fxck commented Oct 26, 2016

jelbourn commented Oct 26, 2016

fxck commented Oct 26, 2016

jelbourn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fxck Oct 26, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fxck commented Oct 26, 2016

jelbourn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fxck commented Oct 26, 2016

fxck commented Oct 27, 2016

jelbourn commented Oct 27, 2016

jelbourn commented Oct 27, 2016

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

fxck commented Oct 21, 2016 •

edited

Loading

jelbourn left a comment •

edited

Loading

fxck commented Oct 22, 2016 •

edited

Loading

fxck Oct 26, 2016 •

edited

Loading