create md-input-container to eventually replace md-input #2052

mmalerba · 2016-12-01T20:44:42Z

No description provided.

hansl

That's a good start ^_^

hansl · 2016-12-01T23:07:35Z

src/demo-app/baseline/baseline-demo.html

@@ -10,7 +10,7 @@
    | Text 3 |
    <md-radio-button value="option_3">Radio 3</md-radio-button>
    | Text 4 |
-    <md-input>Label</md-input>
+    <md-input-wrapper><input placeholder="Input"></md-input-wrapper>


2 things;

since we're not remove the input completely (yet), could you leave 1 here and add the new wrapper?

could you insert new lines and tabulation? it might affect the baseline.

hansl · 2016-12-01T23:09:25Z

src/lib/input/input-wrapper.ts

+  }
+}
+
+export class MdInputWrapperPlaceholderConflictError extends MdError {


Seems like those are not imported in input.ts; I think they should be the same class since they're technically the same error.

I don't see the harm in keeping them separate for now, especially since we're just going to delete the md-input version shortly. It would be weird for MdInput to throw MdInputWrapper* errors

hansl · 2016-12-01T23:17:00Z

src/lib/input/input-wrapper.ts

+  @ContentChild(NgModel) _ngModelChild: NgModel;
+
+  /** Whether the `input` or `textarea` is focused. */
+  _focused = false;


I don't think we need that anymore, since the input is now part of the content tree.

I need this so I can update the focused state of the underline, sadly the DOM structure prevents me from doing it in pure CSS

hansl · 2016-12-01T23:19:01Z

src/lib/input/input-wrapper.ts

+  private _inputElement: HTMLInputElement | HTMLTextAreaElement;
+
+  /** A `MutationObserver` to observe the `input` or `textarea`. */
+  private _inputObserver = new MutationObserver(mutations => {


This might make web workers freak out. @jelbourn what's the proper way of handling mutations like those on elements from the content tree? Create a service with an Observable (that would be my guess)?

I'm not worried about it until there's better web-worker support in Angular. Pretty much everything md-input does is DOM-based, so trying to run the code on a worker threat will probably end up being slower due to marshaling.

hansl · 2016-12-01T23:19:48Z

src/lib/input/input-wrapper.ts

+  /** Initialize the `input` or `textarea` element. */
+  private _initInputEl() {
+    // Find input or textarea element.
+    let inputEls = this._elementRef.nativeElement.querySelectorAll('input, textarea');


@jelbourn Refresh my memory that we cannot use @ContentChild()? I thought we could but might have been mistaken.

If we can't use @ContentChild we'll need to wrap this whole piece in a service.

We can't use @ContentChild unless we want to require the user to put a directive on the input. (Which we could consider if we don't like all this MutationObserver stuff)

hansl · 2016-12-01T23:20:51Z

src/lib/input/input-wrapper.ts

+
+  /** The type attribute of the `input` (or "text" if element is a `textarea`). */
+  private get _inputType(): string { return this._getterSetterOnlyInputType; }
+  private set _inputType(value: string) {


Keeping an internal value for the input type is useless; you can just validate it when the value changes.

I'm keeping a copy so that it's easy to isolate the code that needs access to the input element. I read this value rather than directly reading the type attribute in the _empty getter.

jelbourn

Initial pass

jelbourn · 2016-12-02T21:34:39Z

src/lib/input/input-wrapper.ts

+
+export class MdInputWrapperInputElementCountError extends MdError {
+  constructor(count: number) {
+    super(`md-input-wrapper must contain exactly 1 input or textarea element. Found ${count}.`);


In a follow-up, we'll also want to support using a custom element as an input. E.g., if I make my own component <helpful-input>, we need a way for people to specify that their component acts as the <input> here.

Thinking about it more, though, maybe that means it would be simpler to require a directive on the input element, since we wouldn't be able to rely on the native attributes and events in that case. (I didn't think of this until having my sync with Pantheon yesterday afternoon and they mentioned wanting to do something like this).

jelbourn · 2016-12-02T21:39:33Z

src/lib/input/input-wrapper.ts

+  selector: 'md-hint',
+  host: {
+    '[class.md-right]': 'align == "end"',
+    '[class.md-hint]': 'true'


You can do

host: { 'class': 'md-hint', '[class.md-right]': 'align == "end"', }

And it won't destroy whatever other classes were set on there (when class is static and not a binding).

switched to @HostBinding / @HostListener (since I realized that @HostListener actually does have a way to pass variables).

jelbourn · 2016-12-02T22:17:21Z

src/lib/input/input-wrapper.ts

+    this._inputElement = inputEls[0];
+
+    // Install mutation observer and event listeners and subscribe to ngModel changes.
+    this._inputObserver.observe(this._inputElement, {


Call disconnect in ngOnDestroy?

jelbourn · 2016-12-02T22:20:26Z

src/lib/input/input-wrapper.ts

+  _inputRequired = false;
+
+  /** Whether the `input` or `textarea` is empty. */
+  get _empty(): boolean { return !this._inputValue && this._inputType !== 'date'; }


Also include datetime, time, month, and week. We should probably break it out into a method like isNeverEmptyInputType or something similar.

Oh, and datetime-local

This won't necessarily be true on browsers that don't support those types, though. E.g. only Chrome supports type="date". That's why I went with the feature test in #1860.

jelbourn · 2016-12-02T22:22:37Z

src/lib/input/input-wrapper.ts

+const MD_INPUT_INVALID_INPUT_TYPE = [
+  'file',
+  'radio',
+  'checkbox',


Also "button", "submit", "reset", "color", "range", "image", "hidden"

jelbourn · 2016-12-02T22:28:46Z

src/lib/input/input-wrapper.ts

+  private _inputElement: HTMLInputElement | HTMLTextAreaElement;
+
+  /** A `MutationObserver` to observe the `input` or `textarea`. */
+  private _inputObserver = new MutationObserver(mutations => {


I'm not worried about it until there's better web-worker support in Angular. Pretty much everything md-input does is DOM-based, so trying to run the code on a worker threat will probably end up being slower due to marshaling.

jelbourn · 2016-12-02T22:30:17Z

src/lib/input/input-wrapper.ts

+          this._inputRequired = this._inputElement.required;
+          break;
+      }
+    }


I'd move this function to the class, though, i.e.

private _inputObserver = new MutationObserver(mutations => this._updateFromInputMutation(mutations));

jelbourn · 2016-12-02T22:45:31Z

src/lib/input/input-wrapper.ts

+      attributes: true,
+      attributeFilter: ['disabled', 'id', 'type', 'placeholder', 'required']
+    });
+    for (let eventType in this._inputListeners) {


I think you don't need this extra handling around this._inputListeners; it's less code overall (and less indirection) to just do

inputElement.addEventListener('blur', () => { this._focused = false; }); inputElement.addEventListener('focus', () => { this._focused = true; }); inputElement.addEventListener('input', () => { this._inputValue = this._inputElement.value; });

It's not really relevant anymore, but I like this pattern when I'm adding and removing a group of listeners at the same time. That way I don't end up with a bunch of boundXListener variables just so I can remove them at the end, I can just iterate over the map removing them

jelbourn · 2016-12-02T22:48:50Z

src/lib/input/input-wrapper.ts

+  }
+
+  /** Initialize the `input` or `textarea` element. */
+  private _initInputEl() {


_initializeInputElement
(generally prefer the unabbreviated form)

mmalerba · 2016-12-06T00:15:29Z

addressed comments, PTAL

hansl · 2016-12-06T01:18:43Z

LGTM, I'll let @jelbourn do the rest of the review.

jelbourn

Meta comment: thought about it some more, think we should go with md-input-container; let's chat tomorrow about it.

jelbourn · 2016-12-06T03:36:06Z

src/lib/input/input-wrapper.ts

+    featureTestInput.setAttribute('type', value);
+    return featureTestInput.type === value;
+  });
+})();


Move this to a separate getSupportedInputTypes function? Maybe under core/platform

jelbourn · 2016-12-06T03:37:34Z

src/lib/input/input-wrapper.ts

+  constructor(align: string) {
+    super(`A hint was already declared for 'align="${align}"'.`);
+  }
+}


Let's move these errors to their own file (which most of the other components do).

jelbourn · 2016-12-06T03:41:13Z

src/lib/input/input-wrapper.ts

+  @Input()
+  value: any;
+
+  @Output() placeholderChange = new EventEmitter<string>();


Should this be internal (_placeholderChange)? Also needs a description as to what it's for.

jelbourn · 2016-12-06T03:44:01Z

src/lib/input/input-wrapper.ts

+  focused = false;
+
+  private get _uid() { return this._cachedUid = this._cachedUid || `md-input-${nextUniqueId++}`; }
+  private _cachedUid: string;


Why do you need a separate _uid field instead of setting this.id if it is unset in ngOnInit?

I was just trying to be extra defensive. I figured someone could do [id]="myId" and myId would initially have some value but it could theoretically update at some point in the future, so if it updates to null later on this will make sure we still put an id on it.

jelbourn · 2016-12-06T03:46:14Z

src/lib/input/input-wrapper.ts

+  private get _uid() { return this._cachedUid = this._cachedUid || `md-input-${nextUniqueId++}`; }
+  private _cachedUid: string;
+
+  constructor(private _elementRef: ElementRef, @Optional() private _ngModel: NgModel) {


Instead of injection NgModel, you want to inject FormControl (which should have the same APIs that you're using).

If I use FormControl it seems like the valueChanges.subscribe method is not called for an input with [(ngModel)]="value"

I'll wait for @kara's input

jelbourn · 2016-12-06T03:50:41Z

src/lib/input/input-wrapper.ts

+    // Force setter to be called in case id was not specified.
+    this.id = this.id;
+
+    if (this._ngModel) {


I'm not sure you need this; you should be able to inject NG_VALUE_ACCESSOR on the input and return the value from that in a readonly getter (I don't quite follow why this directive needs an @Input for value).

cc @kara in case there's a better way

jelbourn · 2016-12-06T03:52:41Z

src/lib/input/input-wrapper.ts

+  }
+
+  /** Focus the input element. */
+  focus() { this._elementRef.nativeElement.focus(); }


Might as well go through the renderer for this (#2077)

jelbourn · 2016-12-06T03:59:29Z

src/lib/input/input-wrapper.spec.ts

+    fixture.componentInstance.placeholder = 'Other placeholder';
+    fixture.detectChanges();
+
+    waitForMutationObserver(fixture, () => {


waitForMutationObserver obsolete now?

mmalerba · 2016-12-07T19:00:39Z

Comments addressed

jelbourn · 2016-12-07T19:36:48Z

src/lib/core/platform/feature-detector.ts

+
+
+@Injectable()
+export class MdFeatureDetector {


No Md prefix for this, since it's not really coupled to Material Design.
(I plan on removing all of the Md prefixes from core utilities soon)

jelbourn · 2016-12-07T19:38:24Z

src/lib/core/platform/feature-detector.ts

+@Injectable()
+export class MdFeatureDetector {
+  /** @returns {Set<string>} the input types supported by this browser. */
+  get supportedInputTypes(): Set<string> {


What's your motivation for making a class rather than individual exported functions similar to coerceBooleanProperty?

I worry that having a MdFeatureDetector class will make people want to start using it as a replacement for modernizr and then complain that it doesn't do what they want.

mmalerba · 2016-12-07T23:09:18Z

Addressed comments & rebased

jelbourn · 2016-12-07T23:19:11Z

LGTM, as discussed we'll hold off on merging until after alpha.11

AsherBarak · 2016-12-20T07:12:16Z

Following 20ee360, Is a version release to npm expected? Current version (beta 11) works the old way.

jelbourn · 2016-12-20T07:14:41Z

Soon

OlliL · 2017-01-07T22:23:02Z

How is it supposed to work?

I replaced

      <md-input placeholder="Username"
                [(ngModel)]="username" 
                (keyup.enter)="doLogin()"></md-input>

by

    <md-input-container>
      <input mdInput
             placeholder="Username"
             (keyup.enter)="doLogin()"
             [(ngModel)]="username">
    </md-input-container>

And the style changed from:

to:

Is this intended? I'm using "@angular/material": "^2.0.0-beta.1"

abdulhaq-e · 2017-01-07T22:51:51Z

mdInput => md-input

See the examples here: https://material.angular.io/components/component/input

OlliL · 2017-01-07T22:56:22Z

The link states

must be marked with the mdInput directive.

And also does the README at:
https://github.com/angular/material2/blob/master/src/lib/input/README.md

kara · 2017-01-07T23:11:01Z

@OlliL The README is relevant to what's on master only. You'll want to look at the docs site (linked by @abdulhaq-e) for the documentation of the latest release.

OlliL · 2017-01-07T23:27:27Z

I see.
Having multiple <input> elements in one <md-input-container> seems not to render nicely. I'll go for multiple <md-input-container> elements - one for each <input> element for now. This seems to work now fine. Thanks!

angular-automatic-lock-bot · 2019-09-06T06:03:04Z

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

mmalerba added the pr: needs review label Dec 1, 2016

mmalerba assigned hansl and jelbourn Dec 1, 2016

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

hansl suggested changes Dec 1, 2016

View reviewed changes

jelbourn reviewed Dec 2, 2016

View reviewed changes

This was referenced Dec 2, 2016

feature(md-input): refactor MdInput as an attribute #1858

Closed

fix(input): prevent overlapping placeholder on inputs with native masking #1860

Closed

mmalerba force-pushed the input branch from dcfb3b8 to a1c7e69 Compare December 5, 2016 18:53

hansl approved these changes Dec 6, 2016

View reviewed changes

jelbourn reviewed Dec 6, 2016

View reviewed changes

mmalerba force-pushed the input branch from 953c25d to 4bdfcb0 Compare December 6, 2016 22:48

jelbourn reviewed Dec 7, 2016

View reviewed changes

mmalerba force-pushed the input branch from 86ccf92 to 7db5b10 Compare December 7, 2016 23:08

jelbourn added pr: lgtm and removed pr: needs review labels Dec 7, 2016

mmalerba added the action: merge The PR is ready for merge by the caretaker label Dec 9, 2016

mmalerba added 7 commits December 9, 2016 14:55

created md-input-wrapper and removed useless inputs

dd76026

added demo

ea45082

remove ControlValueAccessor and start migrating input

e88f608

finished converting most input attrs

4cd9381

typescript fixed, css needs work

f1adbdf

removed duplicate directives.

3fd291b

fix scss

9b31865

mmalerba added 16 commits December 9, 2016 14:56

try removing ie11 checks, see if we need them

937186c

added md-input back to baseline demo

2c21d52

started md-input directive

8bf8d02

more refactoring

25e85e8

fixed demos, tests still broken

700577a

remove useless imports

4646be6

fix tests

202b020

fix tests for ff & ie

754e701

move input feature detection into service

aa64775

addressed some comments

dbb6731

s/input-wrapper/input-container

956e8a5

s/NgModel/NgControl

d2534d5

remove FeatureDetector

9010085

s/MdPlatformModule/PlatformModule

e352175

Remove some problematic privates and add mat- versions of everything

68ef6eb

fix import

a6325a6

mmalerba force-pushed the input branch from ecc7997 to a6325a6 Compare December 9, 2016 23:04

mmalerba merged commit ca2046b into angular:master Dec 9, 2016

mmalerba changed the title ~~create md-input-wrapper to eventually replace md-input~~ create md-input-container to eventually replace md-input Dec 14, 2016

mmalerba mentioned this pull request Dec 14, 2016

remove md-input component in favor of md-input-container #2217

Closed

mmalerba deleted the input branch December 25, 2016 23:37

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



		@Injectable()
		export class MdFeatureDetector {

create md-input-container to eventually replace md-input #2052

create md-input-container to eventually replace md-input #2052

Conversation

mmalerba commented Dec 1, 2016

hansl left a 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

mmalerba Dec 2, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

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

crisbeto Dec 4, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmalerba commented Dec 6, 2016

hansl commented Dec 6, 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

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

mmalerba commented Dec 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmalerba commented Dec 7, 2016

jelbourn commented Dec 7, 2016

AsherBarak commented Dec 20, 2016

jelbourn commented Dec 20, 2016

OlliL commented Jan 7, 2017

abdulhaq-e commented Jan 7, 2017

OlliL commented Jan 7, 2017

kara commented Jan 7, 2017 • edited Loading

OlliL commented Jan 7, 2017

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

mmalerba Dec 2, 2016 •

edited

Loading

crisbeto Dec 4, 2016 •

edited

Loading

kara commented Jan 7, 2017 •

edited

Loading